Skip to content

Avoid capturing default config in numeric parser UDF#110

Open
qwerty44-U wants to merge 8 commits into
AbsaOSS:masterfrom
qwerty44-U:fix-date-udf-default-config-serialization
Open

Avoid capturing default config in numeric parser UDF#110
qwerty44-U wants to merge 8 commits into
AbsaOSS:masterfrom
qwerty44-U:fix-date-udf-default-config-serialization

Conversation

@qwerty44-U

@qwerty44-U qwerty44-U commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • avoid capturing the full StandardizationConfig in the numeric parser UDF closure
  • pass the narrower ErrorCodesConfig through UDFResult.fromTry, while preserving the existing StandardizationConfig overload
  • add a serialization regression covering DefaultStandardizationConfig

Release Notes

  • Fixed numeric parser UDF serialization when using the default standardization config.

Verification

  • git diff --check

Scala/Spark tests were not run locally because this environment does not have Java, sbt, or Maven installed (java, sbt, and mvn are not available on PATH).

Fixes #57

targetType: String,
pattern: Option[String],
errorCodes: ErrorCodesConfig,
defaultValue: Option[T] = None): UDFResult[T] = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @qwerty44-U Thank you for the contribution , seems there's compile errors could you please look at them, I've pulled it and the compile error UDFResult.scala:28:8: in object UDFResult, multiple overloaded alternatives of method fromTry define default arguments. is due to this line and the one above L40-L41 the two alternatives of fromTry declare a default on defaultValue causing a clash

@dk1844 dk1844 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the general idea, but please improve

vParser,
vColumnNullable,
vColumnNameForError,
vErrorCodes,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with simplifying the UDF, but I would just pass the stdConfig.errorCodes

Suggested change
vErrorCodes,
stdConfig.errorCodes,

columnNullable: Boolean,
columnNameForError: String,
stdConfig: StandardizationConfig,
errorCodes: ErrorCodesConfig,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change makes sense, just the ErrorCodesConfig type is enough here.

Comment on lines +46 to +51
val vErrorCodes = BasicErrorCodesConfig(
stdConfig.errorCodes.castError,
stdConfig.errorCodes.nullError,
stdConfig.errorCodes.typeError,
stdConfig.errorCodes.schemaError
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I really do not see the point of this change with recreating the error basic config -- seems to me that we end up with the very same result vs using stdConfig.errorCodes directly

@qwerty44-U

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I pushed updates addressing the feedback.

What changed:

  • Kept numericParserToTyped narrowed to ErrorCodesConfig.
  • Removed the BasicErrorCodesConfig(...) reconstruction.
  • Capture stdConfig.errorCodes once as vErrorCodes before building the UDF, so the UDF does not capture the full StandardizationConfig while still using the configured error codes directly.
  • Fixed the overloaded UDFResult.fromTry default-argument compile issue.
  • Added coverage for both ErrorCodesConfig and StandardizationConfig UDFResult.fromTry paths, plus executing the numeric UDF through Spark in the default-config regression.

Verification from the latest run on commit 359010a:

  • Scala 2.12.18: passed
  • Scala 2.13.11: passed
  • test (2.12.20, 2.12, 80, 80, 60): passed
  • license-test: passed
  • check-release-notes: passed

The remaining Scalafmt Check failure is still the checkout issue: the workflow tries to checkout fix-date-udf-default-config-serialization from AbsaOSS/spark-data-standardization, but that branch exists on the fork, so it exits before running Scalafmt.

@MatloaItumeleng MatloaItumeleng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good , thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardization can fail with Task not serializable when casting to date is requested

4 participants