Skip to content

test: cover collation and predicate routing configurations - #5951

Merged
andygrove merged 2 commits into
apache:mainfrom
rich7420:test/4616-collation-predicate-routing-upstream
Sep 16, 2026
Merged

andygrove merged 2 commits into
apache:mainfrom
rich7420:test/4616-collation-predicate-routing-upstream

Conversation

@rich7420

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #4616.

Rationale for this change

Result comparisons alone do not show whether collated expressions use codegen dispatch or fall back, or whether ordinary inputs still execute natively when the dispatcher is disabled.

What changes are included in this PR?

Add six SQL fixtures for string, collection and predicate routing with the dispatcher enabled and disabled. Cover collated concat/reverse native opt-in with typed Catalyst literals, and strengthen IN/InSet tests across dictionary, ANSI and empty-list settings.

How are these changes tested?

The six SQL fixtures, collated native opt-in test and ordinary IN/InSet test 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 expanded empty-list test was covered there but was not selected in this latest local run.

@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 the full change at 89b5669f against fad62309. I found no P1/P2 issue.

The existing answer comparisons could pass without proving which implementation handled an expression. This PR adds six SQL fixtures that check native execution, JVM codegen dispatch, and fallback with the dispatcher enabled or disabled. The fixtures use Parquet column values, including ordinary, empty, and null inputs. Their assertions compare results with Spark and check the named expression's routing on the executed plan.

The expectations agree with the existing compatibility gates. Collated predicates and string comparisons cannot use the byte-wise native path. Collated concat, reverse, array_join, and str_to_map exercise dispatch or fallback with incompatible native execution disabled. The separate concat/reverse opt-in test disables constant folding and uses typed collated literals. It verifies that opt-in route, rather than broader collated-column or null-kernel coverage.

I compared empty-list behavior with the maintained Spark 3.5 and 4.0 sources. Non-legacy IN ()/InSet returns false even for a null operand, while legacy mode preserves null. Spark 3.5 defaults to legacy behavior, and Spark 4.0's unset setting follows the inverse of ANSI mode. The expanded test checks explicit and unset legacy settings across both ANSI and dispatcher modes, preserves the Catalyst expressions by excluding the relevant optimizer rules, and constructs an empty InSet directly. The ordinary IN test also covers both conversion thresholds and dictionary settings.

Validation

Current-head CI passed. The five expression jobs ran the expanded IN and empty-list tests on Spark 3.4–4.2. The six collation fixtures and literal opt-in test executed successfully on Spark 4.0–4.2. The SQL fixtures' Spark 3.x registrations skip their bodies through the minimum-version gate.

The native build and all five expression jobs checked out merge e15e6c85, whose complete source tree equals the reviewed head. Upload and download receipts report the same native artifact digest. This is CI execution evidence. I did not run a separate local product build or independently hash the extracted shared library. Maintained-source inspection covered Spark 3.5 and 4.0. The other versions are supported here by CI execution, not additional canonical-source inspection. The latest check snapshot contains 41 successful and 11 skipped checks, with none pending or failed.

Performance

The change is confined to tests and adds no production allocations, copying, or execution overhead. The regular IN test doubles its configurations to eight, and the empty-list test doubles to twelve. The fixtures use small inputs and reuse the existing harness. CI completed these tests successfully. This change introduces no new expression or runtime performance claim requiring a microbenchmark.

Design

The enabled/disabled fixture pairs keep identical data and queries while changing the dispatcher setting and expected route. That makes an accidental routing change observable even when answers still match. SQL fixtures suit the column-based cases. Direct Catalyst construction is justified for empty InSet and typed collated literals, where ordinary SQL optimization or unsupported child expressions would obscure the intended routing check. The version gates keep collation syntax away from Spark 3.x.

Abstraction & complexity

The PR reuses the existing answer, routing, fallback-reason, and configuration helpers. It adds no production abstraction or new test framework. Grouping the related routing cases by configuration keeps the setup explicit, and the small loops describe the intended matrices directly. I found no actionable simplification needed before merge.

@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. Pinning the collated predicate and string cases to dispatch is worth having, and the in/inset change in CometExpressionSuite is a real strengthening over the bare checkSparkAnswer that was there.

@andygrove
andygrove added this pull request to the merge queue Sep 15, 2026
Merged via the queue into apache:main with commit edd6d98 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