[SPARK-56250][SQL] Remove confusing defensive code in SortExec.rowSorter and add warning comment by viirya · Pull Request #55048 · apache/spark

viirya · 2026-03-27T02:21:03Z

What changes were proposed in this pull request?

SPARK-52609 added some defensive code to SortExec. The defensive has some issues and was fixed by SPARK-56203.

But actually the defensive code is not for Spark itself but to guard multithreading access to SortExec that isn't an issue to Spark itself. The defensive code could easily confuse others. After rethinking about it, it might be better to revert it and add some warning comment.

Why are the changes needed?

To simplify the code and reduce confusion.

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?

Generated-by: Claude Sonnet 4.6

peter-toth · 2026-03-27T07:05:21Z

Let me doublecheck something today and get back to you @viirya .

peter-toth · 2026-03-27T12:16:44Z

Yeah, using theadlocal there doesn't make much sense. I was actually investigating a very different issue when the theoretical problem in #55006 came up, but it had nothing to do with my root problem. Reverting both commits looks ok to me.

peter-toth · 2026-03-27T12:21:56Z

Test failures seem real, but unrelated to this change.

Maybe #54854 is causing them? cc @HeartSaVioR

dongjoon-hyun

Got it. +1 for reverting.

dongjoon-hyun

BTW, @viirya , I believe we should remove some discussion part in the PR description because it never happens in the community.

After some discussion and rethinking about it, it might be better to revert it and add some warning comment.

In addition, the PR title is correct but very misleading because the code itself is Reverting instead of simply adding a comment. This PR includes a code change. It would be great if the PR title clarifies that we are chaining the code itself.

viirya · 2026-03-27T17:47:53Z

Thank you @dongjoon-hyun. I updated the PR description and title.

dongjoon-hyun · 2026-03-27T17:55:55Z

Thank you so much. BTW, I saw that this is created Improvement. However, can we have this version of code change in branch-4.1 too?

viirya · 2026-03-27T18:05:48Z

Thank you so much. BTW, I saw that this is created Improvement. However, can we have this version of code change in branch-4.1 too?

Hmm, yes, maybe bug is more proper as I want to backport this.

viirya · 2026-03-27T18:07:06Z

I updated the JIRA ticket's title and changed it to bug.

dongjoon-hyun · 2026-03-27T18:08:10Z

Thank you again~

peter-toth · 2026-03-27T18:38:09Z

Test failures seem real, but unrelated to this change.
Maybe #54854 is causing them? cc @HeartSaVioR

Same failure again: read stream-stream join v4 state change feed *** FAILED ***. Also happened on my #55036 (comment). This is very likely due to that recently commited PR.

viirya · 2026-03-27T18:58:29Z

Same failure again: read stream-stream join v4 state change feed *** FAILED ***. Also happened on my #55036 (comment). This is very likely due to that recently commited PR.

If it is too flaky, maybe we should consider revert it first to unblock CI.

HeartSaVioR · 2026-03-28T00:08:00Z

I'm looking into this now. You can revert but we can probably fix forward if we can wait for a couple hours.

HeartSaVioR · 2026-03-28T00:53:37Z

#55071 waiting for CI

viirya · 2026-03-28T01:24:02Z

Thank you @HeartSaVioR

This reverts commit 1e52a02.

…be overwritten by other tasks in parallel" This reverts commit 26a9ba6.

