You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Pajaraja (via GitHub)" <gi...@apache.org> on 2025/05/13 09:40:31 UTC
[PR] [SPARK-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Pajaraja opened a new pull request, #50868:
URL: https://github.com/apache/spark/pull/50868
### What changes were proposed in this pull request?
Removed the early stop error in rCTEs when a custom LIMIT operator is present. Also changed error message to reflect this change.
### Why are the changes needed?
This enables users to return arbitrarily large result sets without using flags.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added a golden file test in cte-recursion.sql that breaks the flag limit but has a LIMIT operator.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086791626
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -221,22 +221,20 @@ case class UnionLoopExec(
unionChildren += prevPlan
- currentNumRows += prevCount
-
- if (limit.isDefined) {
+ if (rowLimit != -1) {
currentLimit -= prevCount.toInt
if (currentLimit <= 0) {
- limitReached = true
+ if (rowLimitChanged) {
+ limitReached = true
Review Comment:
hmmm do we still need this variable?
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086793507
##########
sql/core/src/test/resources/sql-tests/inputs/cte-recursion.sql:
##########
@@ -57,6 +57,27 @@ SELECT * FROM t;
DROP VIEW ZeroAndOne;
+-- unlimited recursion stopped from failing by putting LIMIT
+CREATE TEMPORARY VIEW ZeroAndOne(current, next) AS VALUES
+ (0,0),
+ (0,1),
+ (1,0),
+ (1,1);
+
+SET spark.sql.cteRecursionRowLimit=25;
+
+WITH RECURSIVE t(n) AS (
+ SELECT 1
+ UNION ALL
+ SELECT next FROM t LEFT JOIN ZeroAndOne ON n = current
+ )
+SELECT * FROM t LIMIT 30;
+
+SET spark.sql.cteRecursionRowLimit=1000000;
Review Comment:
```suggestion
UNSET spark.sql.cteRecursionRowLimit;
```
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086834217
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -162,7 +162,9 @@ case class UnionLoopExec(
// the number of rows generated in that step.
// If limit is not passed down, currentLimit is set to be zero and won't be considered in the
Review Comment:
Updated!
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086787864
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -162,7 +162,9 @@ case class UnionLoopExec(
// the number of rows generated in that step.
// If limit is not passed down, currentLimit is set to be zero and won't be considered in the
Review Comment:
let's update the comment as well
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2087020942
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -221,22 +221,20 @@ case class UnionLoopExec(
unionChildren += prevPlan
- currentNumRows += prevCount
-
- if (limit.isDefined) {
+ if (rowLimit != -1) {
currentLimit -= prevCount.toInt
if (currentLimit <= 0) {
- limitReached = true
+ if (rowLimitChanged) {
+ limitReached = true
Review Comment:
oh, yes. In line 243, we have `if (!limitReached) { ...`, can we still hit it?
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086831832
##########
sql/core/src/test/resources/sql-tests/inputs/cte-recursion.sql:
##########
@@ -57,6 +57,27 @@ SELECT * FROM t;
DROP VIEW ZeroAndOne;
+-- unlimited recursion stopped from failing by putting LIMIT
+CREATE TEMPORARY VIEW ZeroAndOne(current, next) AS VALUES
+ (0,0),
+ (0,1),
+ (1,0),
+ (1,1);
+
+SET spark.sql.cteRecursionRowLimit=25;
+
+WITH RECURSIVE t(n) AS (
+ SELECT 1
+ UNION ALL
+ SELECT next FROM t LEFT JOIN ZeroAndOne ON n = current
+ )
+SELECT * FROM t LIMIT 30;
+
+SET spark.sql.cteRecursionRowLimit=1000000;
Review Comment:
changed into RESET
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086788659
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -162,7 +162,9 @@ case class UnionLoopExec(
// the number of rows generated in that step.
// If limit is not passed down, currentLimit is set to be zero and won't be considered in the
// condition of while loop down (limit.isEmpty will be true).
- var currentLimit = limit.getOrElse(-1)
+ var currentLimit = limit.getOrElse(rowLimit)
+
+ val rowLimitChanged = limit.isDefined
Review Comment:
```suggestion
val userSpecifiedLimit = limit.isDefined
```
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2087028227
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -221,22 +221,20 @@ case class UnionLoopExec(
unionChildren += prevPlan
- currentNumRows += prevCount
-
- if (limit.isDefined) {
+ if (rowLimit != -1) {
currentLimit -= prevCount.toInt
if (currentLimit <= 0) {
- limitReached = true
+ if (rowLimitChanged) {
+ limitReached = true
Review Comment:
Yes. It is just a check not to do the next step without any reason. But the currentLimit gets updated in the next iteration of the while loop, so that `if` is used to stop everything immediately.
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086785094
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -4636,7 +4636,7 @@
},
"RECURSION_ROW_LIMIT_EXCEEDED" : {
"message" : [
- "Recursion row limit <rowLimit> reached but query has not exhausted, try increasing 'spark.sql.cteRecursionRowLimit'"
+ "Recursion row limit <rowLimit> reached but query has not exhausted, try setting a custom LIMIT value."
Review Comment:
```suggestion
"Recursion row limit <rowLimit> reached but query has not exhausted, try setting a larger LIMIT value when querying the CTE relation."
```
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086803702
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -221,22 +221,20 @@ case class UnionLoopExec(
unionChildren += prevPlan
- currentNumRows += prevCount
-
- if (limit.isDefined) {
+ if (rowLimit != -1) {
currentLimit -= prevCount.toInt
if (currentLimit <= 0) {
- limitReached = true
+ if (rowLimitChanged) {
+ limitReached = true
Review Comment:
We need it to stop the recursion.
--
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-52099][SQL] Enable overriding the recursion row limit by adding a LIMIT operator [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #50868:
URL: https://github.com/apache/spark/pull/50868#discussion_r2086833045
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/UnionLoopExec.scala:
##########
@@ -162,7 +162,9 @@ case class UnionLoopExec(
// the number of rows generated in that step.
// If limit is not passed down, currentLimit is set to be zero and won't be considered in the
// condition of while loop down (limit.isEmpty will be true).
- var currentLimit = limit.getOrElse(-1)
+ var currentLimit = limit.getOrElse(rowLimit)
+
+ val rowLimitChanged = limit.isDefined
Review Comment:
Changed
--
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