Skip to content

test: restore Spark 4.1 Variant shredding suites - #5745

Open
rich7420 wants to merge 2 commits into
apache:mainfrom
rich7420:test/5569-restore-variant-shredding
Open

test: restore Spark 4.1 Variant shredding suites#5745
rich7420 wants to merge 2 commits into
apache:mainfrom
rich7420:test/5569-restore-variant-shredding

Conversation

@rich7420

@rich7420 rich7420 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #5569 (Variant shredding suites).

Rationale for this change

The Spark 4.1.3 patch still excludes both Variant shredding suites for #2209, which was fixed by #4084. Restore their coverage while keeping a focused exclusion for #5741.

What changes are included in this PR?

Remove IgnoreCometSuite from VariantShreddingSuite and ParquetVariantShreddingSuite. Tag only variant logical type annotation - ignore variant annotation with IgnoreComet, linked to #5741. This restores 14 tests with Comet enabled and preserves all 15 with Comet disabled.

Regenerated dev/diffs/4.1.3.diff from Spark v4.1.3 sources. Regeneration also refreshes the existing pom.xml result hash without changing its patch content.

How are these changes tested?

Spark 4.1.3 / JDK 17 fork CI passed, including all seven Spark SQL test groups. The restored suites ran 14 tests successfully, with only the #5741 case ignored. The Hive job was rerun after a runner shutdown.

The regenerated patch applies cleanly to Spark v4.1.3, and the modified Spark sources pass git diff --check.

@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.

Correctness

Reviewed dcc2d4f8 against authoritative base 0b4549fc. The Spark 4.1.3 patch still disabled both Variant shredding suites for the now-closed #2209. This restores their existing assertions and retains one test-level exclusion for the separate annotation-validation issue #5741. The authored change affects only test selection in dev/diffs/4.1.3.diff. It does not implement native Variant support or change scan behavior.

The existing SQLTestUtils.test hook ignores an IgnoreComet tag only when Comet is enabled. Removing the two IgnoreCometSuite mixins therefore restores ordinary registration, while the named annotation test remains ignored in Comet runs. With Comet disabled, that hook delegates to normal registration and preserves all 15 cases. This disabled-mode conclusion is source-derived, not a new execution result.

For semantic context, the maintained Spark 4.0 branch at 03f28fc4 marks shredded Variant projections with per-field VariantMetadata. The unchanged Comet 4.x shim delegates that detection to Spark, and CometScanTypeChecker rejects the marked struct to preserve fallback. The maintained tests exercise null versus missing values, nested arrays/objects, malformed input, cast errors, numeric boundaries and session time zones. Restoring their coverage does not weaken those assertions or alter ANSI/error handling. The maintained Spark 3.5/4.0 integration paths are unchanged. Maintained 3.4/4.1 source branches are unavailable locally, so I do not claim maintained-source coverage for those versions or substitute another Spark checkout.

Validation and CI

Local checks validated both unified patches and confirmed that the other 95 shared patch sections are unchanged after excluding index headers. The POM change is solely its result hash. The authored diff passes git diff --check.

At 2026-09-08 02:00 UTC, checks report 13 successes, 12 skipped and one neutral, with no failures. All seven Spark 4.1.3/JDK 17 test groups and the native/JVM build passed. I verified merge 119d137a, whose parents are exactly the assigned base and head. Its difference from the base equals the authored change. Build logs show application of this patch, and the compiled Spark artifact digest matches the one downloaded by the test jobs.

The Apache CI core-1 log records all eight VariantShreddingSuite tests and six ParquetVariantShreddingSuite tests passing, with only the named #5741 case ignored. These are Comet-enabled Spark test results, including existing fallback behavior. They do not prove native Variant execution or that #5741 is fixed. No local Spark/JNI rebuild or execution was performed. No P1/P2 correctness finding.

Performance

This adds 14 existing tests back to CI and introduces no runtime allocation, copying or scan overhead. The test-registration hook is unchanged, and the new tag is used only during registration. A query microbenchmark would not validate this test-selection change. The passing suite timings are test-run evidence, not a Comet-versus-Spark performance comparison.

Design

Replacing broad suite exclusions with one named, issue-linked exclusion restores useful regression coverage while keeping the known error-policy mismatch visible. It also follows the existing mechanism used elsewhere in this patch. The remaining exclusion is scoped to the test containing the failing annotation check, and all its assertions remain available when Comet is disabled. No further design change is needed for this PR.

Abstraction & complexity

The change removes two mixins and reuses the existing IgnoreComet tag and registration hook. It adds no helper hierarchy, custom runner or special configuration path. The issue link makes the remaining exclusion straightforward to revisit. No actionable complexity concern or other verified P1/P2 finding. Approving this revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants