[SPARK-45920][SQL] group by ordinal should be idempotent #43797

cloud-fan · 2023-11-14T07:45:56Z

What changes were proposed in this pull request?

GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal.

Why are the changes needed?

For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable.

Does this PR introduce any user-facing change?

No

How was this patch tested?

new test

Was this patch authored or co-authored using generative AI tooling?

no

cloud-fan · 2023-11-14T07:46:43Z

cc @gengliangwang @viirya @beliefer

viirya · 2023-11-14T09:05:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+                case Literal(_: Int, IntegerType) =>
+                  Literal(index)


Is this only for IntegerType? If ordinalExpr is something like cast(long as int) that is foldable?

I'm only fixing repeated analysis (analyzing an analyzed plan). It's a much bigger topic to support analyzing an optimized plan, so constant folding is not considered here.

viirya · 2023-11-14T09:06:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+                //             analyze the plan. Using a different integer literal may lead to
+                //             a repeat GROUP BY ordinal resolution which is wrong. GROUP BY
+                //             constant is meaningless so whatever value does not matter here.
+                // TODO: GROUP BY ordinal should pull out grouping expressions to a Project, then


So this is a temporary fix only?

I'd like to backport this surgical fix, and only do the refactoring in the master branch. So yes, this is a temporary fix for the master branch.

viirya

Looks okay for the targeted issue.

dongjoon-hyun · 2023-11-14T16:16:00Z

...rc/test/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinalsSuite.scala

    }
  }
