Avoid capturing default config in numeric parser UDF#110
Conversation
| targetType: String, | ||
| pattern: Option[String], | ||
| errorCodes: ErrorCodesConfig, | ||
| defaultValue: Option[T] = None): UDFResult[T] = { |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
I like the general idea, but please improve
| vParser, | ||
| vColumnNullable, | ||
| vColumnNameForError, | ||
| vErrorCodes, |
There was a problem hiding this comment.
I agree with simplifying the UDF, but I would just pass the stdConfig.errorCodes
| vErrorCodes, | |
| stdConfig.errorCodes, |
| columnNullable: Boolean, | ||
| columnNameForError: String, | ||
| stdConfig: StandardizationConfig, | ||
| errorCodes: ErrorCodesConfig, |
There was a problem hiding this comment.
This change makes sense, just the ErrorCodesConfig type is enough here.
| val vErrorCodes = BasicErrorCodesConfig( | ||
| stdConfig.errorCodes.castError, | ||
| stdConfig.errorCodes.nullError, | ||
| stdConfig.errorCodes.typeError, | ||
| stdConfig.errorCodes.schemaError | ||
| ) |
There was a problem hiding this comment.
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
|
Thanks for the review. I pushed updates addressing the feedback. What changed:
Verification from the latest run on commit
The remaining |
MatloaItumeleng
left a comment
There was a problem hiding this comment.
Looks good , thank you
Summary
StandardizationConfigin the numeric parser UDF closureErrorCodesConfigthroughUDFResult.fromTry, while preserving the existingStandardizationConfigoverloadDefaultStandardizationConfigRelease Notes
Verification
git diff --checkScala/Spark tests were not run locally because this environment does not have Java, sbt, or Maven installed (
java,sbt, andmvnare not available on PATH).Fixes #57