Introducing Map for error columns and their values #85

benedeki · 2023-03-14T09:30:02Z

Relatively big overwrite to use map instead of errCol and sequence of raw values
ErrorMessageSubmitJustErrorValue class created to offer the ability to submit errors without source column but with error value
Some other changes to improve the code

After the presentation, discussions with others and more thoughts, I came to the idea to combine the errCol (which would be better as sequence anyway) and rawValues into one column of map data type. That resulted in a need to do some more exercise in the supporting types, but I believe to for the better.

Also some general code improvements were added as I stumbled upon stuff during the refactoring.

…e of raw values * `ErrorMessageSubmitJustErrorValue` class created to offer the ability to submit errors without source column but with error value

TebaleloS · 2023-03-14T10:36:58Z

...-commons/src/main/scala/za/co/absa/spark/commons/errorhandling/partials/EvaluateViaUdf.scala

     errorMessageSubmit.additionInfo.column
   )
  }
 }

 object EvaluateViaUdf {
-  type ErrorMessageFunction[T] = (ErrType, ErrCode, ErrMsg, ErrCol, RawValues, AdditionalInfo) => T //TODO needed?
+  type ErrorMessageFunction[T] = (ErrType, ErrCode, ErrMsg, ErrColsAndValues, AdditionalInfo) => T //TODO needed?
 }


Please a new line after line 43

I would argue here it's really not needed.

TebaleloS · 2023-03-14T10:38:59Z

...rc/main/scala/za/co/absa/spark/commons/errorhandling/partials/EvaluateIntoErrorMessage.scala

+    val additionInfo = "additionInfo"
+    val errCols = "errCols"
+    val errValues = "errValues"
+  }

 }


Please add a new line at the end.

TebaleloS · 2023-03-14T10:40:40Z

...ons/src/main/scala/za/co/absa/spark/commons/errorhandling/partials/ErrorHandlingCommon.scala

Please add a newline at the end (after line 58)

Same as above 😉

TebaleloS · 2023-03-14T10:41:48Z

...za/co/absa/spark/commons/errorhandling/implementations/ErrorMessageSubmitOnMoreColumns.scala

+      ColumnOrValue.withOption(additionalInfo)
+    )
+  }
+}


Add a new line after line 44

It's there. File has 45 lines. .editorconfig file is actually set to ensure it's always there.

TebaleloS · 2023-03-14T10:43:16Z

...cala/za/co/absa/spark/commons/errorhandling/implementations/ErrorMessageSubmitOnColumn.scala

+      ColumnOrValue.withValue(errType),
+      ColumnOrValue.withValue(errCode),
+      ColumnOrValue.withValue(errMessage),
+      errColName,
      ColumnOrValue.withOption(additionalInfo)
    )
  }


Please add a new line after line 39

Can do that, if it improves reading. But to be honest I probably forget more often 😊

TebaleloS · 2023-03-14T10:45:40Z

spark-commons/src/main/scala/za/co/absa/spark/commons/errorhandling/types/ColumnOrValue.scala

    val isColumn: Boolean = false
    val isValue: Boolean = true
-    val getColumnName: Option[String] = None
+    val columnNames: Set[String] = Set.empty
    val getValue: Option[T] = None
  }
 }


Add a new line at the end

TebaleloS · 2023-03-14T10:46:27Z

spark-commons/src/main/scala/za/co/absa/spark/commons/errorhandling/types/types.scala

  //mapping is missing, should be part of AdditionalInfo, as being very specific

+  type ColumnTransformer = String => Column
+
  //This is to ensure some level of type-safety
  final case class ErrorColumn(column: Column) extends AnyVal
 }


Add a new line at the end

TebaleloS · 2023-03-14T10:48:17Z

spark-commons/src/main/scala/za/co/absa/spark/commons/sql/functions.scala

+    lit(None.orNull).cast(dataType)
+  }
+
+
 }


Please add a new line at the end and remove unnecessary new line (either line 70 or 71)

TebaleloS

Everything else look cool to me except for minor stuff about the unnecessary new lines and missing new lines at the end of every add file.

miroslavpojer · 2023-03-15T12:43:16Z

...rc/main/scala/za/co/absa/spark/commons/errorhandling/partials/EvaluateIntoErrorMessage.scala

@@ -18,18 +18,30 @@ package za.co.absa.spark.commons.errorhandling.partials

 import org.apache.spark.sql.Column
 import org.apache.spark.sql.functions.struct
-import za.co.absa.spark.commons.errorhandling.ErrorMessageSubmit
+import za.co.absa.spark.commons.errorhandling.{ErrorMessage, ErrorMessageSubmit}


Suggested change

import za.co.absa.spark.commons.errorhandling.{ErrorMessage, ErrorMessageSubmit}

import za.co.absa.spark.commons.errorhandling.ErrorMessageSubmit

