dongjoon-hyun commented on code in PR #59194:
URL: https://github.com/apache/spark/pull/59194#discussion_r4328844944
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1008,8 +1008,10 @@ 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 merged limit so that the LocalLimit can be planned without a
later ConstantFolding
Review Comment:
**`EliminateOffsets` still leaves the same kind of unfolded expression, so
queries with only literal values can still fail planning without
`ConstantFolding`.** Its merge at L3143-3144, `case Offset(oe1, Offset(oe2,
child)) => Offset(Add(oe1, oe2), child)`, relies on a later `ConstantFolding`
run just as the merged limit did, while `EliminateLimits` has folded its own
merges since SPARK-36424 (L3119-3124). With
`spark.sql.optimizer.excludedRules=org.apache.spark.sql.catalyst.optimizer.ConstantFolding`:
```sql
SELECT * FROM (SELECT id FROM range(10) OFFSET 1) LIMIT 3 OFFSET 2
```
This rule now correctly produces `GlobalLimit(3, Offset(2, Offset(1,
LocalLimit(6, Range))))`. But `EliminateOffsets` then merges the two offsets
into `Offset(2 + 1, ...)`, and every `Offset` case in the planner requires an
`IntegerLiteral`, so planning fails with `No plan for Offset (2 + 1)`.
`spark.range(10).offset(1).offset(2).collect()` fails the same way. This also
fails on master and is outside the "merged limit" wording of the description,
but it is the same failure as SPARK-57125, and the PR title says that the plan
stays plannable without `ConstantFolding`. I traced this by reading the code
and did not run it. Could you fold that merge the same way, with the same `Int`
check as on L1014, and update EliminateOffsetsSuite.scala:96, which expects
`child.offset(Add(Literal(3), Literal(2)))`? Otherwise, could you mention the
remaining gap in the description and file a follow-up JIRA?
--
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]
