[SPARK-45592][SPARK-45282][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec #43729

eejbyfeldt · 2023-11-09T08:39:41Z

What changes were proposed in this pull request?

This backports #43435 SPARK-45592 to the 3.4 branch. This is because it was already reported there as SPARK-45282 but it required enabling some extra configuration to hit the bug.

Why are the changes needed?

Fix correctness issue.

Does this PR introduce any user-facing change?

Yes, fixing correctness issue.

How was this patch tested?

New tests based on the reproduction example in SPARK-45282

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

No

Fixes correctness issue in 3.5.0. The problem seems to be that when AQEShuffleRead does a coalesced read it can return a HashPartitioning with the coalesced number of partitions. This causes a correctness bug as the partitioning is not compatible for joins with other HashPartitioning even though the number of partitions matches. This is resolved in this patch by introducing CoalescedHashPartitioning and making AQEShuffleRead return that instead. The fix was suggested by cloud-fan > AQEShuffleRead should probably return a different partitioning, e.g. CoalescedHashPartitioning. It still satisfies ClusterDistribution, so Aggregate is fine and there will be no shuffle. For joins, two CoalescedHashPartitionings are compatible if they have the same original partition number and coalesce boundaries, and CoalescedHashPartitioning is not compatible with HashPartitioning. Correctness bug. Yes, fixed correctness issue. New and existing unit test. No Closes apache#43435 from eejbyfeldt/SPARK-45592. Authored-by: Emil Ejbyfeldt <eejbyfeldt@liveintent.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 2be03d8) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

dongjoon-hyun · 2023-11-09T17:18:14Z

Thank you, @eejbyfeldt .

dongjoon-hyun · 2023-11-09T17:18:30Z

Also, cc @viirya and @sunchao , too.

dongjoon-hyun

Hi, @eejbyfeldt .

We can reuse the main title of PR because the code is the same.

[SPARK-45592][SQL] Correctness issue in AQE with InMemoryTableScanExec

Let me revise this PR title.

dongjoon-hyun

+1, LGTM from my side. Thank you so much for helping Apache Spark 3.4.2 release., @eejbyfeldt .

cc @cloud-fan , too

sunchao · 2023-11-09T17:30:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala

+ * fewer number of partitions.
+ */
+case class CoalescedHashPartitioning(from: HashPartitioning, partitions: Seq[CoalescedBoundary])
+  extends Expression with Partitioning with Unevaluable {


hmm why this needs to extend Expression and Unevaluable, I thought just Partitioning is enough.

It was just based on how it was done for HashPartitioning, could be that it not needed.

sunchao · 2023-11-09T17:34:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala

+    case SinglePartitionShuffleSpec =>
+      numPartitions == 1
+    case CoalescedHashShuffleSpec(otherParent, otherPartitions) =>
+      partitions == otherPartitions && from.isCompatibleWith(otherParent)


suppose both from and otherParent are HashShuffleSpec, is it possible that they may have different number of partitions, but after coalescing the number of partitions become the same?

In this case we'll consider the two incompatible but they actually should be?

Even they are coalesced to same number of partitions, the coalesced boundary could be different. I think this is root of the issue and why it needs to make sure boundaries are the same when checking compatibility.

Hmm, this is not related to my above comment. The check partitions == otherPartitions is OK, my question is on from.isCompatibleWith(otherParent), which checks (assuming both are HashShuffleSpec) whether the original number of partitions are the same.

I think it must to be HashShuffleSpec. You mean that their partition numbers can be different but compatible?

I mean their partition numbers are different and thus from.isCompatibleWith(otherParent) will return false, and thus cause this CoalescedHashShuffleSpec.isCompatibleWith to also return false. But should we return true instead if the partitions after coalescing are the same?

In other words, from.isCompatibleWith(otherParent) could be too conservative

Hmm, if first hash partitioning is 5 partitions, second is 4 partitions. How can we get same coalesced partitions with that?

For example:

[[0, 3], [3, 5]] != [[0, 3], [3, 4]]
[[0, 2], [2, 3], [3, 5]] != [[0, 2], [2, 3], [3, 4]]

The end reducer index of last coalesced partition should be different always, no?

I'm not sure whether it is possible for this case to happen. But irrespective of that, I feel this check is unnecessary here.

Of course this is relatively minor stuff and not related to this backport.

But what about two (nonsensical) CoalescedHashShuffleSpec where is partition is just coalesced from single partition from the some parent HashShuffleSpec. Is it not then correct to do the from.isCompatibleWith(otherParent) check? or is it expected that we can make them compatible by just coalescing them in such a way?

dongjoon-hyun · 2023-11-09T17:56:33Z

To @sunchao , if you don't mind, could you comment on the original PR once more? If we need those change, we need to start from master/3.5 branches.

[SPARK-45592][SQL] Correctness issue in AQE with InMemoryTableScanExec #43435

viirya · 2023-11-09T18:13:48Z

This is because it was already reported there as SPARK-45282 but it required enabling some extra configuration to hit the bug.

I took a look at the JIRA. So the bug is only happened on 3.4 if these configurations are enabled? But they are not needed in 3.5?

viirya

This backport looks good. I just wonder why in 3.4 these configuration are needed to trigger the bug.

viirya · 2023-11-09T18:16:52Z

sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

@@ -2461,6 +2461,19 @@ class DatasetSuite extends QueryTest
    )
    assert(result == expected)
  }
