-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-41205][SQL] Check that format is foldable in TryToBinary
#38727
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
| -- format must be foldable | ||
| SELECT try_to_binary(col1, col2) from values ('abc', 'utf-8') as data(col1, col2); | ||
| -- non-foldable input string | ||
| SELECT try_to_binary(col1, 'utf-8') from values ('abc') as data(col1); |
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.
Not strictly relevant to this issue, but I noticed that there were no tests for non-foldable input values, so I added one.
| def this(expr: Expression, formatExpression: Expression) = | ||
| this(expr, Some(formatExpression), | ||
| TryEval(ToBinary(expr, Some(formatExpression), nullOnInvalidFormat = true))) | ||
| TryEval(ToBinary(expr, Some(TryToBinary.checkFormat(formatExpression)), |
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.
Could you perform the check in checkInputDataTypes() as we do that in other expression, please.
BTW, exceptions from expressions constructors are wrapped by AnalysisException additionally which is not convenient to users.
| struct<> | ||
| -- !query output | ||
| org.apache.spark.sql.AnalysisException | ||
| The 'format' parameter of function 'to_binary' needs to be a string literal.; line 1 pos 7 |
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.
When you move the exception from constructor, you should see AnalysisException w/ an error class.
|
I will wait on PR #38737. |
|
Tested PR #38737. That PR incidentally seems to fix this issue: Therefore, closing this PR. |
What changes were proposed in this pull request?
Change
TryToBinary's constructor to test whether the format expression is foldable before creating the replacement expression.Why are the changes needed?
try_to_binary, when called with non-foldable format, throws an unhelpful error message:TryToBinarycreates an instance ofToBinaryas a replacement expression. However,ToBinary's default constructor does not check whether the format expression is foldable, so the code that createsToBinary's own replacement expression fails with an assertion failure.ToBinaryis not typically instantiated using the default constructor. The function registry uses the second auxiliary constructor, which does check whether format is foldable. The code that createsToBinary's replacement expression assumes this second auxiliary constructor is always used.TryToBinarycannot useToBinary's second auxiliary constructor because it needs to setnullOnInvalidFormattotrue.After this PR, the above example will throw a more useful error message:
Does this PR introduce any user-facing change?
No.
How was this patch tested?
New tests.