-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-47696][SQL] try_to_timestamp should handle SparkUpgradeException #45822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| ParseToTimestamp( | ||
| // The expression ParseToTimestamp will throw an SparkUpgradeException if the input is invalid | ||
| // even when failOnError is false. We need to catch the exception and return null. | ||
| TryEval(ParseToTimestamp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we simply update ToTimestamp#eval and codegen to catch SparkUpgradeException additionally? I'm a bit worried about increasing the scope and swallow all errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in ToTimestamp, there is supposed to be a SparkUpgradeException even when failOnErroris false.
To avoid swallowing all errors, we can add another flag isTryFunction besides failOnError. It will make the code more complicated though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we need to maintain 3 behaviors:
try_to_timestampnever failsto_timestampansi mode always fail with invalid inputto_timestampnon-ansi mode only fails withSparkUpgradeException
Do I understand it correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why try_to_timestamp need a different behavior with to_timestamp non-ansi mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why try_to_timestamp need a different behavior with to_timestamp non-ansi mode.
The try_* functions are supposed to return null on invalid inputs, instead of compatible with non-ansi mode. cc @srielau on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This upgrade issue seems special and we should always throw it to provide upgrade instructions. How about #45853 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan after #45853, users will face this bug when setting the timeParserPolicy as legacy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can argue this is not a bug as the upgrade exception must be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan ok, let me close this one for now.
What changes were proposed in this pull request?
Currently, try_to_timestamp will throw an exception on legacy timestamp input.
It should return null instead of error.
Why are the changes needed?
Fix a bug in function try_to_timestamp
Does this PR introduce any user-facing change?
Yes, this PR introduces a bug fix: try_to_timestamp will return null instead of throwing an error on legacy timestamp string input.
How was this patch tested?
New UT
Was this patch authored or co-authored using generative AI tooling?
No