Conversation
sunchao
left a comment
There was a problem hiding this comment.
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.
Which issue does this PR close?
Part of #4616.
Rationale for this change
The existing
array_intersectfixtures 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.sqlfixture passes throughCometSqlFileTestSuiteon Spark 4.1.3 / JDK 21 after rebasing onto main. Formatting and Scalastyle passed. The earlier fork CI failure was a macOShdfsThreadDestructorcrash; this base includes #5892, which removes the triggering HDFS suite from CI.