[SPARK-58866][SQL] Add expression-level entry points to optimizer rules - #58119
[SPARK-58866][SQL] Add expression-level entry points to optimizer rules#58119matvei-zamiatin-db wants to merge 4 commits into
Conversation
…es for the single-pass analyzer
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
Apart from having no tests, the PR adds public API with no callers, right? Can we also address this ,e.g. TimeTravelSpec already needs this and works around its absence by building a throwaway plan and digging the expression back out?
1aaa442 to
1fbb381
Compare
1fbb381 to
7b4652f
Compare
| } | ||
| // `current_time()` and a TIME -> TIMESTAMP cast are not leaves and their only leaf is a | ||
| // literal, so the tree walk accepts them and only these patterns reject them. | ||
| !e.containsAnyPattern(CURRENT_LIKE, CAST_TO_TIMESTAMP) && isLiteralTree(e) |
There was a problem hiding this comment.
CAST_TO_TIMESTAMP rejects every timestamp-target cast, although only TIME -> TIMESTAMP is unsafe. A repeated CAST('1970-01-01' AS TIMESTAMP) incorrectly throws.
| // Arbitrary Java methods and UDFs can be foldable without being pure. | ||
| case _: NonSQLExpression | _: UserDefinedExpression => false | ||
| case _ => e.deterministic && e.children.forall(isLiteralTree) | ||
| } |
There was a problem hiding this comment.
The documented preprocessing still rejects TIME -> TIMESTAMP. ComputeCurrentTime produces a pure StaticInvoke, which line 160 rejects as NonSQLExpression. Constant folding would mask both issues, but it is neither documented nor invoked before this helper.
| * references an enclosing one is decided after it, and visits a definition's own nested `With` | ||
| * before the definition itself. Ids are globally unique, so one flat set is enough. | ||
| */ | ||
| private def safeToDuplicateIds(expression: Expression): Set[CommonExpressionId] = { |
There was a problem hiding this comment.
safeIds assumes globally unique IDs, but sibling canonicalized With expressions both start numbering at 1. A safe sibling can therefore authorize duplication of an unsafe definition. Reject canonicalized IDs as the plan-level path does, or scope safety per With.
| } | ||
|
|
||
| private def replace(e: Expression): Expression = e match { | ||
| def replace(e: Expression): Expression = e match { |
There was a problem hiding this comment.
Can this be private[sql]? We need to be careful with exposing
2 participants
What changes were proposed in this pull request?
This is a behavior-preserving refactor that exposes expression-level entry points on several
FinishAnalysisrules and onRewriteWithExpressionrule so they can rewrite a singleExpressiontree, not just a wholeLogicalPlan. Each rule's rewrite logic is factored into a reusable partial function, the existing plan-level apply delegates to it, and a newapplyForExpression(or widened visibility) is added.TimeTravelSpec.resolveTimestampExpressionis converted to the new entry points, which lets itdrop the fake
Project(Alias(ts), OneRowRelation())wrapper and the twoasInstanceOfcasts itneeded to unwrap the result.
No plan-level rule changes its output, the extracted partial functions are identical to the original inline bodies.
Why are the changes needed?
The single-pass analyzer needs to apply these
FinishAnalysisrewrites to individual, already-resolved expression trees rather than by running a full plan-wide rule pass. Today the logic is reachable only through each rule's plan-level apply, so it can't be invoked on a singleExpression. Extracting shared entry points makes the rewrites reusable.Does this PR introduce any user-facing change?
No. This is an internal refactor. All plan-level rules produce the same output as before.
How was this patch tested?
New unit tests, 23 cases across 6 suites in
sql/catalyst:ReplaceExpressionsSuite(new): 4 cases coveringreplaceonRuntimeReplaceableexpressions, nested
RuntimeReplaceable, recursion through non-replaceable parents, and atree with nothing to replace.
ReplaceCurrentLikeSuite(new): 3 cases coveringapplyForExpressiononCurrentCatalog/CurrentDatabase, current-like nested in a larger expression, and a no-op tree.ComputeCurrentTimeSuite: 3 cases coveringapplyForExpressionon current date/time leaves,the shared-
instantoverload, and the documented "does not descend into subquery plans"behavior.
SpecialDatetimeValuesSuite: 3 cases covering foldable special-datetime casts toDATEandTIMESTAMP, and non-special casts left unchanged.RewriteWithExpressionSuite: 5applyForExpressioncases (inlining, non-cheap commonexpressions, nested
With, a no-op tree, and deferring a ref to an outer common expression)plus 1 plan-level case,
"WITH in a conditional branch referencing an outer common expression", which covers the behavior change described above and fails without the guard.ConstantFoldingSuite: 4 cases coveringConstantFolding.constantFoldingon fully foldable,nested-foldable, partially-foldable, and literal trees.
Existing coverage for the refactored rules (
ComputeCurrentTimeSuite,SpecialDatetimeValuesSuite,RewriteWithExpressionSuite,ConstantFoldingSuite, and thesqlmodule suites) passes unchanged, which is what guards the behavior-preserving part of therefactor.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8