fix: string-to-timestamp does not trim ISO control characters, and leading '+' returns null under ANSI - #5172
Draft
peterxcli wants to merge 4 commits into
Conversation
…of double regex matching `timestamp_parser` ORed `is_match` across 14 regex patterns to decide whether a value already matched a base shape, then `timestamp_parser_with_tz` re-matched the same 14 patterns sequentially to dispatch, running up to 28 regex executions per row (21 for the TIMESTAMP_NTZ variant, which matched three separate times). Classify the shape once into a `TimestampPattern` and thread that result through both decisions. ASCII input - effectively all real data - is classified by a single left-to-right byte scan, the same technique already used by the tuned `date_parser`. Non-ASCII input keeps regex semantics (the `\d` class is Unicode-aware, so it accepts non-ASCII digits the byte scan does not) via a single `RegexSet` pass, replacing the sequential scans. Output is unchanged. A differential test compares the classifier against a sequential scan of the original patterns over an exhaustive enumeration of short strings plus every one-character mutation of a set of realistic timestamps.
…tampPattern Addresses review feedback on apache#5130. Move the regex table from two order-coupled const arrays onto `TimestampPattern` itself: `ALL` lists the shapes in match order and `regex_str` returns each shape's regex, so a shape and its pattern are declared together. The lazy `RegexSet` is now built from `ALL.map(regex_str)`. No behavior change - same patterns, same order, and the differential test still fails if the classifier is perturbed. Port the string-to-timestamp cases from Spark's `DateTimeUtilsSuite`: `test("string to timestamp")`, `SPARK-35780` (full range), `SPARK-37326` (`allowTimeZone`), and the `stringToTimestamp` half of `SPARK-15379`, plus a port of the `permuteWithWhitespaceAndControl` helper. Expected values are computed from a proleptic-Gregorian day count rather than from the parser, so they are independent of the code under test and cover the years beyond chrono's range that the `Long.MinValue`/`MaxValue` microsecond boundaries need. Each case is also checked across eval modes and a small set of session timezones, dimensions Spark's suite does not have. All of Spark's expectations hold except two pre-existing divergences, unchanged from main and unrelated to the shape classifier, tracked by apache#5165: - ISO control characters are not trimmed (`str::trim` only strips Unicode whitespace, where Spark strips whitespace and ISO control), so a control-padded timestamp returns null. - A leading '+' that is not a year sign returns null under ANSI where Spark raises. Both are asserted at their current behavior with a pointer to the issue so they cannot silently widen.
andygrove
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Stacked dependency: This PR depends on #5130 by @andygrove. It is based on
e14e086and relies on the classifier and Spark test port introduced there. Please review this PR as the single fix commit on top of #5130; it will be rebased after #5130 merges.Which issue does this PR close?
Closes #5165.
Rationale for this change
Andy's Spark
DateTimeUtilsSuiteport in #5130 exposed two string-to-timestamp differences tracked by #5165: leading and trailing C0/DEL control characters are not trimmed, and malformed leading+inputs return null under ANSI instead of raisingCAST_INVALID_INPUT.What changes are included in this PR?
T-prefixed time-only values.+values through the existing mode-aware invalid-input paths. Legacy and Try still return null; ANSI now raisesCAST_INVALID_INPUT. Valid+<year>-...input remains accepted.How are these changes tested?
cd native && cargo test -p datafusion-comet-spark-expr --lib— 545 passed.cd native && cargo test -p datafusion-comet-spark-expr --lib spark_string_to_timestamp— 4 passed.cd native && cargo fmt --all -- --checkgit diff --check upstream/pr/5130