Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
HyukjinKwon closed pull request #45931: [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec URL: https://github.com/apache/spark/pull/45931 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
HyukjinKwon commented on PR #45931: URL: https://github.com/apache/spark/pull/45931#issuecomment-2063164089 Merged to master. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on PR #45931: URL: https://github.com/apache/spark/pull/45931#issuecomment-2060168379 > Could you add one test case like `EXPLAIN ... LIMIT ... OFFSET ... ORDER BY ...` at https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/explain.sql ? @beliefer Thank you for you review. I just changed the prompt message. This will not affect the UT test. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1567621607
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset == 0) { "" } else { s" offset=$offset," }
Review Comment:
> It seems the GA failure is unrelated.
@beliefer OK. Thank you.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on PR #45931: URL: https://github.com/apache/spark/pull/45931#issuecomment-2059429386 > Could you add one test case like `EXPLAIN ... LIMIT ... OFFSET ... ORDER BY ...` at https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/explain.sql ? @beliefer OK. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
beliefer commented on PR #45931: URL: https://github.com/apache/spark/pull/45931#issuecomment-2059030991 Could you add one test case like `EXPLAIN ... LIMIT ... OFFSET ... ORDER BY ...` int `explain.sql` ? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
beliefer commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1567314958
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset == 0) { "" } else { s" offset=$offset," }
Review Comment:
It seems the GA failure is unrelated.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
beliefer commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1567304723
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset == 0) { "" } else { s" offset=$offset," }
Review Comment:
It seems the GA failure is unrelated.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1567204208
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset == 0) { "" } else { s" offset=$offset," }
Review Comment:
> How about `val offsetStr = if (offset > 0) s" offset=$offset," else ""` ?
@beliefer Thank you for your review. I made the modifications according to
what you said, but there are some UTs that cannot run through. Can you help me
take a look.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
beliefer commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1567035887
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset == 0) { "" } else { s" offset=$offset," }
Review Comment:
How about `val offsetStr = if (offset > 0) s" offset=$offset," else ""` ?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1566636044
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,9 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset > 0) { s" offset=$offset, " } else { "" }
Review Comment:
> I assume we only need to skip when offset = 0?
@amaliujia Thank you for your review. I made some modifications.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
amaliujia commented on code in PR #45931:
URL: https://github.com/apache/spark/pull/45931#discussion_r1566291325
##
sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala:
##
@@ -358,7 +358,9 @@ case class TakeOrderedAndProjectExec(
val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields)
val outputString = truncatedString(output, "[", ",", "]", maxFields)
-s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString,
output=$outputString)"
+val offsetStr = if (offset > 0) { s" offset=$offset, " } else { "" }
Review Comment:
I assume we only need to skip when offset = 0?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on code in PR #45931: URL: https://github.com/apache/spark/pull/45931#discussion_r1566011613 ## sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala: ## @@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec( val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields) val outputString = truncatedString(output, "[", ",", "]", maxFields) -s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString, output=$outputString)" +s"TakeOrderedAndProject(limit=$limit, offset=$offset, " + Review Comment: > I think we should probably handle the case when `offset` is not set? It would show it as `0`. @HyukjinKwon Thank you for your review. I handled this case. Please review again. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
guixiaowen commented on code in PR #45931: URL: https://github.com/apache/spark/pull/45931#discussion_r1558776883 ## sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala: ## @@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec( val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields) val outputString = truncatedString(output, "[", ",", "]", maxFields) -s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString, output=$outputString)" +s"TakeOrderedAndProject(limit=$limit, offset=$offset, " + Review Comment: > I think we should probably handle the case when `offset` is not set? It would show it as `0`. Thank you for your review. I will handle the case when `offset` is not set. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47767][SQL] Show offset value in TakeOrderedAndProjectExec [spark]
HyukjinKwon commented on code in PR #45931: URL: https://github.com/apache/spark/pull/45931#discussion_r1556759763 ## sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala: ## @@ -358,7 +358,8 @@ case class TakeOrderedAndProjectExec( val orderByString = truncatedString(sortOrder, "[", ",", "]", maxFields) val outputString = truncatedString(output, "[", ",", "]", maxFields) -s"TakeOrderedAndProject(limit=$limit, orderBy=$orderByString, output=$outputString)" +s"TakeOrderedAndProject(limit=$limit, offset=$offset, " + Review Comment: I think we should probably handle the case when `offset` is not set? It would show it as `0`. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