+
+  test("group by ordinal repeated analysis") {


Shall we add a test prefix SPARK-45920:?

dongjoon-hyun · 2023-11-14T16:16:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+                //             analyze the plan. Using a different integer literal may lead to
+                //             a repeat GROUP BY ordinal resolution which is wrong. GROUP BY
+                //             constant is meaningless so whatever value does not matter here.
+                // TODO: GROUP BY ordinal should pull out grouping expressions to a Project, then


Could you file an official JIRA issue and make this as the IDed TODO, please?

dongjoon-hyun

Thank you for making this fix, @cloud-fan .

Given the following warning, this sounds like this could cause a correctness issue. Did I understand correctly?

a repeat GROUP BY ordinal resolution which is wrong.

cloud-fan · 2023-11-15T03:03:51Z

Given the following warning, this sounds like this could cause a correctness issue. Did I understand correctly?

Yes, but only when you manipulate logical plans directly. SQL/DataFrame API can't trigger it.

dongjoon-hyun

+1, LGTM. Thank you, @cloud-fan .

dongjoon-hyun · 2023-11-15T06:26:20Z

BTW, the first commit failed with the following relevant failure. Could you double-check that because the last commit seems irrelevant with that failure, @cloud-fan ?

dongjoon-hyun · 2023-11-16T08:46:26Z

Merged to master first.

dongjoon-hyun · 2023-11-16T08:47:25Z

In you don't mind, could you make a backporting PR to branch-3.5/3.4/3.3 separately? For this PR, I believe we should be more careful due to the different golden files per branches, @cloud-fan .

### What changes were proposed in this pull request? GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#43797 from cloud-fan/group. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. No new test no Closes apache#43797 from cloud-fan/group. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

backport #43797 ### What changes were proposed in this pull request? GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #43836 from cloud-fan/3.5-port. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

backport #43797 ### What changes were proposed in this pull request? GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #43838 from cloud-fan/3.4-port. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

backport #43797 ### What changes were proposed in this pull request? GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #43839 from cloud-fan/3.3-port. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

backport apache#43797 ### What changes were proposed in this pull request? GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#43838 from cloud-fan/3.4-port. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? This is a followup of #43797 . GROUP BY ALL has the same bug and this PR applies the same fix to GROUP BY ALL ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #46113 from cloud-fan/group-all. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a followup of #43797 . GROUP BY ALL has the same bug and this PR applies the same fix to GROUP BY ALL ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #46113 from cloud-fan/group-all. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit b5bb75c) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a followup of #43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? Yes, we will fix the error. ### How was this patch tested? Test added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50461 from mihailom-db/mihailom-db/fixGroupBy. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a followup of #43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? Yes, we will fix the error. ### How was this patch tested? Test added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50461 from mihailom-db/mihailom-db/fixGroupBy. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 02db872) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a followup of apache/spark#43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? Yes, we will fix the error. ### How was this patch tested? Test added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50461 from mihailom-db/mihailom-db/fixGroupBy. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…sis to avoid issue with group by ordinal ### What changes were proposed in this pull request? This is a followup to #43797 and #50461 where hacks were introduced in order to solve the issue of repeated analysis on plans that have a group by ordinal. The latter PR caused a regression so the hack needs to be removed. This PR proposed a move of `UnresolvedOrdinal` construction before Analyzer runs. ### Why are the changes needed? We are reverting a hack introduced in the previous PRs to improve the behavior of group by ordinal and additionally fix the issue that #50461 was trying to solve. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #50606 from mihailotim-db/mihailotim-db/new_group_by_ordinal. Authored-by: Mihailo Timotic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…tent (apache#333) [SPARK-45920][SQL][3.5] group by ordinal should be idempotent backport apache#43797 ### What changes were proposed in this pull request? GROUP BY ordinal is not idempotent today. If the ordinal points to another integer literal and the plan get analyzed again, we will re-do the ordinal resolution which can lead to wrong result or index out-of-bound error. This PR fixes it by using a hack: if the ordinal points to another integer literal, don't replace the ordinal. ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#43836 from cloud-fan/3.5-port. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> Co-authored-by: Wenchen Fan <[email protected]>

This is a followup of apache#43797 . GROUP BY ALL has the same bug and this PR applies the same fix to GROUP BY ALL For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. no new test no Closes apache#46113 from cloud-fan/group-all. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit b5bb75c) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This is a followup of apache#43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? Yes, we will fix the error. ### How was this patch tested? Test added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50461 from mihailom-db/mihailom-db/fixGroupBy. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 9cc8b18) Signed-off-by: Wenchen Fan <[email protected]>

group by ordinal should be idempotent

3deef67

github-actions bot added the SQL label Nov 14, 2023

viirya reviewed Nov 14, 2023

View reviewed changes

viirya approved these changes Nov 14, 2023

View reviewed changes

dongjoon-hyun reviewed Nov 14, 2023

View reviewed changes

HyukjinKwon approved these changes Nov 15, 2023

View reviewed changes

address comments

bf34e26

dongjoon-hyun approved these changes Nov 15, 2023

View reviewed changes

fix golden

457d14e

dongjoon-hyun approved these changes Nov 16, 2023

View reviewed changes

dongjoon-hyun closed this in daebb59 Nov 16, 2023

This was referenced Nov 16, 2023

[SPARK-45920][SQL][3.5] group by ordinal should be idempotent #43836

Closed

[SPARK-45920][SQL][3.4] group by ordinal should be idempotent #43838

Closed

[SPARK-45920][SQL][3.3] group by ordinal should be idempotent #43839

Closed

cloud-fan mentioned this pull request Apr 18, 2024

[SPARK-47895][SQL] group by all should be idempotent #46113

Closed

mihailomilosevic2001 mentioned this pull request Mar 31, 2025

[SPARK-47895][SQL] group by alias should be idempotent #50461

Closed

mihailotim-db mentioned this pull request Apr 16, 2025

[SPARK-51820][SQL] Move UnresolvedOrdinal construction before analysis to avoid issue with group by ordinal #50606

Closed

[SPARK-45920][SQL] group by ordinal should be idempotent #43797

[SPARK-45920][SQL] group by ordinal should be idempotent #43797

Uh oh!

Conversation

cloud-fan commented Nov 14, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

cloud-fan commented Nov 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 15, 2023

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Nov 15, 2023

Uh oh!

dongjoon-hyun commented Nov 16, 2023

Uh oh!

dongjoon-hyun commented Nov 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants