Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The existing json_array_length fixtures compare results but cannot distinguish native execution from JVM codegen dispatch. This test adds 12 query/configuration cases covering both opt-in flags, column and literal arguments, NULL, empty arrays, invalid input, and Spark's lenient JSON parsing on the default path.
The matrix matches the current serde: native opt-in selects the named json_array_length scalar function regardless of the dispatcher flag. Without native opt-in, enabling the dispatcher selects CometScalaUDFCodegen, while disabling it requires the function-specific fallback reason. The payload variants belong to a protobuf oneof, so the positive assertions also exclude the opposite execution route. Each query projects only the expression under test. Constant folding is disabled for literals, and the harness rebuilds and executes a separate Spark baseline before comparing results and returning the executed Comet plan.
The maintained Spark 3.5 and 4.0 implementations return a nullable integer, count outer array elements, return NULL for invalid/non-array inputs, and accept the lenient forms tested on the default path. Spark 4.0 replaces the expression with StaticInvoke. Inspecting the executable payload handles the successful rewrite's loss of implementation tags. This PR changes only tests and introduces no new overflow, error-mode or timezone behavior. Maintained Spark 3.4 and 4.1 branches were unavailable locally, so that source comparison remains qualified.
Validation
At the review cutoff, CI had 41 successful checks and 11 skipped, with none failed or running. I read all five expression-job logs. The new routing test and both existing json_array_length fixtures passed on Spark 3.4, 3.5, 4.0, 4.1 and 4.2. Other canceled or ignored tests in those suites are not counted as coverage.
Those jobs executed merge 66b5b7a3, whose parents are base 2d1aab3a and reviewed head e2b28a7e. All 13 reviewed test/implementation files are identical between that merge and the head. The merge adds only three unrelated shuffle-file changes. The native builder and all five consumers report the same artifact digest. I did not run a local JVM/native build.
No verified P1/P2 findings.
Performance
The change adds no production-path work. It reuses one four-row table across the routing matrix, with data and configuration cleanup scoped by existing helpers. The CI timings confirm the test ran but are not a performance benchmark. No material overhead or new performance claim needs a microbenchmark here.
Design
The three outcomes are explicit: native execution, JVM dispatch, and Spark fallback. Answer checks alone would miss an unintended switch between the first two. The payload check closes that gap and has a concrete reason to live in the Scala suite: Spark 4's rewrite prevents the existing tag-based SQL assertions from reliably identifying this route. The existing SQL fixtures continue to cover the broader input cases.
Abstraction & complexity
The test reuses withSubjects, withSQLConf, the answer/fallback helpers, and AQE plan stripping. It introduces no new helper framework or production abstraction. The small matrix and direct inspection of the single projected expression are easy to follow. I found no actionable simplification or additional issue.
Which issue does this PR close?
Part of #4616.
Rationale for this change
The
json_array_lengthfixtures compare results but do not distinguish native execution from JVM codegen dispatch. They also omit fallback coverage when the dispatcher is disabled.What changes are included in this PR?
Add a routing test covering native opt-in and dispatcher settings, column and literal arguments, NULL, empty arrays, and lenient JSON parsing on the default path. Compare results with Spark and check fallback reasons or the native projection's expression payload. Inspecting the payload also covers Spark 4's
StaticInvokerewrite, which does not preserve the expression's implementation tags.How are these changes tested?
The targeted
CometCodegenSuitetest and both existingjson_array_lengthSQL fixtures pass locally on Spark 3.5.9 and 4.1.3 with JDK 17. The new test covers 12 query/configuration combinations.make format(Spark 3.5) and Apache RAT passed.