Skip to content

test: cover array_intersect routing configurations - #5952

Open
rich7420 wants to merge 1 commit into
apache:mainfrom
rich7420:test/4616-array-intersect-routing-upstream
Open

rich7420 wants to merge 1 commit into
apache:mainfrom
rich7420:test/4616-array-intersect-routing-upstream

Conversation

@rich7420

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #4616.

Rationale for this change

The existing array_intersect fixtures compare results without asserting the selected implementation or covering a disabled dispatcher.

What changes are included in this PR?

Exercise all four combinations of native opt-in and codegen dispatch in the existing fixture. Assert native, dispatched and fallback paths. Include a longer right-hand array to check Spark's left-input ordering on compatible paths, and normalize the permitted order difference for native opt-in.

How are these changes tested?

The updated array_intersect.sql fixture passes through CometSqlFileTestSuite on Spark 4.1.3 / JDK 21 after rebasing onto main. Formatting and Scalastyle passed. The earlier fork CI failure was a macOS hdfsThreadDestructor crash; this base includes #5892, which removes the triggering HDFS suite from CI.

@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

Reviewed 8b1f3b95 against fad62309. This changes one SQL fixture. Previously, the fixture checked results and native operators but did not distinguish a native array_intersect from the JVM codegen dispatcher. The new assertions cover all four combinations:

Native opt-in Codegen dispatcher Expected path
Enabled Enabled Native
Disabled Enabled Dispatched
Disabled Disabled Spark fallback with the disabled-dispatch reason
Enabled Disabled Native

The sequence matches CometArrayIntersect and the existing serde routing. Both configuration keys appear in the fixture header, so withSQLConf restores them after the later SET statements. The harness disables constant folding, executes column inputs against Spark and Comet, and checks that the named expression appears only in the expected implementation set. The fallback assertion also checks Spark-equivalent results and the specific reason.

The added [2,1] / [3,1,2] row is a useful ordering control. Maintained Spark 3.5 and 4.0 scan the left array and emit [2,1]. DataFusion 55.1.0 probes the longer input, producing [1,2] here. The compatible paths compare unsorted results, while the final native-opt-in query sorts the result to allow that documented difference. Sorting does not remove duplicates or nulls. Existing duplicate, null, empty-array, NaN and type cases remain in the fixture. Its two ignored signed-zero queries remain ignored, so this PR does not establish coverage for that known difference.

Validation

The updated fixture passed in the Spark 3.5 expression job and Spark 4.0 expression job. I verified their actual checkout 5ee1bf24, its parents, its full-tree equality with this head, and the native artifact's matching producer/download ID and SHA256. Those jobs reported 1,408 and 1,417 successful tests respectively, with other canceled/ignored tests reported separately. The current snapshot has 41 successful checks and 11 skipped checks. I did not run a local JVM/native build. Maintained Spark 3.4 and 4.1 sources were unavailable and are not claimed as canonical source coverage.

Performance

The implementation and dependency lock are unchanged, so this patch adds no production execution cost. It reuses the existing small table, adds one row and three route-checking queries, and strengthens the first assertion. This avoids running the entire type fixture four times. These are correctness and routing checks, not a speedup benchmark; no new kernel performance claim needs validation in this scope.

Design

Keeping the four states in the existing fixture makes the opt-in precedence and disabled-dispatch behavior explicit. The deliberately different element orders make an unintended native route observable in the compatible cases. Reusing expect_native, expect_dispatch and expect_fallback gives the tests the same contracts as the rest of the suite. No product behavior or default changes.

Abstraction & complexity

The change introduces no new helper, parser feature or test framework. The short sequence of configuration changes is easy to follow, and the native-only ordering normalization is confined to the query that needs it. I found no actionable correctness, performance, design or complexity issue in this diff. Approving.

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.

2 participants