Skip to content

test: cover json_array_length routing configurations - #5940

Open
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:test/json-array-length-routing
Open

peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:test/json-array-length-routing

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #4616.

Rationale for this change

The json_array_length fixtures 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 StaticInvoke rewrite, which does not preserve the expression's implementation tags.

How are these changes tested?

The targeted CometCodegenSuite test and both existing json_array_length SQL 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.

@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

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.

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