You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "agubichev (via GitHub)" <gi...@apache.org> on 2023/09/01 18:36:47 UTC
[GitHub] [spark] agubichev opened a new pull request, #42778: [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
agubichev opened a new pull request, #42778:
URL: https://github.com/apache/spark/pull/42778
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
TransposeWindows rule reorders parent and child window functions. Currently it incorrectly reorders the window functions in cases where the top window function orders by on the result of the bottom window function, e.g. `sum1` in the following example:
`SELECT ROW_NUMBER() OVER (PARTITION BY C ORDER BY sum1)
FROM (SELECT ROW_NUMBER() OVER (PARTITION BY C) as sum1 FROM T)
`
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
TransposeWindows currently produces invalid query plans for the case described above.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Unit test.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
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
[GitHub] [spark] agubichev commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "agubichev (via GitHub)" <gi...@apache.org>.
agubichev commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1315214615
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1305,11 +1305,20 @@ object TransposeWindow extends Rule[LogicalPlan] {
}
}
+ // Returns false if the parent 'w1' window function order by any of the window
+ // expressions produced by 'w2'.
+ private def compatibleOrderBy(w1: Window, w2: Window): Boolean = {
+ val childWindowExprs = w2.windowExpressions.map(x => x.toAttribute)
+ val parentOrder = w1.orderSpec.flatMap(x => x.references)
+ childWindowExprs.intersect(parentOrder).isEmpty
+ }
+
private def windowsCompatible(w1: Window, w2: Window): Boolean = {
w1.references.intersect(w2.windowOutputSet).isEmpty &&
Review Comment:
It's a subtle bug.
`references` by definition do not include output attributes (see https://github.com/apache/spark/blob/798fce3b571907ee52058004cc38c2e8dbc4b016/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L91).
So if the top Window function orders by C and projects (outputs) C, then C is not included into references.
Therefore, we need that extra check.
(and we can't just take all attributes in the Window operator either, since PARTITION BY clauses do need to intersect)
--
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
[GitHub] [spark] agubichev commented on pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "agubichev (via GitHub)" <gi...@apache.org>.
agubichev commented on PR #42778:
URL: https://github.com/apache/spark/pull/42778#issuecomment-1703200108
> Looks good, thanks for the fix.
>
> Can you also create and link a jira for the bug?
Added
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1315322373
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1305,11 +1305,20 @@ object TransposeWindow extends Rule[LogicalPlan] {
}
}
+ // Returns false if the parent 'w1' window function order by any of the window
+ // expressions produced by 'w2'.
+ private def compatibleOrderBy(w1: Window, w2: Window): Boolean = {
+ val childWindowExprs = w2.windowExpressions.map(x => x.toAttribute)
+ val parentOrder = w1.orderSpec.flatMap(x => x.references)
+ childWindowExprs.intersect(parentOrder).isEmpty
+ }
+
private def windowsCompatible(w1: Window, w2: Window): Boolean = {
w1.references.intersect(w2.windowOutputSet).isEmpty &&
Review Comment:
no, the `Window` operator won't exclude C, as it only excludes `windowOutputSet`
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1314425064
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1305,11 +1305,20 @@ object TransposeWindow extends Rule[LogicalPlan] {
}
}
+ // Returns false if the parent 'w1' window function order by any of the window
+ // expressions produced by 'w2'.
+ private def compatibleOrderBy(w1: Window, w2: Window): Boolean = {
+ val childWindowExprs = w2.windowExpressions.map(x => x.toAttribute)
+ val parentOrder = w1.orderSpec.flatMap(x => x.references)
+ childWindowExprs.intersect(parentOrder).isEmpty
+ }
+
private def windowsCompatible(w1: Window, w2: Window): Boolean = {
w1.references.intersect(w2.windowOutputSet).isEmpty &&
Review Comment:
I'm a bit confused. Isn't the ORDER BY column part of `Window#references`? The code here should prevent this bug from happening IIUC.
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1315321463
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,18 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
Review Comment:
This tests pass with the latest master branch.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,18 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
Review Comment:
This test passes with the latest master branch.
--
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
[GitHub] [spark] agubichev commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "agubichev (via GitHub)" <gi...@apache.org>.
agubichev commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1316176643
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,18 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
Review Comment:
modified the test to reflect the bug better. Now it fails at the latest 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-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
[GitHub] [spark] agubichev commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "agubichev (via GitHub)" <gi...@apache.org>.
agubichev commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1315214615
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1305,11 +1305,20 @@ object TransposeWindow extends Rule[LogicalPlan] {
}
}
+ // Returns false if the parent 'w1' window function order by any of the window
+ // expressions produced by 'w2'.
+ private def compatibleOrderBy(w1: Window, w2: Window): Boolean = {
+ val childWindowExprs = w2.windowExpressions.map(x => x.toAttribute)
+ val parentOrder = w1.orderSpec.flatMap(x => x.references)
+ childWindowExprs.intersect(parentOrder).isEmpty
+ }
+
private def windowsCompatible(w1: Window, w2: Window): Boolean = {
w1.references.intersect(w2.windowOutputSet).isEmpty &&
Review Comment:
It's a subtle bug.
`references` by definition do not include output attributes (see https://github.com/apache/spark/blob/798fce3b571907ee52058004cc38c2e8dbc4b016/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L91).
So if the top Window function orders by C and projects (outputs) C, then C is not included into references.
Therefore, we need that extra check.
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1317021295
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,38 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
+ // Parent orders by the window expression of the child, no reordering.
+ // Child plan is SELECT a, b, ROW_NUMBER() OVER (PARTITION BY a, b ORDER BY a) as rn_child
+ val input = testRelation.analyze
+ val childPartitionFields = Seq(a, b)
+ val childOrderByFields = Seq(a.asc)
+ val windowFrame = SpecifiedWindowFrame(RowFrame, UnboundedPreceding, CurrentRow)
+
+ val childRowNumber = WindowExpression(RowNumber(),
+ WindowSpecDefinition(childPartitionFields, childOrderByFields, windowFrame))
+ val childRowNumberAlias = childRowNumber.as("rn_child")
+ val childRowNumberRef = childRowNumberAlias.toAttribute
+
+ val childWindow = Window(Seq(childRowNumberAlias) ++ childPartitionFields,
+ childPartitionFields, childOrderByFields, input)
Review Comment:
After reading the code more, the window query plan does support a project list, although we only use window expressions as the project list in `Window`.
The only exception is `DecorrelateInnerQuery`. The newly added code for window decorrelation will add attributes to `Window#windowExpressions`. Shall we fix it instead of making general projectList work properly in Window?
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1316894335
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,38 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
+ // Parent orders by the window expression of the child, no reordering.
+ // Child plan is SELECT a, b, ROW_NUMBER() OVER (PARTITION BY a, b ORDER BY a) as rn_child
+ val input = testRelation.analyze
+ val childPartitionFields = Seq(a, b)
+ val childOrderByFields = Seq(a.asc)
+ val windowFrame = SpecifiedWindowFrame(RowFrame, UnboundedPreceding, CurrentRow)
+
+ val childRowNumber = WindowExpression(RowNumber(),
+ WindowSpecDefinition(childPartitionFields, childOrderByFields, windowFrame))
+ val childRowNumberAlias = childRowNumber.as("rn_child")
+ val childRowNumberRef = childRowNumberAlias.toAttribute
+
+ val childWindow = Window(Seq(childRowNumberAlias) ++ childPartitionFields,
+ childPartitionFields, childOrderByFields, input)
Review Comment:
This is not a valid `Window` operator as `Window#windowExpressions` must be `Alias(WindowExpression(...), ...)`.
--
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-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
URL: https://github.com/apache/spark/pull/42778
--
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-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #42778:
URL: https://github.com/apache/spark/pull/42778#issuecomment-1867077881
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1323019007
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,38 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
+ // Parent orders by the window expression of the child, no reordering.
+ // Child plan is SELECT a, b, ROW_NUMBER() OVER (PARTITION BY a, b ORDER BY a) as rn_child
+ val input = testRelation.analyze
+ val childPartitionFields = Seq(a, b)
+ val childOrderByFields = Seq(a.asc)
+ val windowFrame = SpecifiedWindowFrame(RowFrame, UnboundedPreceding, CurrentRow)
+
+ val childRowNumber = WindowExpression(RowNumber(),
+ WindowSpecDefinition(childPartitionFields, childOrderByFields, windowFrame))
+ val childRowNumberAlias = childRowNumber.as("rn_child")
+ val childRowNumberRef = childRowNumberAlias.toAttribute
+
+ val childWindow = Window(Seq(childRowNumberAlias) ++ childPartitionFields,
+ childPartitionFields, childOrderByFields, input)
Review Comment:
I gave it up. It seems OK and convenient to keep a projectList in `Window`. Shall we just remove `Window#producedAttributes`? `Project` and `Aggregate` do not have it and use `Nil` as the default, `Window` should follow them.
--
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
[GitHub] [spark] agubichev commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "agubichev (via GitHub)" <gi...@apache.org>.
agubichev commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1315214615
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1305,11 +1305,20 @@ object TransposeWindow extends Rule[LogicalPlan] {
}
}
+ // Returns false if the parent 'w1' window function order by any of the window
+ // expressions produced by 'w2'.
+ private def compatibleOrderBy(w1: Window, w2: Window): Boolean = {
+ val childWindowExprs = w2.windowExpressions.map(x => x.toAttribute)
+ val parentOrder = w1.orderSpec.flatMap(x => x.references)
+ childWindowExprs.intersect(parentOrder).isEmpty
+ }
+
private def windowsCompatible(w1: Window, w2: Window): Boolean = {
w1.references.intersect(w2.windowOutputSet).isEmpty &&
Review Comment:
It's a subtle bug.
`references` by definition do not include output attributes (see https://github.com/apache/spark/blob/798fce3b571907ee52058004cc38c2e8dbc4b016/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L91).
So if the top Window function order by C and projects (outputs) C, then C is not included into references.
Therefore, we need that extra check.
(and we can't just take all attributes in the Window operator either, since PARTITION BY clauses do need to intersect)
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #42778: [SPARK-45055] [SQL] Do not transpose windows if they conflict on ORDER BY / PROJECT clauses
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42778:
URL: https://github.com/apache/spark/pull/42778#discussion_r1317149818
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala:
##########
@@ -160,4 +160,38 @@ class TransposeWindowSuite extends PlanTest {
comparePlans(optimized, correctAnswer.analyze)
}
+ test("two windows with overlapping project/order by lists") {
+ // Parent orders by the window expression of the child, no reordering.
+ // Child plan is SELECT a, b, ROW_NUMBER() OVER (PARTITION BY a, b ORDER BY a) as rn_child
+ val input = testRelation.analyze
+ val childPartitionFields = Seq(a, b)
+ val childOrderByFields = Seq(a.asc)
+ val windowFrame = SpecifiedWindowFrame(RowFrame, UnboundedPreceding, CurrentRow)
+
+ val childRowNumber = WindowExpression(RowNumber(),
+ WindowSpecDefinition(childPartitionFields, childOrderByFields, windowFrame))
+ val childRowNumberAlias = childRowNumber.as("rn_child")
+ val childRowNumberRef = childRowNumberAlias.toAttribute
+
+ val childWindow = Window(Seq(childRowNumberAlias) ++ childPartitionFields,
+ childPartitionFields, childOrderByFields, input)
Review Comment:
I'm making a try with https://github.com/apache/spark/pull/42837
--
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