You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "rdtr (via GitHub)" <gi...@apache.org> on 2026/10/01 23:22:42 UTC
[PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
rdtr opened a new pull request, #59194:
URL: https://github.com/apache/spark/pull/59194
This replaces https://github.com/apache/spark/pull/56180, which was closed as stale. It addresses the review comments there: the merged limit is now folded with `ConstantFolding.constantFolding` as @asugranyes suggested, and the overflow case raised by @shrirangmhalgi is already rejected during analysis with `SUM_OF_LIMIT_AND_OFFSET_EXCEEDS_MAX_INT`.
### What changes were proposed in this pull request?
`LimitPushDown` rewrites `LocalLimit(le, Offset(oe, child))` to `Offset(oe, LocalLimit(Add(le, oe), child))` and relies on a later `ConstantFolding` run to fold the `Add`. This
PR folds the merged limit inside the rule with `ConstantFolding.constantFolding`:
case LocalLimit(le, Offset(oe, grandChild)) =>
Offset(oe, LocalLimit(ConstantFolding.constantFolding(Add(le, oe)), grandChild))
This reuses the existing folding path, including its ANSI overflow behavior, and also folds foldable expressions that are not integer literals.
### Why are the changes needed?
`BasicOperators` only plans `LocalLimit(IntegerLiteral, _)`. When `ConstantFolding` is excluded through `spark.sql.optimizer.excludedRules`, the unfolded `Add` reaches physical
planning and fails with:
java.lang.AssertionError: assertion failed: No plan for LocalLimit (1 + 2)
### Does this PR introduce _any_ user-facing change?
No. With the default configuration, `ConstantFolding` folds the `Add` anyway. Only queries that failed when `ConstantFolding` was excluded now succeed.
### How was this patch tested?
Added a test in `LimitPushdownSuite` that runs only `LimitPushDown` and checks that the merged limit is folded, for integer literals and for a foldable expression (`1 + 1`).
The test fails without the change in `Optimizer.scala`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "rdtr (via GitHub)" <gi...@apache.org>.
rdtr commented on PR #59194:
URL: https://github.com/apache/spark/pull/59194#issuecomment-5957831501
Thanks @asugranyes.
@HyukjinKwon Please let me know if you have any other comments.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #59194:
URL: https://github.com/apache/spark/pull/59194#issuecomment-5975983887
Thanks for the ping. The earlier inline point is addressed: narrowing the comments and the test to the merged `LocalLimit` is the right scoping, and folding it with `ConstantFolding.constantFolding(Add(le, oe))` is correct since `le`/`oe` are analysis-guaranteed foldable integers and the sum is overflow-checked at analysis. One small follow-up on this change: the PR description still mentions a `1 + 1` test case that was removed and says "Only queries that failed when ConstantFolding was excluded now succeed"; since a user-written non-literal `LIMIT`/`OFFSET` still relies on `ConstantFolding`, consider scoping that wording to the merged limit. See the review comment for the full assessment.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:5957831501","thread_id":"conversation:5957831501","verdict_sha256":"c76b5d035771d845a6fa2069e28ce94f788910e6872bcccfe7eb345801bf93ce"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "rdtr (via GitHub)" <gi...@apache.org>.
rdtr commented on PR #59194:
URL: https://github.com/apache/spark/pull/59194#issuecomment-6046725961
@HyukjinKwon Thanks, good point. I updated the PR description: the `1 + 1` test case is gone, and the user-facing change now says that only the merged limit is folded, so a non-literal `LIMIT` or `OFFSET` written by the user still needs `ConstantFolding`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "rdtr (via GitHub) via reviews" <re...@spark.apache.org>.
rdtr commented on PR #59194:
URL: https://github.com/apache/spark/pull/59194#issuecomment-6698313636
Thanks @dongjoon-hyun for the detailed review. I addressed all seven comments in https://github.com/apache/spark/pull/59194/commits/37b67159179da29179978bcf2249ef585aca4260.
1. Overflow: `LimitPushDown` now folds the sum only when both values are integer literals and the sum fits in an `Int`, through a new helper `LimitAndOffsetValues.add`. Otherwise it keeps the unevaluated `Add` as before. I ran your `UNION ALL` example on master with ANSI off and `ConstantFolding` excluded, and it already returned 3 rows there instead of failing with `No plan`. `EliminateLimits` merges the two `LocalLimit`s in the `Union` branch with `Least(...).eval()`, which evaluates the `Add`, and the plan has `LocalLimit -2147483648`. So the wrong result is pre-existing in that configuration too. I'll file a follow-up JIRA for the overflow and open a PR on top of this one. I also corrected the overflow sentence in the description.
2. Early evaluation: with the change above, a sum that overflows is not evaluated in `LimitPushDown`. Your `LIMIT 0` example fails with `ARITHMETIC_OVERFLOW` on the previous commit and returns no rows again now, like master.
3. `EliminateOffsets`: the `Offset` merge uses the same helper. `EliminateOffsetsSuite` "Combines Offset operators" now expects `Offset(5)`, and a new test checks that an overflowing sum stays an `Add`.
4. End-to-end test: added "SPARK-57125: LIMIT and OFFSET succeed when ConstantFolding is in excludedRules" next to the SPARK-59044 test. It runs your two queries and `SELECT * FROM (SELECT id FROM range(0, 10, 1, 1) OFFSET 1) LIMIT 3 OFFSET 2` for item 3. I used a single partition so that the rows returned without `ORDER BY` are deterministic. On master it fails with `No plan for LocalLimit (1 + 2)`, and with only the `EliminateOffsets` change reverted it fails with `No plan for Offset (2 + 1)`.
5. Description: the user-facing change section now says "Yes", with the before and after behavior, and notes that optimized plans can differ in the default configuration. I also changed the title, since the PR now covers `EliminateOffsets` too.
6. Backport: the change no longer calls `ConstantFolding.constantFolding`. The `Optimizer.scala` change applies cleanly to branch-4.0 and branch-4.1. On those branches only the `SQLQuerySuite` test conflicts, because the SPARK-59044 test next to it is not there.
7. The test now calls `LimitPushDown` directly, and "How was this patch tested?" mentions the `PushProjectionThroughLimitAndOffsetSuite` expectations.
Could you take another look, and let me know if I am missing anything.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59194:
URL: https://github.com/apache/spark/pull/59194#issuecomment-6016682458
LGTM, thank you @rdtr and @HyukjinKwon @asugranyes! Let's loop in @viirya too
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "rdtr (via GitHub)" <gi...@apache.org>.
rdtr commented on code in PR #59194:
URL: https://github.com/apache/spark/pull/59194#discussion_r4164314051
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/LimitPushdownSuite.scala:
##########
@@ -335,6 +335,20 @@ class LimitPushdownSuite extends PlanTest {
GlobalLimit(1, Offset(2, LocalLimit(3, testRelation))).analyze)
}
+ test("SPARK-57125: Push down limit through Offset folds the merged limit") {
+ // LimitPushDown must produce a plannable plan without a later ConstantFolding run.
+ object OptimizeOnlyLimitPushDown extends RuleExecutor[LogicalPlan] {
+ val batches = Batch("Limit pushdown", Once, LimitPushDown) :: Nil
+ }
+ comparePlans(
+ OptimizeOnlyLimitPushDown.execute(testRelation.offset(2).limit(1).analyze),
+ GlobalLimit(1, Offset(2, LocalLimit(3, testRelation))).analyze)
+ comparePlans(
+ OptimizeOnlyLimitPushDown.execute(
+ testRelation.offset(Literal(1) + Literal(1)).limit(1).analyze),
+ GlobalLimit(1, Offset(Literal(1) + Literal(1), LocalLimit(3, testRelation))).analyze)
Review Comment:
Thanks, good catch. I went with narrowing the scope: the comments now say only that the merged `LocalLimit` is folded so it can be planned without a later `ConstantFolding` run, and I removed the `1 + 1` offset case, since a non-literal `OFFSET` or `LIMIT` written by the user still relies on `ConstantFolding`, like before.
I also updated `PushProjectionThroughLimitAndOffsetSuite`, which expected the unfolded `LocalLimit (15 + 5)`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59194:
URL: https://github.com/apache/spark/pull/59194#discussion_r4161604743
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/LimitPushdownSuite.scala:
##########
@@ -335,6 +335,20 @@ class LimitPushdownSuite extends PlanTest {
GlobalLimit(1, Offset(2, LocalLimit(3, testRelation))).analyze)
}
+ test("SPARK-57125: Push down limit through Offset folds the merged limit") {
+ // LimitPushDown must produce a plannable plan without a later ConstantFolding run.
+ object OptimizeOnlyLimitPushDown extends RuleExecutor[LogicalPlan] {
+ val batches = Batch("Limit pushdown", Once, LimitPushDown) :: Nil
+ }
+ comparePlans(
+ OptimizeOnlyLimitPushDown.execute(testRelation.offset(2).limit(1).analyze),
+ GlobalLimit(1, Offset(2, LocalLimit(3, testRelation))).analyze)
+ comparePlans(
+ OptimizeOnlyLimitPushDown.execute(
+ testRelation.offset(Literal(1) + Literal(1)).limit(1).analyze),
+ GlobalLimit(1, Offset(Literal(1) + Literal(1), LocalLimit(3, testRelation))).analyze)
Review Comment:
**Nit (P3):** This expected plan keeps `Offset(1 + 1)`, but every `Offset` planning case in `SparkStrategies` (`BasicOperators` and `SpecialLimits`, plus the `OffsetAndLimit`/`LimitAndOffset` extractors) only matches an `IntegerLiteral` offset. So with `ConstantFolding` excluded, this plan still fails physical planning (`No plan for Offset (1 + 1)`), even though the test comment above says `LimitPushDown` must produce a plannable plan, and the new Optimizer comment says "this rule alone produces a plannable plan".
The rule change itself looks right for the merged `LocalLimit`. I'd suggest narrowing both comments to say that the merged `LocalLimit` limit is folded so that `LocalLimit` can be planned without a later `ConstantFolding` run, and noting that this case only checks the merged limit. If you'd rather make the claim hold for this case, folding `oe` as well (e.g. `Offset(ConstantFolding.constantFolding(oe), ...)`) would also work, but a foldable non-literal `LIMIT` would still need `ConstantFolding`.
**Verification:**
- **Inspection:** The rewritten Optimizer and test comments describe only folding of the merged LocalLimit expression and no longer assert end-to-end plannability for a non-literal offset.
- **Regression:** The SPARK-57125 limit-pushdown test still passes and still fails if the merged LocalLimit is left as an unfolded Add.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59194:
URL: https://github.com/apache/spark/pull/59194#discussion_r4328844923
##########
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
+ // run: BasicOperators only plans LocalLimit(IntegerLiteral, _).
case LocalLimit(le, Offset(oe, grandChild)) =>
- Offset(oe, LocalLimit(Add(le, oe), grandChild))
+ Offset(oe, LocalLimit(ConstantFolding.constantFolding(Add(le, oe)), grandChild))
Review Comment:
**The analysis check does not cover every `LocalLimit` over `Offset` that reaches this case, so this sum can still overflow.** The PR description says the overflow case "is already rejected during analysis with `SUM_OF_LIMIT_AND_OFFSET_EXCEEDS_MAX_INT`", but CheckAnalysis.scala:921-935 only checks a `LocalLimit` whose direct child is an `Offset` in the analyzed plan. The optimizer also creates such pairs, which the analyzer never sees: the `Union` case at L976-977 and `maybePushLocalLimit`'s `case (_, None)` at L942-944 push the limit onto an `Offset` branch, the semi/anti join case at L1016-1017 pushes `Literal(1)`, and `PushProjectionThroughLimitAndOffset` or `RemoveNoopOperators` can move or remove a `Project` between them. For example, with a table `t` that has 3 rows:
```sql
(SELECT * FROM t OFFSET 1) UNION ALL (SELECT * FROM t) LIMIT 2147483647
```
- With ANSI on (the default), this valid query fails with `[ARITHMETIC_OVERFLOW]`, although the user wrote no addition.
- With ANSI off, the sum wraps to -2147483648, and `LocalLimitExec` returns no rows for that branch (limit.scala:200 and :233), so the query silently returns 3 rows instead of 5.
- With ANSI off, `SELECT * FROM (SELECT * FROM t OFFSET 10) LIMIT 2147483647` still matches `OffsetAndLimit`, because `globalLimit + offset == localLimit` (basicLogicalOperators.scala:2041) wraps the same way, and then fails the `CollectLimitExec` assertion (limit.scala:49) as an INTERNAL_ERROR.
This is not a regression in the default configuration, since `ConstantFolding` produced the same values before. But with `ConstantFolding` excluded and ANSI off, the `UNION ALL` query used to fail with `No plan for LocalLimit (2147483647 + 1)`, and with this PR it returns wrong rows silently. I traced this by reading the code and did not run it. I know asugranyes pointed out on #56180 that an `Add` left behind cannot be planned without `ConstantFolding`, but that only affects sums that overflow, which cannot give a correct plan here anyway, and keeping the `Add` makes them fail loudly instead of returning wrong rows. Could you fold only when the sum fits in an `Int`, e.g.
```scala
val merged = Add(le, oe)
val fits = le.eval().asInstanceOf[Int].toLong + oe.eval().asInstanceOf[Int] <= Int.MaxValue
Offset(oe, LocalLimit(if (fits) ConstantFolding.constantFolding(merged) else merged, grandChild))
```
and also correct the overflow sentence in the PR description, with a follow-up JIRA for the overflow itself?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/LimitPushdownSuite.scala:
##########
@@ -335,6 +335,16 @@ class LimitPushdownSuite extends PlanTest {
GlobalLimit(1, Offset(2, LocalLimit(3, testRelation))).analyze)
}
+ test("SPARK-57125: Push down limit through Offset folds the merged limit") {
Review Comment:
Minor: This test checks the output of `LimitPushDown` alone, but nothing checks the user-visible part of the fix: that a `LIMIT`/`OFFSET` query is planned and returns the right rows when `ConstantFolding` is excluded. SPARK-59044 fixed the same kind of failure for `EliminateOffsets` and added an end-to-end test for both planning paths, "SPARK-59044: OFFSET 0 succeeds when EliminateOffsets is in excludedRules" (SQLQuerySuite.scala:5359 on master). For example:
```scala
withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> ConstantFolding.ruleName) {
checkAnswer(sql("SELECT id FROM range(10) LIMIT 1 OFFSET 2"), Row(2))
checkAnswer(sql("SELECT count(*) FROM (SELECT id FROM range(10) LIMIT 2 OFFSET 2)"), Row(2))
}
```
By my reading, both queries fail on master with `No plan for LocalLimit (...)` and pass with this PR. The source needs more rows than the limit plus the offset; otherwise `EliminateLimits` removes the limit before this rule runs. Could you add such a test next to the SPARK-59044 one, after a rebase?
##########
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
+ // run: BasicOperators only plans LocalLimit(IntegerLiteral, _).
case LocalLimit(le, Offset(oe, grandChild)) =>
Review Comment:
**Evaluating the sum here, before `EliminateOffsets` and `EliminateLimits` run, turns a query that returns an empty result on master into an error.** In the operator optimization batch, this rule (L112) runs before `EliminateOffsets` (L122), `EliminateLimits` (L123) and `ConstantFolding` (L139), so on master the unfolded `Add` could be pruned with its subtree before anything evaluated it. Now it is evaluated right here. For example, with ANSI on (the default) and a table `t`:
```sql
SELECT * FROM (SELECT * FROM (SELECT * FROM t OFFSET 10) LIMIT 2147483647) LIMIT 0
```
The analysis check does not apply, because a `Project` sits between the inner `LIMIT` and `OFFSET`. On master, this rule leaves `LocalLimit(2147483647 + 10, t)` unevaluated, and `EliminateLimits` then replaces the whole plan with an empty `LocalRelation` (`case GlobalLimit(IntegerLiteral(0), child)` at L3116), so the query returns no rows. With this PR, L1014 evaluates `2147483647 + 10` first, and the query fails with `[ARITHMETIC_OVERFLOW]`. `spark.range(100).offset(10).select("id").limit(Int.MaxValue).limit(0).collect()` behaves the same, and so does an outer `OFFSET 2147483647` instead of `LIMIT 0`. This is an edge case, but it contradicts "No user-facing change". I traced this by reading the code and did not run it. Could you fold only when the sum fits in an `Int`, as suggested on L1014, so that this case behaves as on master?
##########
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
+ // run: BasicOperators only plans LocalLimit(IntegerLiteral, _).
Review Comment:
Minor: About the PR description. The "Does this PR introduce _any_ user-facing change?" section answers "No", but the template counts bug fixes and other behavior changes as user-facing. With `ConstantFolding` excluded, `SELECT id FROM range(10) LIMIT 1 OFFSET 2` used to fail with `No plan for LocalLimit (1 + 2)` and now returns `2`, and SPARK-59044 answered "Yes" for the same kind of change. Also, "`ConstantFolding` folds the `Add` anyway" does not mean that the default-configuration plans stay the same: the code that runs between this rule and `ConstantFolding` in the same iteration (e.g. `maybePushLocalLimit` and `LimitPushDownThroughWindow`, which match `IntegerLiteral` limits) now sees the literal one iteration earlier, so some optimized plans can differ, although the results do not. Could you answer "Yes" there, with the before and after behavior?
##########
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
+ // run: BasicOperators only plans LocalLimit(IntegerLiteral, _).
case LocalLimit(le, Offset(oe, grandChild)) =>
- Offset(oe, LocalLimit(Add(le, oe), grandChild))
+ Offset(oe, LocalLimit(ConstantFolding.constantFolding(Add(le, oe)), grandChild))
Review Comment:
Minor: About a backport. `ConstantFolding.constantFolding` became `private[sql]` in SPARK-52875, so this line compiles on master, branch-4.x, branch-4.2 and branch-4.1. On branch-4.0 it is still `private def constantFolding(` (expressions.scala:74), so a cherry-pick there would not compile, and branch-4.0 has no `PushProjectionThroughLimitAndOffsetSuite`. If this fix should also go to branch-4.0, could you note in the JIRA that the backport needs a separate patch (e.g. evaluating the sum directly, as `EliminateLimits` does at L3119-3124)?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/LimitPushdownSuite.scala:
##########
@@ -335,6 +335,16 @@ class LimitPushdownSuite extends PlanTest {
GlobalLimit(1, Offset(2, LocalLimit(3, testRelation))).analyze)
}
+ test("SPARK-57125: Push down limit through Offset folds the merged limit") {
+ // The merged LocalLimit must be folded without a later ConstantFolding run.
+ object OptimizeOnlyLimitPushDown extends RuleExecutor[LogicalPlan] {
Review Comment:
Nit: This local executor is left over from the first commit, where it served two cases. With one case left, it wraps a single rule, and `comparePlans(LimitPushDown(testRelation.offset(2).limit(1).analyze), ...)` does the same, as other optimizer suites do (e.g. InferFiltersFromConstraintsSuite.scala:398). The input and the expected plan are also the same as in the test above, so only the missing `ConstantFolding` differs. Separately, "How was this patch tested?" could also mention the updated `PushProjectionThroughLimitAndOffsetSuite` expectations, which fail without the fix too. Could you call the rule directly here, unless you prefer the `RuleExecutor` style of this suite?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57125][SQL] LimitPushDown should fold the merged limit so the plan stays plannable without ConstantFolding [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org