Feature/100 0 n columns #111

salamonpavel · 2023-11-03T14:15:00Z

Closes #100
Introduces support for 0-n columns (depends on individual measure).

github-actions · 2023-11-03T14:17:21Z

JaCoCo agent module code coverage report - spark:2 - scala 2.12.12

File	Coverage [85.5%]	🍏
MeasurementBuilder.scala	100%	🍏
AtumContext.scala	97.03%	🍏
Measure.scala	89.17%	🍏
MeasuresBuilder.scala	85.51%	🍏
Measurement.scala	67.56%	❌

Total Project Coverage	81.07%	🍏

github-actions · 2023-11-03T14:17:23Z

JaCoCo server module code coverage report - scala 2.12.12

There is no coverage information present for the Files changed

Total Project Coverage	12.45%	❌

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala

lsulak · 2023-11-08T15:40:55Z

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala

-    measureName: String,
-    resultValueType: ResultValueType.ResultValueType
-  ) extends Measure {
+    private val columnExpression = countDistinct(col(controlCols.head), controlCols.tail.map(col): _*)


I'm still not sure if this is what we want. I thought that it's about 'distinct -> count' operation, performed on a single column. Do you remember what was used in the old Atum & what can our users require @benedeki ?

With the suggested code it can be performed on single column or multiple columns. If this particular measure is expected to work differently please let me know. @benedeki

I like the universality of supporting multi-column uniqueness 👍

lsulak · 2023-11-08T16:01:29Z

Code is based off of feature/99-custom-measure-2.

Why not to repoint the PR then? It would be easier to do the review

salamonpavel · 2023-11-08T16:05:12Z

Why not to repoint the PR then? It would be easier to do the review

Good point, will change the target branch to be the feature/99-custom-measure-2.

# Conflicts: # agent/src/main/scala/za/co/absa/atum/agent/model/Measurement.scala # agent/src/main/scala/za/co/absa/atum/agent/model/MeasuresBuilder.scala

agent/src/main/scala/za/co/absa/atum/agent/model/Measurement.scala

lsulak

I finished the review, I like the changes. There is only one tiny minor thing I found, and 1 thing regarding distinct count functionality. Otherwise LGTM and I will be happy to approve once these are addressed.

…cala Co-authored-by: Ladislav Sulak <[email protected]>

benedeki

To consider:
I like class hierarchies. What you think of having 3 abstract classes

MeasureWithoutColumn
MeasureOneColumn
MeasureMultipleColumns

benedeki · 2023-11-15T09:09:15Z

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala

-    measureName: String,
-    resultValueType: ResultValueType.ResultValueType
-  ) extends Measure {
+    private val columnExpression = countDistinct(col(controlCols.head), controlCols.tail.map(col): _*)


I like the universality of supporting multi-column uniqueness 👍

benedeki · 2023-11-15T09:14:56Z

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala

    def apply(controlCol: String): AbsSumOfValuesOfColumn = AbsSumOfValuesOfColumn(measureName, controlCol)
  }

  case class SumOfHashesOfColumn private (measureName: String, controlCol: String) extends AtumMeasure {
-
+    private val columnExpression: Column = sum(crc32(col(controlCol).cast("String")))


Could this function also support a list of columns, concatenated.
Extra bonus is * for all columns. 😉

miroslavpojer

pulled
built
test run
code coverage - required merge of conflicting files, there were changes in tests
- in current test implementation I see not covered paths

I will continue in review after merge of conflict files.

miroslavpojer · 2023-11-20T07:30:35Z

agent/src/test/scala/za/co/absa/atum/agent/model/MeasurementTest.scala

@@ -28,15 +28,15 @@ class MeasurementTest extends AnyFlatSpec with Matchers with SparkTestBase { sel

  "MeasurementProvided" should "be able to be converted to MeasurementProvided object when the result is Double" in {


Test speak about Double, but works with Big Decimals

benedeki

I think it's ok.
Just decide uf you want to add the hash function on multiple columns to add here or separate ticket+PR.

Oh and there's Miroslav's comment on one of the tests.

benedeki · 2023-12-10T17:55:18Z

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala

+  private def handleAggregationResult(dataType: DataType, result: Any): String = {
+    val aggregatedValue = dataType match {
+      case _: LongType =>
+        if (result == null) 0 else result
+      case _: StringType =>
        val value =
-          if (collected == null) new java.math.BigDecimal(0)
-          else collected.asInstanceOf[java.math.BigDecimal]
+          if (result == null) new java.math.BigDecimal(0)
+          else result.asInstanceOf[java.math.BigDecimal]
        value.stripTrailingZeros // removes trailing zeros (2001.500000 -> 2001.5, but can introduce scientific notation (600.000 -> 6E+2)
          .toPlainString // converts to normal string (6E+2 -> "600")
      case _ =>


For the record, this we might want to revisit and rethink. But not part of this PR. 😉

salamonpavel · 2023-12-28T09:43:59Z

I will re-point this PR to master instead of feature/99-custom-measure-2.

github-actions · 2023-12-28T12:06:32Z

JaCoCo agent module code coverage report - spark:2 - scala 2.12.18

File	Coverage [89.28%]	🍏
MeasurementBuilder.scala	100%	🍏
AtumContext.scala	97.07%	🍏
Measure.scala	89.39%	🍏
MeasuresBuilder.scala	84.73%	🍏
AtumAgent.scala	84.52%	🍏
Measurement.scala	82.18%	🍏

Total Project Coverage	77.08%	❌

github-actions · 2023-12-28T12:06:34Z

JaCoCo server module code coverage report - scala 2.12.18

There is no coverage information present for the Files changed

Total Project Coverage	0%	❌

salamonpavel added 5 commits November 1, 2023 14:07

#99: CustomMeasure

3849ccd

#99: CustomMeasure

f1007c9

#99: Measure refactored

3437219

#99: forCustomMeasure test

82a027f

#100: support for 0-n columns

afd63ec

salamonpavel marked this pull request as ready for review November 3, 2023 14:20

salamonpavel requested review from benedeki, lsulak, TebaleloS, Zejnilovic and dk1844 as code owners November 3, 2023 14:20

salamonpavel added 2 commits November 8, 2023 15:36

#99: Reformat code in Measurement.scala to follow guidelines

9c688e2

#99: Remove unused import

e8f7de8

lsulak reviewed Nov 8, 2023

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala Show resolved Hide resolved

lsulak reviewed Nov 8, 2023

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/model/Measure.scala Show resolved Hide resolved

lsulak reviewed Nov 8, 2023

View reviewed changes

salamonpavel changed the base branch from master to feature/99-custom-measure-2 November 8, 2023 16:05

salamonpavel added 2 commits November 9, 2023 08:59

#99: Change the way MeasureDTO is imported

d7fd394

Merge branch 'feature/99-custom-measure-2' into feature/100-0-n-columns

1861e95

# Conflicts: # agent/src/main/scala/za/co/absa/atum/agent/model/Measurement.scala # agent/src/main/scala/za/co/absa/atum/agent/model/MeasuresBuilder.scala

lsulak reviewed Nov 9, 2023

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/model/Measurement.scala Outdated Show resolved Hide resolved

lsulak reviewed Nov 9, 2023

View reviewed changes

Update agent/src/main/scala/za/co/absa/atum/agent/model/Measurement.s…

5df402e

…cala Co-authored-by: Ladislav Sulak <[email protected]>

benedeki reviewed Nov 15, 2023

View reviewed changes

miroslavpojer reviewed Nov 20, 2023

View reviewed changes

benedeki reviewed Dec 10, 2023

View reviewed changes

salamonpavel changed the base branch from feature/99-custom-measure-2 to master December 28, 2023 09:44

merge conflicts with master resolved

31bcc28

minor

a69bba4

benedeki approved these changes Dec 29, 2023

View reviewed changes

salamonpavel merged commit 4057443 into master Jan 2, 2024
8 of 10 checks passed

salamonpavel deleted the feature/100-0-n-columns branch January 2, 2024 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/100 0 n columns #111

Feature/100 0 n columns #111

salamonpavel commented Nov 3, 2023 •

edited

Loading

github-actions bot commented Nov 3, 2023

github-actions bot commented Nov 3, 2023

lsulak Nov 8, 2023

salamonpavel Nov 9, 2023

benedeki Nov 15, 2023

lsulak commented Nov 8, 2023

salamonpavel commented Nov 8, 2023

lsulak left a comment

benedeki left a comment

benedeki Nov 15, 2023

benedeki Nov 15, 2023

miroslavpojer left a comment •

edited

Loading

miroslavpojer Nov 20, 2023

benedeki left a comment

benedeki Dec 10, 2023

salamonpavel commented Dec 28, 2023

github-actions bot commented Dec 28, 2023 •

edited

Loading

github-actions bot commented Dec 28, 2023

		@@ -28,15 +28,15 @@ class MeasurementTest extends AnyFlatSpec with Matchers with SparkTestBase { sel

		"MeasurementProvided" should "be able to be converted to MeasurementProvided object when the result is Double" in {

Feature/100 0 n columns #111

Feature/100 0 n columns #111

Conversation

salamonpavel commented Nov 3, 2023 • edited Loading

github-actions bot commented Nov 3, 2023

JaCoCo agent module code coverage report - spark:2 - scala 2.12.12

github-actions bot commented Nov 3, 2023

JaCoCo server module code coverage report - scala 2.12.12

lsulak Nov 8, 2023

Choose a reason for hiding this comment

salamonpavel Nov 9, 2023

Choose a reason for hiding this comment

benedeki Nov 15, 2023

Choose a reason for hiding this comment

lsulak commented Nov 8, 2023

salamonpavel commented Nov 8, 2023

lsulak left a comment

Choose a reason for hiding this comment

benedeki left a comment

Choose a reason for hiding this comment

benedeki Nov 15, 2023

Choose a reason for hiding this comment

benedeki Nov 15, 2023

Choose a reason for hiding this comment

miroslavpojer left a comment • edited Loading

Choose a reason for hiding this comment

miroslavpojer Nov 20, 2023

Choose a reason for hiding this comment

benedeki left a comment

Choose a reason for hiding this comment

benedeki Dec 10, 2023

Choose a reason for hiding this comment

salamonpavel commented Dec 28, 2023

github-actions bot commented Dec 28, 2023 • edited Loading

JaCoCo agent module code coverage report - spark:2 - scala 2.12.18

github-actions bot commented Dec 28, 2023

JaCoCo server module code coverage report - scala 2.12.18

salamonpavel commented Nov 3, 2023 •

edited

Loading

miroslavpojer left a comment •

edited

Loading

github-actions bot commented Dec 28, 2023 •

edited

Loading