#247: Paging support in Reader module #316

benedeki · 2025-02-04T10:05:12Z

classes abstracting work with paging and rolling through them
RequestResult class refactoring
package basic renamed to core

Requirement for #247

Release notes:

Added paging support to the module.

* classes abstracting work with paging and rolling through them * `RequestResult` class refactoring

github-actions · 2025-02-04T10:08:47Z

JaCoCo model module code coverage report - scala 2.13.11

Overall Project	58.76%	🍏

There is no coverage information present for the Files changed

github-actions · 2025-02-04T10:08:48Z

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project	78.2%	🍏

There is no coverage information present for the Files changed

github-actions · 2025-02-04T10:08:49Z

JaCoCo reader module code coverage report - scala 2.13.11

Overall Project	96.76%	🍏
Files changed	45.29%	❌

File	Coverage
PartitioningReader.scala	100%	🍏
FlowReader.scala	100%	🍏
AbstractPage.scala	100%	🍏
PartitioningIdProvider.scala	100%	🍏
Reader.scala	100%	🍏
ReaderException.scala	100%	🍏
RequestException.scala	100%	❌
GroupedPage.scala	99.21% `-98.02%`	❌
Page.scala	97.01% `-94.78%`	❌
RequestResult.scala	68.18%	🍏

github-actions · 2025-02-04T10:08:50Z

JaCoCo server module code coverage report - scala 2.13.11

Overall Project	68.39%	🍏

There is no coverage information present for the Files changed

…aOSS/atum-service into feature/247-paging-in-reader

salamonpavel · 2025-02-12T09:15:00Z

reader/src/main/scala/za/co/absa/atum/reader/exceptions/RequestException.scala

+import sttp.model.{StatusCode, Uri}
+import za.co.absa.atum.model.envelopes.ErrorResponse
+
+abstract class RequestException(message: String) extends ReaderException(message)


We could make this class sealed to restrict its subclassing to the same file. This can help in maintaining control over the hierarchy and ensuring that all subclasses are known and defined in one place.

