andygrove opened a new pull request, #5130:
URL: https://github.com/apache/datafusion-comet/pull/5130
## Which issue does this PR close?
Part of #5128 (first "High impact" item: string to timestamp / timestamp_ntz
double regex matching).
## Rationale for this change
Optimize existing expression.
`timestamp_parser` ORed `is_match` across 14 regex patterns just to decide
whether the value
already matched a base shape (so that an offset suffix must not be
stripped), and then
`timestamp_parser_with_tz` re-matched the same 14 patterns sequentially to
pick the parse
routine — up to 28 regex executions per row. `timestamp_ntz_parser` matched
three separate
times (7 time-only + 7 date + 7 in the inner parse), for up to 21 per row.
## What changes are included in this PR?
Classify the input shape **once** into a `TimestampPattern` and thread that
single result
through every decision that previously re-matched.
ASCII input — effectively all real data — is classified by one left-to-right
byte scan, the
same technique as the already-tuned `date_parser`. The 14 patterns are
digit-count and
separator shapes, so the scan is a direct transcription of them, including
the two
asymmetries that are easy to lose: a bare year takes 4-6 digits while a
separated year takes
4-7, and a trailing hour takes 1-2 digits while an hour followed by a minute
must be exactly 2.
Non-ASCII input keeps regex semantics rather than the byte scan: the `regex`
crate's `\d` is
Unicode-aware, so the patterns accept non-ASCII digits (e.g. Arabic-Indic)
that an ASCII scan
does not. Those inputs go through a single `RegexSet` pass instead of the
sequential scans, so
that path gets faster too without any behavior change.
Also removes the seven single-line `parse_str_to_*_timestamp` wrappers,
which existed only to
be function pointers in the pattern table.
## How are these changes tested?
Existing tests — the full `datafusion-comet-spark-expr` suite (540 tests) and
`CometCastSuite` (162 succeeded, 0 failed, 9 ignored, on the Spark 4.1
profile so the
`is_spark4_plus` leading-whitespace path is covered).
Two new tests pin the classifier:
- `test_timestamp_pattern_classifier_matches_regexes` — a differential test
comparing the
classifier against a sequential scan of the original patterns over an
exhaustive
enumeration of every string of up to 5 shape characters (`019-:.T +`),
plus every
one-character deletion, substitution, and insertion applied to nine
realistic timestamp
strings, plus non-ASCII inputs. Verified to fail if the classifier is
perturbed (widening
the bare-year digit range to 4-7 makes it report `"0202001"`).
- `test_timestamp_pattern_digit_count_asymmetries` — readable anchors for
the digit-count
asymmetries above.
### Benchmark
New benchmark `native/spark-expr/benches/cast_string_to_timestamp.rs` (no
coverage existed
for this cast). Baseline captured on unmodified source. Every shape
improved; nothing
regressed.
| shape | main | this PR | change |
| --- | --- | --- | --- |
| `timestamp/canonical` | 1830.8 µs | 483.4 µs | **-73.6%** (3.79x) |
| `timestamp/microseconds` | 2238.4 µs | 576.9 µs | **-74.2%** (3.88x) |
| `timestamp/date_only` | 890.0 µs | 334.0 µs | **-62.5%** (2.66x) |
| `timestamp/time_only` | 2271.3 µs | 1198.2 µs | **-47.2%** (1.90x) |
| `timestamp/iso_z_suffix` | 2896.2 µs | 1127.8 µs | **-61.1%** (2.57x) |
| `timestamp/named_tz_suffix` | 4380.9 µs | 2574.8 µs | **-41.2%** (1.70x) |
| `timestamp/invalid` | 2566.4 µs | 993.8 µs | **-61.3%** (2.58x) |
| `timestamp/sparse_nulls` | 1740.7 µs | 465.2 µs | **-73.3%** (3.74x) |
| `timestamp/dense_nulls` | 203.2 µs | 68.4 µs | **-66.3%** (2.97x) |
| `timestamp/non_ascii_digits` | 1045.1 µs | 709.6 µs | **-32.1%** (1.47x) |
| `timestamp/mixed` | 1805.1 µs | 598.3 µs | **-66.9%** (3.02x) |
| `timestamp_named_zone/canonical` | 2082.2 µs | 722.4 µs | **-65.3%**
(2.88x) |
| `timestamp_ntz/canonical` | 2271.3 µs | 406.6 µs | **-82.1%** (5.59x) |
| `timestamp_ntz/microseconds` | 2692.6 µs | 508.0 µs | **-81.1%** (5.30x) |
| `timestamp_ntz/date_only` | 1352.8 µs | 263.3 µs | **-80.5%** (5.14x) |
| `timestamp_ntz/iso_z_suffix` | 2838.5 µs | 1065.2 µs | **-62.5%** (2.66x) |
| `timestamp_ntz/invalid` | 2096.8 µs | 975.1 µs | **-53.5%** (2.15x) |
| `timestamp_ntz/dense_nulls` | 247.8 µs | 61.4 µs | **-75.2%** (4.03x) |
| `timestamp_ntz/mixed` | 2045.5 µs | 530.7 µs | **-74.1%** (3.85x) |
All 19 shapes reported `Performance has improved.` at p = 0.00.
## Are there any user-facing changes?
No. Output is bit-identical: same values, same null buffer, same ANSI error
text.
--
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]
