Skip to content

test: cover datetime and timezone routing configurations - #5950

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

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

Conversation

@rich7420

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #4616.

Rationale for this change

Datetime tests can match Spark while taking a different implementation path. They need to distinguish native execution, codegen dispatch and fallback as configuration and input types change.

What changes are included in this PR?

Add eight SQL fixtures covering dispatcher settings, native opt-in, timezone conversion and collated datetime arguments. Strengthen existing date_trunc and date_format tests with expression-level routing assertions, including non-UTC timezones.

How are these changes tested?

The eight fixtures in CometSqlFileTestSuite and CometTemporalExpressionSuite pass in the local verification run on Spark 4.1.3 / JDK 21 after rebasing onto main. Native code was rebuilt with Rust 1.98.1; formatting and Scalastyle passed. The existing incompatible non-UTC date_format opt-in case checks native execution without claiming Spark result parity.

@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

Existing datetime tests can return the right values while silently switching between a native kernel and generated JVM code. This PR adds eight SQL fixtures that pin those routes across dispatcher settings, native opt-in and collated arguments. It also strengthens five existing date_trunc and date_format tests with expression-level assertions.

The fixtures use nullable Parquet columns, and the SQL harness disables constant folding. Each native or dispatch assertion checks Spark result parity, Comet operator coverage, presence in the expected expression set and absence from the other set. Fallback queries compare results and require an expression-specific disabled-dispatcher reason. The opt-in fixtures keep unsupported formats on fallback even when incompatible native behavior is enabled.

I compared the relevant implementations with maintained Spark 3.5 and 4.0 sources. Spark distinguishes date truncation from timestamp truncation, returns NULL for invalid truncation units, and applies the session timezone to timestamp truncation and formatting. convert_timezone consumes and returns TIMESTAMP_NTZ. Spark 4.0 accepts collated string arguments in the tested functions. The version gates correctly restrict timezone conversion to 3.5+ and collation syntax to 4.0+.

The coverage is deliberately bounded. These new SQL fixtures run with ANSI disabled and ordinary dates. They do not establish extreme-range, invalid-timezone or ANSI-error compatibility. Native opt-in truncation with a column format filters out NULL formats, consistent with the existing documented incompatibility, while dispatch fixtures retain nullable formats. The non-UTC date_format opt-in case checks execution and native routing only because result divergence is already documented. The non-UTC date_trunc case retains its Spark comparison on the existing bounded 2024 data.

Validation

At the review cutoff, CI had 41 successful checks and 11 skipped, with none failed or running. I read the expression logs for Spark 3.4, 3.5, 4.0, 4.1 and 4.2. They show the temporal fixtures and strengthened Scala tests passing. Timezone fixtures execute on 3.5+, and collation fixtures execute on 4.0+. The harness reports version-gated fixtures as successful test entries, so those entries are not credited as execution on older versions. Other canceled and ignored suite tests are also excluded from the coverage claim.

All five expression jobs and the native builder checked out merge b932ccc8, with parents base fad62309 and reviewed head 1ffb9d0e. Its entire tree equals the head. All five consumers report the builder's matching native artifact digest. I did not run a local JVM/native build. Maintained Spark 3.4 and 4.1 branches were unavailable locally, so canonical source comparison is limited to 3.5 and 4.0, separately from CI runtime evidence.

No verified P1/P2 findings.

Performance

The diff changes tests only. Each new fixture creates two rows and checks a small set of routes. The Scala changes reuse existing data and execution, adding plan assertions rather than another runtime pass. There is no production overhead or new performance claim requiring a microbenchmark.

Design

The three configurations make the routing contract explicit: compatible inputs remain native, incompatible inputs dispatch by default, and explicit opt-in selects the native implementation where one exists. Unsupported formats still fall back with dispatch disabled. Keeping the existing generated-data Scala cases preserves broader non-UTC coverage while the SQL fixtures make the configuration boundaries easy to inspect.

Abstraction & complexity

The change reuses the SQL parser, scoped table/config cleanup and existing expression-inspection helpers. Grouping related functions by routing configuration shares setup and makes the paired expectations visible. Separate fixtures are reasonable because their expected routes differ. No new framework or production abstraction is introduced, and I found no actionable simplification.

@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. The disabled/enabled/opt-in triple makes the routing explicit for each temporal expression, and tightening the existing CometTemporalExpressionSuite cases from checkSparkAnswerAndOperator to checkSparkAnswerAndImpl is the more valuable half of this.

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