fix: align string to timestamp parsing with Spark's segment rules - #5682
fix: align string to timestamp parsing with Spark's segment rules#5682peterxcli wants to merge 2 commits into
Conversation
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
left a comment
There was a problem hiding this comment.
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.
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.parseTimestampStringis a segment scanner with per-segment digit rules (isValidDigits). They disagreed in four ways on the default-on path, for bothTIMESTAMPandTIMESTAMP_NTZ:2020-1-1,2020-01-01 12:34:52020-01-01 12:34:56.2020-10-01Z,2020-01-01+05:30,2020-10-01 UTC0002020-01-01 00:00:00What changes are included in this PR?
isValidDigitsrules: year\d{4,6}(onlystringToDate, ported bydate_parser, allows 7 —CAST(... AS DATE)is unchanged), month/day/hour/minute/second\d{1,2}, fraction\.\d*.ends_with_seconds_segment()and only honour a stripped zone suffix when the remainder ends in a seconds or fraction segment, in bothtimestamp_parserandtimestamp_ntz_parser. Any other suffix placement falls through with the unstripped value and is reported as malformed (NULL, orCAST_INVALID_INPUTunder ANSI).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 nottrim_end()the remainder the way the NTZ parser does; Spark accepts it. Worth a follow-up issue.How are these changes tested?
timestamp_parser_spark_segment_rules_test,timestamp_ntz_parser_spark_segment_rules_testandtest_cast_string_to_timestamp_spark_segment_rules_arraycover every case in the issue table in legacy/try/ANSI modes, regression values for previously accepted shapes (zone formsZ,+05:30,UTC,America/Los_Angelesafter a time, negative year), 6-digit valid vs 7-digit invalid years, anddate_parserstill accepting a 7-digit year.cargo test -p datafusion-comet-spark-expr: 662 passed;cargo clippy --all-targets --workspace -- -D warningsclean.cast StringType to TimestampType - Spark segment rulesand theTimestampNTZTypecounterpart inCometNativeCastSuitecompare against Spark in legacy, try and ANSI modes, with each malformed string in its own query so every ANSI error is checked.CometNativeCastSuiteon Spark 4.1.3: 170 passed, 0 failed.