Removed import not used in code.

miroslavpojer

pulled
build
code review

jirifilip

Hello, David, nice code. I attached my comments but don't take them too seriously, I don't have that much experience in Scala 😃

jirifilip · 2023-03-15T13:55:57Z

spark-commons/src/main/scala/za/co/absa/spark/commons/errorhandling/ErrorMessage.scala

- * @param additionInfo - Sequence of Mappings i.e Mapping Table Column -> Equivalent Mapped Dataset column
+ * @param errColsAndValues - The names of the columns where the error occurred and their raw values (which are the
+ *                         potential culprits of the error)
+ * @param additionInfo - any optional additional information in the form of a JSON string
 */
 case class ErrorMessage(


I was thinking if it wouldn't be better to use the types directly instead of type aliases. These type aliases might add some overhead for the users. I'm a bit stupid so I would be constantly forgetting what type ErrType really is 😄

Reason why I used type aliases was that I wasn't sure we are settled on the types. This way it is rather easy to change.

Also the AdditionalInfo actually is planned to be more then alias eventually (#90)

I am happy to discuss the other aliases once the thing is settled (aliases should be easy to remove) 👍

jirifilip · 2023-03-15T14:08:05Z

...rc/main/scala/za/co/absa/spark/commons/errorhandling/implementations/ErrorMessageArray.scala

+    dataFrame.withColumn(errorColName, reMap(array_union(deMap(col(errorColName)), colToUnion)))
+  }
+
+  protected def doTheAggregation(dataFrame: DataFrame, errCols: Column*): DataFrame = {


I was thinking if we should use the word "aggregation" here and also in ErrorHandling trait. So I searched the dictionary for the definition of the word and it fits here but when I first started reading the code, I thought that there would be some df.groupBy(...).agg(...), i.e. some squishing of rows and then some calculation 😄 But if I understood correctly, it's more for combining the errors into one column (or actually, I guess it depends on the implementation).

Thanks. Actually you inspired me, as it's a little an internal function it can be little more eloquent. Hopefully it will improve the understanding. 😉

* `ErrorMessageSubmitWithoutColumn` changed from `case class` to `class` to allow inheritance * some PR comments addressed

benedeki · 2023-03-16T17:47:31Z

Moving the the main PR, general direction approved in direct discussions.

* Relatively big overwrite to use map instead of errCol and sequenc…

2db8c95

…e of raw values * `ErrorMessageSubmitJustErrorValue` class created to offer the ability to submit errors without source column but with error value

benedeki requested review from Zejnilovic, dk1844 and lsulak as code owners March 14, 2023 09:30

benedeki requested review from miroslavpojer and TebaleloS March 14, 2023 09:30

* Forgotten register function call

51b16f4

TebaleloS reviewed Mar 14, 2023

View reviewed changes

* line ends improved

4783fca

miroslavpojer reviewed Mar 15, 2023

View reviewed changes

jirifilip reviewed Mar 15, 2023

View reviewed changes

* ErrorMessageSubmits moved to submits sub-package

c8c4aa0

* `ErrorMessageSubmitWithoutColumn` changed from `case class` to `class` to allow inheritance * some PR comments addressed

benedeki merged commit 8e747e7 into feature/83-create-a-spike-for-error-handling Mar 16, 2023

benedeki deleted the feature/83-create-a-spike-for-error-handling-map-errcol branch March 16, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing Map for error columns and their values #85

Introducing Map for error columns and their values #85

benedeki commented Mar 14, 2023 •

edited

Loading

TebaleloS Mar 14, 2023

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023 •

edited

Loading

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023

benedeki Mar 14, 2023

TebaleloS Mar 14, 2023

TebaleloS left a comment •

edited

Loading

miroslavpojer Mar 15, 2023

miroslavpojer left a comment •

edited

Loading

jirifilip left a comment

jirifilip Mar 15, 2023

benedeki Mar 16, 2023

jirifilip Mar 15, 2023

benedeki Mar 16, 2023

benedeki commented Mar 16, 2023

	import za.co.absa.spark.commons.errorhandling.{ErrorMessage, ErrorMessageSubmit}
	import za.co.absa.spark.commons.errorhandling.ErrorMessageSubmit

Introducing Map for error columns and their values #85

Introducing Map for error columns and their values #85

Conversation

benedeki commented Mar 14, 2023 • 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

TebaleloS Mar 14, 2023 • 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

TebaleloS left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavpojer left a comment • edited Loading

Choose a reason for hiding this comment

jirifilip left a 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 Mar 16, 2023

benedeki commented Mar 14, 2023 •

edited

Loading

TebaleloS Mar 14, 2023 •

edited

Loading

TebaleloS left a comment •

edited

Loading

miroslavpojer left a comment •

edited

Loading