andygrove commented on PR #5682:
URL:
https://github.com/apache/datafusion-comet/pull/5682#issuecomment-5552876461
I dug into this one because the regex edits looked like they might cost
something, and it turns out the opposite is true. Benchmarking
`cast_string_to_timestamp` at 81d637b9b against 0a1fc76a0, the cast gets a lot
faster: -37% on the `canonical` shape, -44% on `microseconds`, -25% on
`offset_suffix`, and comparable numbers across the NTZ and non-UTC groups. I
did a base-vs-base run first to get a noise floor and those two shapes sit
under 1%, so it isn't measurement noise. The cause is the `\d` to `[0-9]` swap
rather than the segment rules, since `regex` comes in with default features and
`\d` compiles to the Unicode `Nd` class, which is much more expensive to match
than one ASCII range. Could you add that to the description? As written this
reads as a pure strictness change, and the next person to touch these patterns
has no way to know the ASCII classes are load-bearing for throughput as well as
correctness. It would also be worth adding cases to
`cast_string_to_timestamp.rs` fo
r the shapes you have made reachable, since none of the six existing ones use
1-digit segments, an empty fraction, or a zone after a date-only value.
The description also undersells the fix. On main, `CAST('2020-10-1' AS
TIMESTAMP)` returns 2020-10-01 01:00:00 in a UTC session rather than NULL,
because `RE_DAY` needed a 2-digit day, so `extract_offset_suffix` read the
trailing `-1` as a -01:00 offset and `RE_MONTH` then matched the remaining
`2020-10`. `2020-12-1` does the same thing. Those are silently wrong values
rather than NULLs, which makes them the most valuable regression tests in the
whole set, and neither one is in `SPARK_SEGMENT_RULE_VALID` or
`sparkSegmentRuleTimestamps`. Every valid case you listed is a 1-digit-month
shape, and those only ever returned NULL. Could you add the 2-digit-month with
1-digit-day shape to both corpora? While you are in there, neither Scala list
has a negative-year case, valid or malformed, even though `-?[0-9]{4,6}` is one
of the edited patterns and the Rust lists do cover it.
Related, and also worth putting in the description: on main
`CAST('2020-01-01 12:34:56.1٢٢٢' AS TIMESTAMP)` panics with "end byte index 6
is not a char boundary", because Unicode `\d` let `RE_MICROSECOND` match a
fraction of multi-byte digits and `parse_to_timestamp_info` then byte-slices a
`&str`. `T1:2:3.1٢٢٢` reaches the same bug through
`parse_str_to_time_only_timestamp`. Your change makes both unreachable, which
is a bigger deal than stricter parsing. Since those two slices are now safe
only because no pattern can hand them non-ASCII, and that invariant lives a few
hundred lines away at the regex definitions, could we make them
char-boundary-safe on their own so that a future pattern edit cannot resurrect
the panic?
On the `'2021-11-22 10:54:27 +08:00'` case you left out of scope, I think it
belongs in this PR. Adding `let stripped = stripped.trim_end();` before the
`ends_with_seconds_segment` call in `timestamp_parser` returns the Spark value,
leaves `2020-10-01Z` at NULL, and passes all 662 spark-expr tests. It is the
same `trim_end()` that `timestamp_ntz_parser` already applies. As it stands the
two paths return different things for that input, a NULL and a value, which
cuts against the point of giving them one shared zone rule. If you would rather
keep it separate that is fine, but then it needs a filed issue and a bullet
under "Known result-value divergences" in
`docs/source/user-guide/latest/compatibility/index.md`, next to the #5149
whitespace entry, or it will not get picked up.
For what it is worth, I checked `parseTimestampString` and `isValidDigits`
at v3.4.4, v3.5.9, v4.0.1, v4.1.1 and master, and all five agree on every rule
you encoded, including the timestamp year bound of 6 against 7 for
`stringToDate`, the empty fraction, and the zone id only being captured inside
the seconds or fraction segment. No compatibility objection from me on the
behavior itself.
Last thing, and I think it is the real story here. Your note about the fuzz
alphabet is the important one: `timestampPattern` being `"0123456789/:T" +
whitespaceChars` with no `-`, `.` or `+` means it cannot generate a plausible
date, which is why this whole family of mismatches sat behind a passing fuzz
test. Any appetite for widening it? If it surfaces more mismatches, those are
issues to file rather than a reason to leave the alphabet alone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