+
+  test("SPARK-45592: Coaleasced shuffle read is not compatible with hash partitioning") {


This test looks similar to the reproducer reported in SPARK-45282. Does it need the configurations used in the reproducer?

Or we need to add the reproducer as new test (with the configurations) in 3.4?

Changed the test to actually reproduce the issue on 3.4.

viirya · 2023-11-09T18:21:44Z

Manually using the reproduction example in the SPARK-45282 ticket.

Is it possible to add it as test?

sunchao · 2023-11-09T18:26:47Z

could you comment on the original PR once more

OK I cross posted there

maryannxue · 2023-11-09T18:54:00Z

First of all, the test does NOT repro for us. It's not written in a robust way (certain enough to trigger the bug).
Second, this patch seems to prevent some legit coalescing from happening, which can cause perf issues.

Can we close this one for now? We will push a new fix out to OSS soon.

eejbyfeldt · 2023-11-09T19:40:49Z

This backport looks good. I just wonder why in 3.4 these configuration are needed to trigger the bug.

The defaults were change in 3.5: SPARK-42768 but the test case from SPARK-45592 also depends on newer features like SPARK-42101. I there are many different ways to hit the same root bug.

eejbyfeldt · 2023-11-09T20:11:31Z

Hi, @eejbyfeldt .

We can reuse the main title of PR because the code is the same.
[SPARK-45592][SQL] Correctness issue in AQE with InMemoryTableScanExec
Let me revise this PR title.

The title might have been slightly miss leading as InMemoryTableScanExec was only enable in AQE in 3.5.0 SPARK-42101. Which is how is need to reproduce using the test case in SPARK-45592 but I guess the root cause of the bug existed before as shown by the case describe in SPARK-45282 .

dongjoon-hyun · 2023-11-09T22:08:14Z

To @eejbyfeldt , feel free to revise back if you think so.

To @maryannxue , for the following comment, although we can wait for a new fix, I don't think we need to close this PR first because we don't know what you are going to propose there. I'm looking forward to reviewing your PR. Please let us know when a new fix is ready.

Can we close this one for now? We will push a new fix out to OSS soon.

To be clear, I'm reviewing this PR as the Apache Spark 3.4.2 release manager. I'd love to deliver the Apache Spark 3.4.2 with the proper fix.

dongjoon-hyun · 2023-11-12T21:52:55Z

First of all, @maryannxue 's comment was about the original PR, not for specific this PR (branch-3.4).

First of all, the test does NOT repro for us. It's not written in a robust way (certain enough to trigger the bug).
Second, this patch seems to prevent some legit coalescing from happening, which can cause perf issues.

Second, she already agreed the original PR here.

[SPARK-45592][SQL] Correctness issue in AQE with InMemoryTableScanExec #43435 (comment)

Synced with @cloud-fan offline, (2) in the above suggestion wouldn't work. Let's go ahead with current fix.

Third, given the situation on @maryannxue 's PR (#43760), it's not written for branch-3.4 at all. To me, it seems that we need to spend more times on her PR. In addition, I'd like to have the consistent status for Apache Spark 4.0/3.5.2/3.4.2. In other words, her patch and test case will land in the same way to master/3.5/3.4 later.

#43760 (comment)

Lastly, for now, there is no landed patch not only branch-3.4 but also for master and branch-3.5. As the release manager of Apache Spark 3.4.2, I'm going to merge @eejbyfeldt 's contribution first as the fix and will wait @maryannxue 's further contribution.

dongjoon-hyun · 2023-11-12T21:53:33Z

The failure is irrelevant memory issue. Merged to branch-3.4.

…MemoryTableScanExec ### What changes were proposed in this pull request? This backports #43435 SPARK-45592 to the 3.4 branch. This is because it was already reported there as SPARK-45282 but it required enabling some extra configuration to hit the bug. ### Why are the changes needed? Fix correctness issue. ### Does this PR introduce _any_ user-facing change? Yes, fixing correctness issue. ### How was this patch tested? New tests based on the reproduction example in SPARK-45282 ### Was this patch authored or co-authored using generative AI tooling? No Closes #43729 from eejbyfeldt/SPARK-45282. Authored-by: Emil Ejbyfeldt <eejbyfeldt@liveintent.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2023-11-12T21:55:21Z

Merged to branch-3.4. Thank you all to help Apache Spark 3.4.2 release. I'll also share the AS-IS status in the dev mailing thread.

…MemoryTableScanExec ### What changes were proposed in this pull request? This backports apache#43435 SPARK-45592 to the 3.4 branch. This is because it was already reported there as SPARK-45282 but it required enabling some extra configuration to hit the bug. ### Why are the changes needed? Fix correctness issue. ### Does this PR introduce _any_ user-facing change? Yes, fixing correctness issue. ### How was this patch tested? New tests based on the reproduction example in SPARK-45282 ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#43729 from eejbyfeldt/SPARK-45282. Authored-by: Emil Ejbyfeldt <eejbyfeldt@liveintent.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

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

dongjoon-hyun reviewed Nov 9, 2023

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-45282][SQL][3.4] Backport SPARK-45592 to 3.4 branch~~ [SPARK-45592][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec Nov 9, 2023

dongjoon-hyun changed the title ~~[SPARK-45592][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec~~ [SPARK-45592][SPARK-45282][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec Nov 9, 2023

dongjoon-hyun approved these changes Nov 9, 2023

View reviewed changes

sunchao reviewed Nov 9, 2023

View reviewed changes

viirya approved these changes Nov 9, 2023

View reviewed changes

viirya reviewed Nov 9, 2023

View reviewed changes

Use test case from SPARK-45282

cdc4498

dongjoon-hyun closed this Nov 12, 2023

dongjoon-hyun mentioned this pull request Nov 13, 2023

[SPARK-45592][SPARK-45282][SQL] Correctness issue in AQE with InMemoryTableScanExec #43760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-45592][SPARK-45282][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec #43729

[SPARK-45592][SPARK-45282][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec #43729

eejbyfeldt commented Nov 9, 2023 •

edited

dongjoon-hyun commented Nov 9, 2023

dongjoon-hyun commented Nov 9, 2023

dongjoon-hyun left a comment

dongjoon-hyun left a comment

sunchao Nov 9, 2023

eejbyfeldt Nov 9, 2023

sunchao Nov 9, 2023

viirya Nov 9, 2023

sunchao Nov 9, 2023

viirya Nov 9, 2023

sunchao Nov 9, 2023

sunchao Nov 9, 2023

viirya Nov 9, 2023

sunchao Nov 9, 2023

sunchao Nov 9, 2023

eejbyfeldt Nov 9, 2023

dongjoon-hyun commented Nov 9, 2023

viirya commented Nov 9, 2023

viirya left a comment

viirya Nov 9, 2023

viirya Nov 9, 2023

eejbyfeldt Nov 9, 2023

viirya commented Nov 9, 2023

sunchao commented Nov 9, 2023

maryannxue commented Nov 9, 2023

eejbyfeldt commented Nov 9, 2023 •

edited

eejbyfeldt commented Nov 9, 2023 •

edited

dongjoon-hyun commented Nov 9, 2023 •

edited

dongjoon-hyun commented Nov 12, 2023 •

edited

dongjoon-hyun commented Nov 12, 2023

dongjoon-hyun commented Nov 12, 2023

[SPARK-45592][SPARK-45282][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec #43729

[SPARK-45592][SPARK-45282][SQL][3.4] Correctness issue in AQE with InMemoryTableScanExec #43729

Conversation

eejbyfeldt commented Nov 9, 2023 • edited

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?

dongjoon-hyun commented Nov 9, 2023

dongjoon-hyun commented Nov 9, 2023

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 9, 2023

viirya commented Nov 9, 2023

viirya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Nov 9, 2023

sunchao commented Nov 9, 2023

maryannxue commented Nov 9, 2023

eejbyfeldt commented Nov 9, 2023 • edited

eejbyfeldt commented Nov 9, 2023 • edited

dongjoon-hyun commented Nov 9, 2023 • edited

dongjoon-hyun commented Nov 12, 2023 • edited

dongjoon-hyun commented Nov 12, 2023

dongjoon-hyun commented Nov 12, 2023

eejbyfeldt commented Nov 9, 2023 •

edited

eejbyfeldt commented Nov 9, 2023 •

edited

eejbyfeldt commented Nov 9, 2023 •

edited

dongjoon-hyun commented Nov 9, 2023 •

edited

dongjoon-hyun commented Nov 12, 2023 •

edited