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: [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]
