Skip to content

fix: align string to timestamp parsing with Spark's segment rules - #5682

Open
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:fix/string-to-timestamp-segment-rules
Open

fix: align string to timestamp parsing with Spark's segment rules#5682
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:fix/string-to-timestamp-segment-rules

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5674.

Rationale for this change

Comet's string→timestamp parser is a table of anchored regexes plus an offset-suffix fallback, while Spark's SparkDateTimeUtils.parseTimestampString is a segment scanner with per-segment digit rules (isValidDigits). They disagreed in four ways on the default-on path, for both TIMESTAMP and TIMESTAMP_NTZ:

input Spark Comet before
2020-1-1, 2020-01-01 12:34:5 value (1–2 digit segments) NULL / ANSI error
2020-01-01 12:34:56. value (empty fraction) NULL / ANSI error
2020-10-01Z, 2020-01-01+05:30, 2020-10-01 UTC NULL (zone only after seconds) value
0002020-01-01 00:00:00 NULL (timestamp years ≤ 6 digits) value

What changes are included in this PR?

  • Relax the shape regexes to Spark's isValidDigits rules: year \d{4,6} (only stringToDate, ported by date_parser, allows 7 — CAST(... AS DATE) is unchanged), month/day/hour/minute/second \d{1,2}, fraction \.\d*.
  • Add ends_with_seconds_segment() and only honour a stripped zone suffix when the remainder ends in a seconds or fraction segment, in both timestamp_parser and timestamp_ntz_parser. Any other suffix placement falls through with the unstripped value and is reported as malformed (NULL, or CAST_INVALID_INPUT under ANSI).
  • Previously accepted inputs keep bit-identical values; the common path is unchanged (a direct match still skips suffix extraction; the gate only runs on the fallback path).

Note for #5130 (single-scan classifier): the regex edits are digit-count changes to the same 14 shapes, and the gate is a predicate on the classified shape of the stripped remainder, so it should rebase onto the classifier as a one-line matches! check.

Observed but left out of scope: timestamp_parser (TZ only) still returns NULL for '2021-11-22 10:54:27 +08:00' (space before a bare offset) because it does not trim_end() the remainder the way the NTZ parser does; Spark accepts it. Worth a follow-up issue.

How are these changes tested?

  • Rust: timestamp_parser_spark_segment_rules_test, timestamp_ntz_parser_spark_segment_rules_test and test_cast_string_to_timestamp_spark_segment_rules_array cover every case in the issue table in legacy/try/ANSI modes, regression values for previously accepted shapes (zone forms Z, +05:30, UTC, America/Los_Angeles after a time, negative year), 6-digit valid vs 7-digit invalid years, and date_parser still accepting a 7-digit year. cargo test -p datafusion-comet-spark-expr: 662 passed; cargo clippy --all-targets --workspace -- -D warnings clean.
  • Scala: cast StringType to TimestampType - Spark segment rules and the TimestampNTZType counterpart in CometNativeCastSuite compare against Spark in legacy, try and ANSI modes, with each malformed string in its own query so every ANSI error is checked. CometNativeCastSuite on Spark 4.1.3: 170 passed, 0 failed.

Spark's SparkDateTimeUtils.parseTimestampString validates each segment
with isValidDigits: month, day, hour, minute and second take 1-2 digits,
the fraction after '.' may be empty, a timestamp year takes at most 6
digits (only stringToDate allows 7), and a zone id is only captured when
the scanner is inside the seconds or fraction segment.

Comet's regex table required exactly 2 digits and a non-empty fraction,
allowed 7-digit years, and stripped a zone suffix from any shape, so
'2020-1-1', '2020-01-01 12:34:5' and '2020-01-01 12:34:56.' returned
NULL (or raised under ANSI) while '2020-10-01Z' and
'0002020-01-01 00:00:00' were accepted. Relax the segment quantifiers,
cap the year at 6 digits for timestamp shapes, allow an empty fraction,
and only honour a stripped suffix when the remainder ends in a seconds
or fraction segment, for both TIMESTAMP and TIMESTAMP_NTZ. Previously
accepted inputs keep their exact values; CAST(... AS DATE) keeps its
7-digit years because date_parser is a separate port of stringToDate.

