You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "kecookier (via GitHub)" <gi...@apache.org> on 2024/09/09 09:47:51 UTC
[PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
kecookier opened a new pull request, #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168
## What changes were proposed in this pull request?
If the offset is negative, convert lag() to lead().
## How was this patch tested?
Exist UT.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "kecookier (via GitHub)" <gi...@apache.org>.
kecookier commented on code in PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#discussion_r1751145852
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/WindowFunctionsBuilder.scala:
##########
@@ -31,7 +31,7 @@ object WindowFunctionsBuilder {
val substraitFunc = windowFunc match {
// Handle lag with negative inputOffset, e.g., converts lag(c1, -1) to lead(c1, 1).
// Spark uses `-inputOffset` as `offset` for Lag function.
- case lag: Lag if lag.offset.eval(EmptyRow).asInstanceOf[Int] > 0 =>
Review Comment:
@PHILO-HE This PR addresses the lag result mismatch issue in my current version of Spark. Thank you for your input. I have reviewed the Spark code, and now I understand the differences.
In Spark 3.0, the LAG function calculates the bound using both the offset and the direction. However, in versions post Spark 3.1, the function does not consider the direction. Instead, it uses a single literal expression that includes the offset of the current row to calculate the bound. For example, in lag(), the offset will be wrapped with UnaryMinus(offset).
lag in spark 3.0 https://github.com/apache/spark/blob/branch-3.0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala#L474
lag in spark 3.2 https://github.com/apache/spark/blob/branch-3.2/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala#L538
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#issuecomment-2337941740
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#issuecomment-2337649959
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#discussion_r1751163802
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/WindowFunctionsBuilder.scala:
##########
@@ -31,7 +31,7 @@ object WindowFunctionsBuilder {
val substraitFunc = windowFunc match {
// Handle lag with negative inputOffset, e.g., converts lag(c1, -1) to lead(c1, 1).
// Spark uses `-inputOffset` as `offset` for Lag function.
- case lag: Lag if lag.offset.eval(EmptyRow).asInstanceOf[Int] > 0 =>
Review Comment:
@kecookier, I see. Thanks for your clarification! Seems there is no way to let gluten adapt to the two implementations. Should you fix it in a forked gluten on your side?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "kecookier (via GitHub)" <gi...@apache.org>.
kecookier commented on PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#issuecomment-2337761870
@PHILO-HE Can you help review this PR?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "kecookier (via GitHub)" <gi...@apache.org>.
kecookier commented on code in PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#discussion_r1751205362
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/WindowFunctionsBuilder.scala:
##########
@@ -31,7 +31,7 @@ object WindowFunctionsBuilder {
val substraitFunc = windowFunc match {
// Handle lag with negative inputOffset, e.g., converts lag(c1, -1) to lead(c1, 1).
// Spark uses `-inputOffset` as `offset` for Lag function.
- case lag: Lag if lag.offset.eval(EmptyRow).asInstanceOf[Int] > 0 =>
Review Comment:
Ok, I will close this PR.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "kecookier (via GitHub)" <gi...@apache.org>.
kecookier commented on PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#issuecomment-2339547750
I will make it compatible with Spark 3.0 in the internal repo.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#issuecomment-2337649554
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICEGJ-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on Github Issues?
https://github.com/apache/incubator-gluten/issues
Then could you also rename ***commit message*** and ***pull request title*** in the following format?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #7168:
URL: https://github.com/apache/incubator-gluten/pull/7168#discussion_r1750224806
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/WindowFunctionsBuilder.scala:
##########
@@ -31,7 +31,7 @@ object WindowFunctionsBuilder {
val substraitFunc = windowFunc match {
// Handle lag with negative inputOffset, e.g., converts lag(c1, -1) to lead(c1, 1).
// Spark uses `-inputOffset` as `offset` for Lag function.
- case lag: Lag if lag.offset.eval(EmptyRow).asInstanceOf[Int] > 0 =>
Review Comment:
@kecookier, seems offset > 0 implies inputOffset < 0, so the original handling is expected, right?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL][MINOR] Fix typo error when convert lag() to lead() [incubator-gluten]
Posted by "kecookier (via GitHub)" <gi...@apache.org>.
kecookier closed pull request #7168: [VL][MINOR] Fix typo error when convert lag() to lead()
URL: https://github.com/apache/incubator-gluten/pull/7168
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org