Repository navigation
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Three Spark patches changed
localReads.lengthto1while retaining access tolocalReads(1), making the Spark-only baseline test impossible to pass. - Design approach: Restore Spark’s assertion by removing the overriding hunk from the 3.5.9, 4.0.4 and 4.1.3 patches.
- Correctness / compatibility analysis: Applied the base and head AQE patch sections to each matching Spark tag. All applied successfully and matched their declared blob hashes. Each restored test body matches upstream exactly, apart from the retained
IgnoreComettag. - Key design decisions: Preserve Comet’s existing test exclusion and reuse upstream behavior without adding branches or abstractions.
- Implementation sketch: Remove the incorrect assertion and accompanying comment. All other changes are verified patch hashes and downstream offsets.
- Behavioral changes worth calling out: The baseline test again expects two local reads. Comet-enabled runs still skip it. Production execution and performance are unchanged.
- Suggested improvements: None meeting the reporting threshold. No introduced P1/P2 issues found within this review.
Reviewed the complete diff from 4ca04b155bed7abe9e0e267f6438c31cca2c278d to f39539fa12382817a472f30446f97a8c57ea2396. The PR is not a draft. Read the supplied snapshot, live discussion and linked issue #6122. No existing review concerns remain unresolved.
Routed skills: review-comet-pr and review-comet-shuffle-pr.
Exact-head CI: label passed. Comet CI and CodeQL report action_required. No build or Spark-suite verdict is available.
Validation limits: Patch application, source comparison, offset checks and git diff --check passed. Scala/Spark suites were not run locally because no prepared Spark/Comet build was available. The author-reported runtime results were not independently reproduced.
|
|
|
Thanks for catching this, @andygrove ! I’ve updated the 3.4.3 diff and pushed the fix. |
|
@mizulun could you fix conflicts? |
|
Sure, I’ll fix the conflicts as soon as possible. Thanks for the reminder! |
Which issue does this PR close?
Closes #6122.
Rationale for this change
In
AdaptiveQueryExecSuite, the testReuse the default parallelism in local shuffle readhad its assertion changed from Spark'sassert(localReads.length == 2)to== 1in the 3.5.9, 4.0.4 and 4.1.3 diffs. The next statement readslocalReads(1), so the test cannot pass for any length. With one local read it throwsIndexOutOfBoundsException, and with two the assertion fails.CI stays green because the test carries
IgnoreComet, so it only runs when Comet is disabled. That is the Spark-only baseline run. The 4.2.0 diff already keeps Spark's value, and this PR brings the other three diffs in line with it.What changes are included in this PR?
dev/diffs/3.5.9.diff,dev/diffs/4.0.4.diff,dev/diffs/4.1.3.diff: drop the// Comet shuffle changes shuffle metricscomment and restoreassert(localReads.length == 2).The
IgnoreComettag on the test stays.spark-sql-tests.md: apply it to the Spark tag, edit the source, then rungit diff. Apart from the removed hunk, the only changes are the line offsets of later hunks inAdaptiveQueryExecSuite.scala.How are these changes tested?
For each Spark version, the diff was applied to its tag checkout and the test was run in both modes:
ENABLE_COMET=falseENABLE_COMET=trueThe baseline run now exercises the restored assertion, and the Comet run still skips the test through the existing
IgnoreComettag.