Skip to content

test: fix flaky mixed field id directory test on Spark 3.4 and 3.5 - #6312

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:investigate-issue-6301
Sep 28, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:investigate-issue-6301

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #6301.

Rationale for this change

The nightly failed on Spark 3.4 in ParquetReadV1Suite "a file without ids next to a file with ids is checked on its own", which #6116 added. The assertion that failed was on the Spark reference error, not Comet's, and the same test passed on Spark 3.4 in the previous nightly.

The test writes each side with sparkContext.parallelize on the five-core test session, so each write leaves an empty part-00000 next to two one-row files. The read packs the six files into three tasks: task 0 [ids, ids], task 1 [no ids, no ids] and task 2 [empty with ids, empty without ids]. On Spark 3.x, tasks 1 and 2 fail with different exceptions:

  • Task 1 fails on its first file and raises the bare RuntimeException from ParquetReadSupport.
  • Task 2 reads the file without ids after an empty file. Its error is raised inside the try { hasNext } in FileScanRDD.nextIterator, which wraps it in SparkException("Encountered error while reading file ...").

On 3.x the DAGScheduler wraps whichever task fails first in "Job aborted", and isMissingFieldIdsError looks only at getCause. The test passes when task 1 fails first and fails when task 2 does. The nightly log reports Task 2 in stage 2468.0 failed. Spark 3.5 has the same exposure. Spark 4.x wraps every read error once in FAILED_READ_FILE and does not add "Job aborted", so the test always passes there. Comet raises the same exception from every task on every version.

What changes are included in this PR?

The test writes each side with .repartition(1), which is how the suite already writes a single file. With one file per side there is no empty file, only one task fails, and Spark raises the bare RuntimeException every time. The assertion keeps Spark's own getCause form from ParquetFieldIdIOSuite. The test comment explains why each side is one file.

branch-1.1 has the same test through #6266 and will need the same change.

How are these changes tested?

The change is to the test itself. The race cannot be forced from the suite, so the fix was checked in two ways:

  • A plain Spark program (no Comet) repeats the test's writes and drains each read partition on its own. On Spark 3.4.3 and 3.5.9 it shows the three-task layout and the two exception shapes above. With .repartition(1) it shows two single-file tasks, and the one that fails raises the bare RuntimeException in 30 of 30 runs on each version.
  • The ParquetReadV1Suite tests with "ids" in their name (7 tests, including this one) pass locally on the default profile (Spark 4.1) and with -Pspark-3.4. On Spark 3.4 the test now writes two files, of 454 and 476 bytes: the same two-row files as the plain Spark program, and no empty file.

The run-all-spark-profiles label is on this PR, so the Comet suites run against Spark 3.4, 3.5, 4.0 and 4.2 before it lands.

The test wrote each side with parallelize over the five-core test
session, which leaves an empty part-00000 per write. The read packs the
two empty files into one task, and on Spark 3.x a file without ids read
after an empty file in the same task has its error wrapped in one more
SparkException. Which of the two failing tasks finished first decided
the error the job reported, and the one-level getCause check failed
when the wrapped one won.

Write each side with repartition(1) so there is one file per side and
a single failing task.
@andygrove andygrove added the run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue label Sep 28, 2026
@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Sep 28, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: Empty fixture files could give Spark 3.4/3.5 failures different exception nesting, making the mixed-field-ID test depend on which task failed first.
  • Design approach: Write each side with .repartition(1), producing one nonempty file per schema.
  • Correctness / compatibility analysis: Compared the relevant writer, reader, scheduler, and field-ID checks against Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources. The change removes the empty-file recursion responsible for the extra wrapper while preserving the missing-ID and null-fill assertions.
  • Key design decisions: Keeps the strict exception-cause assertion and native-plan verification. The existing single-file fixture pattern avoids adding helpers or version-specific branches.
  • Implementation sketch: Adds .repartition(1) before both fixture writes in ParquetReadSuite.scala and documents the failure mechanism.
  • Behavioral changes worth calling out: Adds two shuffle exchanges for two-row test fixtures. Production execution is unchanged, and the expected result remains 100, 200, null, null.
  • Suggested improvements: None at P1/P2 severity. No introduced P1/P2 issues found within this review.

Reviewed full SHA 3eee3a860773b6ba84e58ba7a763b58ab5c831ba against base ce455f32d948355e073638e81009ecc3e5dea349. The complete PR diff from merge-base 786bbc8fd630abfd0b29bdaffea6168f9f228863 contains one changed file and no stacked prerequisites. Confirmed the PR is not a draft. Read AGENTS.md and used review-comet-pr. No sibling skill applies to this test-only change. The snapshot and live discussion contained no reviews, comments, or threads.

Exact-head CI at 2026-09-28 15:39 UTC: 20 checks succeeded, 39 were running, and 33 were skipped. No failures were reported. Scan suites for all supported Spark profiles were still running in the PR workflow and label workflow.

Validation limits: git diff --check passed. No local suite or runtime reproduction was run because this checkout lacks built Comet artifacts and cached Spark dependencies. Runtime validation remains pending CI.

@andygrove
andygrove added this pull request to the merge queue Sep 28, 2026
Merged via the queue into apache:main with commit 65b334b Sep 28, 2026
96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nightly CI failed on 2026-09-28

2 participants