…ces comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ter and add warning comment [SPARK-52609](https://issues.apache.org/jira/browse/SPARK-52609) added some defensive code to SortExec. The defensive has some issues and was fixed by [SPARK-56203](https://issues.apache.org/jira/browse/SPARK-56203). But actually the defensive code is not for Spark itself but to guard multithreading access to SortExec that isn't an issue to Spark itself. The defensive code could easily confuse others. After rethinking about it, it might be better to revert it and add some warning comment. To simplify the code and reduce confusion. No Existing tests Generated-by: Claude Sonnet 4.6 Closes#55048 from viirya/revert-sort-exec-defensive-code. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 842d4dd) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

viirya · 2026-03-28T22:28:41Z

Merged to master/4.1.

Thank you. @cloud-fan @HyukjinKwon @peter-toth @dongjoon-hyun @HeartSaVioR

LuciferYang · 2026-03-30T10:02:38Z

It looks like the build for the 4.1 branch has failed. Do you have time to fix it? @viirya

https://github.com/apache/spark/actions/runs/23695720926/job/69030618971

[error] /home/runner/work/spark/spark/sql/core/src/main/scala/org/apache/spark/sql/execution/SortExec.scala:77:17: type mismatch;
[error] found : ThreadLocal[org.apache.spark.sql.execution.UnsafeExternalRowSorter]
[error] required: org.apache.spark.sql.execution.UnsafeExternalRowSorter
[error] rowSorter = new ThreadLocal[UnsafeExternalRowSorter]()
[error] ^

viirya · 2026-03-30T16:52:31Z

It looks like the build for the 4.1 branch has failed. Do you have time to fix it? @viirya

* https://github.com/apache/spark/actions/runs/23695720926/job/69030618971

[error] /home/runner/work/spark/spark/sql/core/src/main/scala/org/apache/spark/sql/execution/SortExec.scala:77:17: type mismatch;
[error] found : ThreadLocal[org.apache.spark.sql.execution.UnsafeExternalRowSorter]
[error] required: org.apache.spark.sql.execution.UnsafeExternalRowSorter
[error] rowSorter = new ThreadLocal[UnsafeExternalRowSorter]()
[error] ^

Okay. I will fix it.

viirya · 2026-03-30T16:57:58Z

Fix it at #55098.

…SortExec.rowSorter and add warning comment ### What changes were proposed in this pull request? #55048 was backported to 4.1 but there is an error when resolving the conflicts. This patch fixes it. ### Why are the changes needed? Restoring the build of 4.1. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes#55098 from viirya/revert-sort-exec-defensive-code-4.1. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2026-03-30T17:18:10Z

The follow-up PR is merged.

viirya requested a review from cloud-fan March 27, 2026 02:21

viirya changed the title ~~[SPARK-56250] Add warning comment to SortExec.rowSorter~~[SPARK-56250][SQL] Add warning comment to SortExec.rowSorterMar 27, 2026

viirya requested a review from peter-toth March 27, 2026 02:22

cloud-fan approved these changes Mar 27, 2026
View reviewed changes

HyukjinKwon approved these changes Mar 27, 2026
View reviewed changes

peter-toth approved these changes Mar 27, 2026
View reviewed changes

peter-toth mentioned this pull request Mar 27, 2026
[SPARK-56241][SQL] Derive outputOrdering from KeyedPartitioning key expressions #55036
Closed

viirya force-pushed the revert-sort-exec-defensive-code branch from f26f367 to 4a721ceCompare March 27, 2026 16:14

dongjoon-hyun approved these changes Mar 27, 2026
View reviewed changes

dongjoon-hyun reviewed Mar 27, 2026
View reviewed changes

viirya changed the title ~~[SPARK-56250][SQL] Add warning comment to SortExec.rowSorter~~[SPARK-56250][SQL] Remove confusing defensive code in SortExec.rowSorter and add warning commentMar 27, 2026

viirya force-pushed the revert-sort-exec-defensive-code branch from 4a721ce to d3700dcCompare March 27, 2026 18:51

viiryaand others added 4 commits March 28, 2026 00:11

Revert "[SPARK-56203][SQL] Fix race condition in SortExec.rowSorter"
747ed46
This reverts commit 1e52a02.

Revert "[SPARK-52609][SQL] Make sure row sorter from each task won't …
8dadbb5
…be overwritten by other tasks in parallel" This reverts commit 26a9ba6.

[SQL] Add warning comment to SortExec.rowSorter and fix cleanupResour…
a776ecf
…ces comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

[SQL] Soften thread-safety wording in SortExec.rowSorter warning comment
a317af2
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

viirya force-pushed the revert-sort-exec-defensive-code branch from d3700dc to 6e53a99Compare March 28, 2026 07:12

[SQL] Fix non-ASCII em dash in SortExec.rowSorter warning comment
323c4f6
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

viirya force-pushed the revert-sort-exec-defensive-code branch from 6e53a99 to 323c4f6Compare March 28, 2026 16:07

viirya closed this in 842d4ddMar 28, 2026

viirya deleted the revert-sort-exec-defensive-code branch March 28, 2026 22:28

viirya mentioned this pull request Mar 30, 2026
[SPARK-56250][SQL][FOLLOWUP][4.1] Remove confusing defensive code in SortExec.rowSorter and add warning comment #55098
Closed

Uh oh!

Conversation

viirya commented Mar 27, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

peter-toth commented Mar 27, 2026

Uh oh!

peter-toth commented Mar 27, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peter-toth commented Mar 27, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Mar 27, 2026

Uh oh!

dongjoon-hyun commented Mar 27, 2026

Uh oh!

viirya commented Mar 27, 2026

Uh oh!

viirya commented Mar 27, 2026

Uh oh!

dongjoon-hyun commented Mar 27, 2026

Uh oh!

peter-toth commented Mar 27, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented Mar 27, 2026

Uh oh!

HeartSaVioR commented Mar 28, 2026

Uh oh!

HeartSaVioR commented Mar 28, 2026

Uh oh!

viirya commented Mar 28, 2026

Uh oh!

viirya commented Mar 28, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LuciferYang commented Mar 30, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented Mar 30, 2026

Uh oh!

viirya commented Mar 30, 2026

Uh oh!

dongjoon-hyun commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

viirya commented Mar 27, 2026•
edited
Loading

peter-toth commented Mar 27, 2026•
edited
Loading

peter-toth commented Mar 27, 2026•
edited
Loading

peter-toth commented Mar 27, 2026•
edited
Loading

viirya commented Mar 28, 2026•
edited
Loading

LuciferYang commented Mar 30, 2026•
edited
Loading