[SPARK-50377][SQL] Allow to evaluate foldable RuntimeReplaceable #48912
Conversation
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
Outdated
Show resolved
Hide resolved
…essions/arithmetic.scala
| final override def eval(input: InternalRow = null): Any = { | ||
| // For convenience, we allow to evaluate `RuntimeReplaceable` expressions, in case we need to | ||
| // get a constant from foldable expression before the query execution starts. | ||
| assert(input == null) |
There was a problem hiding this comment.
Sorry, could you clarify why do you require null instead of just bypass input to replacement.eval()
There was a problem hiding this comment.
If the premise here must be that input == null, then would it be better to throw a meaningful QueryExecutionError instead of an AssertionError when input is not null?
There was a problem hiding this comment.
Ideally people should never eagerly evaluate a RuntimeReplaceable, the only case I see is evaluating foldable expressions. I think it's better to only allow it, to make sure that there is no RuntimeReplaceable leaked to the runtime execution.
There was a problem hiding this comment.
I use assert as this means bug, not a user error.
There was a problem hiding this comment.
I use
assertas this means bug, not a user error.
Got it，thanks
|
thanks for the review, merging to master! |
What changes were proposed in this pull request?
This is to fix a regression caused by #47143 . The problem is, in some places, we want to get a constant from a foldable expression before the query execution starts. #47143 brings two problems:
UnaryPositiveis no longer aUnaryExpression, which means it's not foldable anymore even if its child is foldable.UnaryPositiveis no longer evaluable.Lagis such a place. It may evaluate theinputOffsetparameter eagerly.lag(..., +1)no longer works after #47143 .Instead of fixing
Lag, this PR makes two changes and hopefully we can avoid all similar problems:UnaryPositiveextendUnaryExpressionagain. We need follow-up PRs to check otherRuntimeReplaceableexpressions and see if they should extendUnaryExpressionorBinaryExpression, etc.RuntimeReplaceable#evalso that we can evaluate foldingRuntimeReplaceableeagerly when needed.Why are the changes needed?
Fix the regression on
lagand avoid similar issues in the future.Does this PR introduce any user-facing change?
No, the regression is not released yet.
How was this patch tested?
new test
Was this patch authored or co-authored using generative AI tooling?
no