Scalafmt formatting is off (hasn't been applied).

salamonpavel · 2025-02-12T09:26:56Z

reader/src/main/scala/za/co/absa/atum/reader/result/AbstractPage.scala

+    def pageEnd: Long
+
+    def pageSize: Int = (pageEnd - pageStart).toInt + 1
+    def hasPrior: Boolean = pageStart > 0


What if pageStart is 0 or negative but the absolute value is still lower than pageSize? Could this happen?

salamonpavel · 2025-02-12T09:29:59Z

reader/src/main/scala/za/co/absa/atum/reader/result/GroupedPage.scala

+
+import scala.collection.immutable.ListMap
+
+case class GroupedPage[K, V, F[_]: MonadError](


Can you elaborate on the usage of this class? What do we need it for? I often try to limit the scope of my PRs as much as possible.

salamonpavel · 2025-02-12T09:40:28Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+
+  def next: F[RequestResult[Page[T, F]]] = next(limit)
+
+  def +(other: Page[T, F]): Page[T, F] = {


I think it's not really needed at this point if even in the future ...

salamonpavel · 2025-02-12T09:41:09Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+                                      private[reader] val pageRoller: PageRoller[T, F]
+                                    ) extends AbstractPage[Vector[T], F] {
+
+  def apply(index: Int): T = items(index)


Is it wise to provide apply which might lead to IndexOutOfBoundsExceptions? Iterating over Seq is preferable ...

salamonpavel · 2025-02-12T09:42:44Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+    this.copy(items = newItems, pageRoller = newPageRoller)
+  }
+
+  def prior(newPageSize: Int): F[RequestResult[Page[T, F]]] = {


I don't think prior is needed. But I might be wrong or maybe I am just not seeing the usage. When I think of Unify I cannot imagine that the backend calls prior given it's stateless.

As mentioned in Teams I am of the opinion that we should force forward-only consumption pattern to try to avoid repetitive calls to db.

salamonpavel · 2025-02-12T09:44:53Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+
+  def apply(index: Int): T = items(index)
+
+  def map[B](f: T => B): Page[B, F] = {


Why do you expect the user to use map on Page if items can be mapped easily anyway?

salamonpavel · 2025-02-12T09:46:43Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+    this.copy(items = newItems, hasNext = newHasNext, pageStart = newPageStart, pageEnd = newPageEnd)
+  }
+
+  def groupBy[K](f: T => K): GroupedPage[K, T, F] = {


Maybe we could discuss these mapping/grouping stuff first on our tech call?

I honestly like it and find it very universal

salamonpavel · 2025-02-12T09:47:05Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+}
+
+object Page {
+  type PageRoller[T, F[_]] = (Int, Long) => F[RequestResult[Page[T, F]]]


lsulak · 2025-02-17T09:29:05Z

reader/src/main/scala/za/co/absa/atum/reader/core/PartitioningIdProvider.scala

@@ -23,7 +23,7 @@ import za.co.absa.atum.model.envelopes.SuccessResponse.SingleSuccessResponse
 import za.co.absa.atum.model.types.basic.AtumPartitions
 import za.co.absa.atum.model.types.basic.AtumPartitionsOps
 import za.co.absa.atum.model.utils.JsonSyntaxExtensions.JsonSerializationSyntax
-import za.co.absa.atum.reader.basic.RequestResult.RequestResult
+import RequestResult.RequestResult


I'd prefer this style to be explicit, WDYT?

import za.co.absa.atum.reader.core.RequestResult.RequestResult

I am also in favor of using fully qualified imports.

lsulak · 2025-02-17T12:00:12Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+  def groupBy[K](f: T => K): GroupedPage[K, T, F] = {
+    val (newItems, itemsCounts) = items.foldLeft(ListMap.empty[K, Vector[T]], 0) { case ((groupsAcc, count), item) =>
+      val k = f(item)
+      (groupsAcc.updated(k, groupsAcc.getOrElse(k, Vector.empty) :+ item), count + 1)


consider extracting groupsAcc.getOrElse(k, Vector.empty) into a val, so that this line doesn't do so much

lsulak · 2025-02-17T12:01:52Z

reader/src/main/scala/za/co/absa/atum/reader/result/Page.scala

+  }
+
+  def groupBy[K](f: T => K): GroupedPage[K, T, F] = {
+    val (newItems, itemsCounts) = items.foldLeft(ListMap.empty[K, Vector[T]], 0) { case ((groupsAcc, count), item) =>


Hmm any plans with the itemsCounts ? It looks potentially useful but it's nowhere used yet. Alternatively you can just delete and simplify the whole foldLeft block

BTW, if you decide to use and keep it, I'd propose to rename count to countAcc, just as a common practice (at least mine :D )

lsulak · 2025-02-17T12:14:13Z

reader/src/test/scala/za/co/absa/atum/reader/core/PartitioningIdProviderUnitTests.scala

@@ -38,7 +39,7 @@ class PartitioningIdProviderUnitTests extends AnyFunSuiteLike {
  private val atumPartitionsToNotFound = AtumPartitions(List.empty)

  private implicit val serverConfig: ServerConfig = ServerConfig(serverUrl)
-  private implicit val monad: IdMonad.type = IdMonad
+  private implicit val monad: MonadError[Identity] = IdMonad


very clever-the whole file, just read it for the first time

Why ReaderWithPartitioningIdForTest(partitioning: AtumPartitions) must have the partitioning in constructor? It seems like unnecessary duplicity potentially

Also in line 71:

val result: Long = response.getOrElse(throw new Exception("Failed to get partitioning id"))

I think in tests this is unnecessary, I'd just do something like: val result: Long = response.get

it's test that simulates something in well-isolated and very specific & deterministic scenario

Line 91: isn't this a bit more readable?

result.left.map(e => assert(e.isInstanceOf[ParsingException])) instead of swap

lsulak · 2025-02-17T14:57:34Z

reader/src/test/scala/za/co/absa/atum/testing/data/PageData.scala

+    TestItem(4, "cc")
+  )
+
+  def roller(limit: Int, offset: Long): RequestResult[Page[TestItem, Identity]] = {


don't you want to make this function more universal? It only works with offset 0 or 6

lsulak · 2025-02-17T14:58:49Z

reader/src/test/scala/za/co/absa/atum/reader/result/AbstractPageUnitTests.scala

+    assert(!page.hasPrior)
+
+    val anotherPage = new AbstractPage[Iterable[Int], Identity] {
+      override def items: Iterable[Int] = Seq(1, 2, 3)


I'm thinking whether this logically makes sense. I understand it, pages can overlap, okay, for the test cases let's be it, but the page starts at 1 and ends at 2, so it should have only Seq(2, 3) right?

I'd propose to implement logically correct tests so that they are understood & the intention for the code under test is well understood - tests are a sort of a documentation as well

lsulak · 2025-02-17T15:00:06Z

reader/src/test/scala/za/co/absa/atum/reader/result/GroupedPageUnitTests.scala

@@ -0,0 +1,179 @@
+/*
+ * Copyright 2025 ABSA Group Limited


lsulak · 2025-02-17T15:00:21Z

reader/src/test/scala/za/co/absa/atum/reader/result/PageUnitTests.scala

@@ -0,0 +1,89 @@
+/*
+ * Copyright 2025 ABSA Group Limited


lsulak · 2025-02-17T15:00:26Z

reader/src/test/scala/za/co/absa/atum/testing/implicits/AbstractPageImplicits.scala

@@ -0,0 +1,51 @@
+/*
+ * Copyright 2025 ABSA Group Limited


lsulak · 2025-02-17T15:00:33Z

reader/src/test/scala/za/co/absa/atum/testing/implicits/RequestResultImplicits.scala

@@ -0,0 +1,37 @@
+/*
+ * Copyright 2025 ABSA Group Limited


lsulak · 2025-02-17T15:01:15Z

reader/src/test/scala/za/co/absa/atum/testing/data/PageData.scala

@@ -0,0 +1,72 @@
+/*
+ * Copyright 2025 ABSA Group Limited


lsulak · 2025-02-17T15:08:24Z

reader/src/test/scala/za/co/absa/atum/reader/result/GroupedPageUnitTests.scala

+      hasNext = true,
+      limit = 6,
+      pageStart = 0,
+      pageEnd = 5


logically speaking, what is pageEnd here, what does it represent? Number of all items in the ListMap, or just number of 'keys', i.e. 3 or 6 ? (minus 1 since we index from 0, so 2 or 5) ?

I guess the overall number of items so that this is correct, just wanted to double check

lsulak · 2025-02-17T15:15:13Z

reader/src/test/scala/za/co/absa/atum/reader/result/GroupedPageUnitTests.scala

+      pageEnd = 5
+    )
+
+    val Right(priorPage2) = nextPage2.prior(7)


Why don't you test scenrio when the limit is for example 4?

lsulak · 2025-02-17T15:19:09Z

reader/src/test/scala/za/co/absa/atum/reader/result/GroupedPageUnitTests.scala

+        5 -> Vector(PageData.TestItem(5, "dd"))
+      ),
+      hasNext = false,
+      limit = 6,


is this logically correct? Number of items is higher than 6

#247: Paging support in Reader module

1488d1f

* classes abstracting work with paging and rolling through them * `RequestResult` class refactoring

benedeki requested review from lsulak, Zejnilovic, dk1844, salamonpavel and ABLL526 as code owners February 4, 2025 10:05

benedeki self-assigned this Feb 4, 2025

benedeki and others added 2 commits February 4, 2025 11:05

* some omitted files

96ffa33

Merge branch 'master' into feature/247-paging-in-reader

8b69e3e

benedeki added 4 commits February 4, 2025 11:57

* filenames check exclusions fix

04b56bd

Merge branch 'feature/247-paging-in-reader' of https://github.com/Abs…

698b765

…aOSS/atum-service into feature/247-paging-in-reader

* further fix

855a333

* fix trial 3

be90711

benedeki mentioned this pull request Feb 4, 2025

#247 Implement basics of FlowReader #306

Open

* Licenses fix

afd64a6

benedeki linked an issue Feb 11, 2025 that may be closed by this pull request

Implement basics of FlowReader #247

Open

salamonpavel reviewed Feb 12, 2025

View reviewed changes

lsulak reviewed Feb 17, 2025

View reviewed changes

reader/src/test/scala/za/co/absa/atum/testing/data/PageData.scala

@@ -0,0 +1,72 @@

/*

* Copyright 2025 ABSA Group Limited

Copy link

Collaborator

lsulak Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2021

lsulak reviewed Feb 17, 2025

View reviewed changes


		import scala.collection.immutable.ListMap

		case class GroupedPage[K, V, F[_]: MonadError](


		def next: F[RequestResult[Page[T, F]]] = next(limit)

		def +(other: Page[T, F]): Page[T, F] = {


		def apply(index: Int): T = items(index)

		def map[B](f: T => B): Page[B, F] = {

#247: Paging support in Reader module #316

Are you sure you want to change the base?

#247: Paging support in Reader module #316

Conversation

benedeki commented Feb 4, 2025 • edited Loading

github-actions bot commented Feb 4, 2025

JaCoCo model module code coverage report - scala 2.13.11

github-actions bot commented Feb 4, 2025

JaCoCo agent module code coverage report - scala 2.13.11

github-actions bot commented Feb 4, 2025 • edited Loading

JaCoCo reader module code coverage report - scala 2.13.11

github-actions bot commented Feb 4, 2025

JaCoCo server module code coverage report - scala 2.13.11

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salamonpavel Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedeki commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

salamonpavel Feb 12, 2025 •

edited

Loading

lsulak Feb 17, 2025 •

edited

Loading