-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-51119][SQL][FOLLOW-UP] Fix missing fallback case for parsing corrupt exists_default value #50685
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
…f exists_default_value
|
|
||
| // Parse the expression. | ||
| val expr = Literal.fromSQL(defaultSQL) match { | ||
| // EXISTS_DEFAULT will have a cast from analyze() due to coerceDefaultValue |
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.
Don't we expect the cast to be evaluated before the exists default is persisted?
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.
Yes in the happy path.
There are unfortunately bugs from before though, where column exists_defaults were persisted as unresolved, hence the fallback mechanism.
|
thanks, merging to master/4.0! |
…orrupt exists_default value ### What changes were proposed in this pull request? Add another fallback for broken (non-resolved) exists_default values for SPARK-51119 original fix. ### Why are the changes needed? #49962 added a fallback in case there were already broken (ie, non-resolved) persisted default values in catalogs. A broken one is something like 'current_database, current_user, current_timestamp' , these are non-deterministic and will bring wrong results in EXISTS_DEFAULT, where user expects the value resolved when they set the default. However, this fallback missed one case when the current_xxx is in a cast. This fixes it. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add to existing unit test in StructTypeSuite ### Was this patch authored or co-authored using generative AI tooling? No Closes #50685 from szehon-ho/SPARK-51119-follow-3. Authored-by: Szehon Ho <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 516859f) Signed-off-by: Wenchen Fan <[email protected]>
…orrupt exists_default value ### What changes were proposed in this pull request? Add another fallback for broken (non-resolved) exists_default values for SPARK-51119 original fix. ### Why are the changes needed? apache#49962 added a fallback in case there were already broken (ie, non-resolved) persisted default values in catalogs. A broken one is something like 'current_database, current_user, current_timestamp' , these are non-deterministic and will bring wrong results in EXISTS_DEFAULT, where user expects the value resolved when they set the default. However, this fallback missed one case when the current_xxx is in a cast. This fixes it. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add to existing unit test in StructTypeSuite ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#50685 from szehon-ho/SPARK-51119-follow-3. Authored-by: Szehon Ho <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 5cb83d4) Signed-off-by: Wenchen Fan <[email protected]>
What changes were proposed in this pull request?
Add another fallback for broken (non-resolved) exists_default values for SPARK-51119 original fix.
Why are the changes needed?
#49962 added a fallback in case there were already broken (ie, non-resolved) persisted default values in catalogs. A broken one is something like 'current_database, current_user, current_timestamp' , these are non-deterministic and will bring wrong results in EXISTS_DEFAULT, where user expects the value resolved when they set the default.
However, this fallback missed one case when the current_xxx is in a cast. This fixes it.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add to existing unit test in StructTypeSuite
Was this patch authored or co-authored using generative AI tooling?
No