asugranyes commented on code in PR #56180:
URL: https://github.com/apache/spark/pull/56180#discussion_r3329422750
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -973,8 +973,19 @@ object LimitPushDown extends Rule[LogicalPlan] {
val newAgg = EliminateSorts(a.copy(child = LocalLimit(le,
a.child))).asInstanceOf[Aggregate]
Limit(le, p.copy(child = Project(newAgg.aggregateExpressions,
newAgg.child)))
// Merge offset value and limit value into LocalLimit and pushes down
LocalLimit through Offset.
+ // Fold the sum eagerly when both operands are integer literals so this
rule produces a
+ // planable logical plan in a single application. Otherwise physical
planning
+ // (BasicOperators in SparkStrategies) would fail with `AssertionError: No
plan for
+ // LocalLimit (a + b)` when ConstantFolding is excluded from the optimizer
pipeline, since
+ // BasicOperators only matches LocalLimit(IntegerLiteral, _). In practice
both operands
+ // come from `LIMIT N OFFSET M` clauses (already folded to literals) so
this single-case
+ // fold covers all realistic inputs.
case LocalLimit(le, Offset(oe, grandChild)) =>
- Offset(oe, LocalLimit(Add(le, oe), grandChild))
+ val mergedLimit = (le, oe) match {
+ case (IntegerLiteral(l), IntegerLiteral(o)) => Literal(l + o,
IntegerType)
+ case _ => Add(le, oe)
+ }
+ Offset(oe, LocalLimit(mergedLimit, grandChild))
Review Comment:
Could we apply `constantFolding` directly to the synthesized `Add(le, oe)`
here?
My understanding is that this would reuse the existing folding path,
including ANSI overflow behavior, without reimplementing the checks locally. It
would also avoid special-casing only `IntegerLiteral` + `IntegerLiteral`.
Since `constantFolding` recursively folds child expressions before trying to
fold the parent expression, I think this could also cover the broader
foldable-expression cases mentioned in the PR description, e.g. `LIMIT 3 - 1
OFFSET 5 - 3.`
Something like:
```suggestion
Offset(oe, LocalLimit(constantFolding(Add(le, oe)), grandChild))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