Closes apache#5674

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@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

This change brings the timestamp grammar closer to Spark 3.5 and 4.0. It permits one-digit month, day and time segments and an empty fractional segment, restricts years to four through six digits, and accepts a timezone suffix only after seconds or fractional seconds. The separate DATE parser is unchanged. I compared the TIMESTAMP and TIMESTAMP_NTZ paths, the Legacy/ANSI/Try wrappers, null propagation, timezone handling and the native array dispatch against Spark's parser and cast implementation.

There is one new [P2] correctness issue in the broadened digit classes, detailed inline. A newly accepted non-ASCII digit can pass the regex and then be replaced by a default component when integer parsing fails. That turns malformed column input into a real timestamp, including in ANSI mode, instead of following Spark's rejection semantics. I separated this regression from the older two-digit Unicode behavior and did not report the older behavior as introduced here.

Validation and CI

The focused Rust probe compiled the exact head/base production parser blocks, helper graph, error definitions and Arrow array entry points. It covered 117 main inputs plus separate Unicode and empty/error sets across Legacy, ANSI and Try and both Spark-version flags. The 2,040 main/Unicode evidence assertions and 60 empty/error checks include assertions confirming the reported defect. They are not a claim that every input matched Spark. Column-array checks are important here because literal casts can be folded by Spark before native execution.

At the final CI/discussion refresh, there were 65 successful and nine skipped checks. I also verified the tested merge's exact parents for the inspected Rust and Spark jobs. Those logs report 1,114 Rust tests passed with four skipped, 1,296 Spark 3.5 tests passed with 11 cancelled and 12 ignored, and 1,303 Spark 4.0 tests passed with three cancelled and 12 ignored. No failures were reported in those inspected test summaries. The custom Unicode input was exercised through native Arrow entry points, not through a local Spark/JNI run. I did not run a local full Comet workspace suite or a distributed query.

Performance

The new suffix predicate does not add a heap allocation, but zoned inputs can now be classified once by that predicate and again by the main regex dispatch. I checked this with two alternating-order component benchmark runs using 8,192-row arrays, nulls, repeated kernel calls and nine samples. Ordinary date and ISO cases were roughly unchanged. Zulu-suffix cases measured about 1.16–1.21 times the previous time, and numeric offsets about 1.07–1.10 times. Named-zone results varied more.

These are shared-host parser measurements without CPU pinning, not a Spark-versus-Comet or end-to-end throughput result. They identify the extra classification cost but do not establish a query-level regression. The existing regex-dispatch design remains the main source of repeated work. I did not infer a performance improvement from the expanded correctness coverage.

Design

Keeping the timezone-suffix rule in one predicate makes the intended Spark rule easier to inspect across TIMESTAMP and TIMESTAMP_NTZ. Preserving the separate DATE path also limits the behavioral scope. The year bound, empty-fraction behavior and suffix placement are understandable local changes rather than a new parsing architecture.

The important design invariant is that the shape recognizer and numeric decoder must accept the same alphabet. The inline fix can restore that invariant locally while retaining the intended one-digit support. A broader parser rewrite is not needed to address the demonstrated regression.

Abstraction & complexity

The patch reuses the existing parser table, conversion helpers and cast dispatch rather than adding a new public interface, dependency or configuration. The shared suffix helper represents a concrete rule used by both timestamp paths, so its level of abstraction is appropriate.

The remaining complexity comes from having regex recognition and component decoding as separate stages. I traced both stages instead of treating a regex match as proof that decoding succeeds. The existing fallback defaults make that separation observable for malformed digits, which is why the new grammar needs the ASCII constraint described inline.

Comment thread native/spark-expr/src/conversion_funcs/string.rs Outdated
@peterxcli
peterxcli requested a review from sunchao September 4, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants