Skip to content

test: guard native plan equality against omitted parameters - #5953

Merged
andygrove merged 1 commit into
apache:mainfrom
rich7420:test/5831-plan-equality-guard-upstream
Sep 16, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
rich7420:test/5831-plan-equality-guard-upstream

Conversation

@rich7420

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Related to #5831; builds on the fixes in #5828.

Rationale for this change

A new constructor field can be omitted from a native operator's handwritten equals, allowing exchange reuse between different plans. A structural guard makes each omission require an explicit decision.

What changes are included in this PR?

Discover native plan classes and check constructor parameters against reads in equals or documented exclusions. Add a negative fixture and register the suite in Linux and macOS CI.

CometBroadcastExchangeExec.mode is explicitly excluded because Comet broadcasts mode-independent Arrow batches. This preserves its current reuse behavior; the guard checks field coverage rather than proving equality semantics.

How are these changes tested?

Both CometPlanEqualitySuite tests pass in the local verification run on Spark 4.1.3 / JDK 21 after rebasing onto main. Formatting and Scalastyle passed. Cross-version fork CI passed before rebasing. The PR contains only the guard and CI registration; #5828's production fixes are already on main.

@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Sep 15, 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.

Correctness

Handwritten plan equality can omit a constructor field and let Spark reuse an exchange between plans with different results. This PR adds a structural guard for concrete CometNativeExec subclasses, plus CometBroadcastExchangeExec and CometUnionExec, and registers it in the Linux and macOS execution suites. It changes no production equality or execution code.

The guard compares reflected primary-constructor parameter names with accessor calls and field reads in the actual declaring class's equals(Object) bytecode. It restricts reads to the operator's class hierarchy, excludes test classes by production code location, rejects empty discovery and stale exclusions, and specifically requires the delegated originalPlan read for native scans and broadcast exchanges. The negative fixture expects exactly the unreferenced omitted parameter, so the check cannot pass simply by reporting no missing fields.

I checked the existing constructors, equality methods and canonicalization against maintained Spark 3.5 and 4.0. Both Spark branches compare canonical plans in sameResult, and exchange reuse keys its cache by the canonical plan. Fields such as join type, null-aware anti-join mode, generator outer, limit offset and scan filters therefore need to preserve their existing identity role. This guard detects a missing read of a non-excluded constructor field. It does not prove that the read affects the comparison, verify hashCode, inspect helper bodies transitively, or detect semantics absent from the constructor. Those limitations are documented, and the existing result-based join and generator reuse regressions remain unchanged.

The exclusions fit the current source. Native scan identity retains a canonicalized originalPlan and compares filters separately, while transient scan plumbing is cleared. Iceberg metadata is deferred execution state, with metadata location, scan hash and runtime filters already compared. The sort/join ordering exclusions retain the existing convention of comparing the inputs from which ordering is derived.

The broadcast mode exclusion is an explicit design decision relative to #5831. Spark builds a mode-specific relation with mode.transform. Comet serializes and broadcasts Arrow batches directly, and its existing canonical equality reduces to the child. The new exclusion documents that existing reuse behavior without changing it or asserting equality between mode-specific Spark relations.

Validation

At the review cutoff, CI had 41 successful checks and 11 skipped, with none failed or running. The original Linux execution-job logs show both new tests passing on Spark 3.4, 3.5, 4.0, 4.1 and 4.2, including Scala 2.12 and 2.13. macOS was skipped, so its registration has source validation only. Other canceled and ignored tests in the execution suites are not credited as coverage.

All five execution jobs and the native builder checked out merge 0d550560, with parents base fad62309 and reviewed head 6eb2bc4e. The entire merge tree equals the head, and all five jobs downloaded the builder's matching native artifact. The new suite itself performs JVM reflection and bytecode inspection, not native query execution. I did not run a local build. Maintained Spark 3.4 and 4.1 branches were unavailable locally, so canonical source comparison is limited to 3.5 and 4.0.

No verified P1/P2 findings.

Performance

There is no production-path overhead. Classpath discovery and bytecode parsing run once in the guard test, without constructing operators or starting a Spark session. The five CI jobs report roughly 1.0–1.2 seconds for that test. These are test durations, not engine performance measurements.

Design

Automatic discovery avoids maintaining a second operator list, while explicit exclusions make new omissions reviewable. Requiring the delegated identity read prevents the scan exclusion list from exempting the whole scan. Keeping this structural check alongside result-based regressions is appropriate because the two detect different failure modes.

Abstraction & complexity

The suite uses two small helpers for constructor reflection and bytecode inspection, with one exclusion map and one negative fixture. It reuses existing dependencies and CI suite lists. The bytecode approach handles the existing handwritten methods without generating arbitrary operator instances or inventing test values for every constructor. I found no actionable simplification.

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

Thanks. The guard will fail a future PR that adds a constructor parameter without comparing it, which is the point, and the self-test plus the stale-exclusion assertion keep it from rotting quietly. Registered in both the Linux and macOS suite lists, so preflight is happy.

@andygrove
andygrove added this pull request to the merge queue Sep 15, 2026
Merged via the queue into apache:main with commit 41d6d44 Sep 16, 2026
52 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 test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants