andygrove commented on PR #5682:
URL:
https://github.com/apache/datafusion-comet/pull/5682#issuecomment-5561675333
I re-probed and re-benchmarked at `89e5ff5cc`. Everything I asked for last
time is in, and the `starts_with('T')` rewrite is better than what I had in
mind. I traced `parseTimestampString` at v4.0.4 and v4.1.3 to check it: once
`getTrimmedStart(bytes) > 0` and `bytes(j) == 'T'`, every branch falls to
`return (Array.empty, None, false)`, so there is no leading-whitespace
T-prefixed input Spark accepts and your predicate is a strict superset of the
four regexes it replaced. I probed `\x01T2:30`, `\x7FT2:30`, `\x0BT2:30` and `"
Tgarbage"` under both version flags and Comet agrees with Spark on all of them.
The `trim_end()` I asked you to add turned out to carry an edge case I had
not thought through when I suggested it. Spark builds the zone string as `new
String(bytes, j, strEndTrimmed - j)` and then resolves it with
`getZoneId(zoneName.trim)`. Java's `String.trim` only strips chars `<= U+0020`,
so Spark keeps `U+00A0` and the other non-ASCII spaces, which makes `ZoneId.of`
throw and the whole cast return NULL, and it strips `0x01` through `0x1F`,
which makes the value parse. Rust's `trim_end` gets both of those backwards. I
ran seven forms against Spark 4.1.3 through Parquet-backed columns and they all
diverge:
| input (`WS` is the character named) | Spark | Comet |
| --- | --- | --- |
| `2020-01-01 12:34:56` `WS=U+00A0` `+08:00` | `null` | `2019-12-31
20:34:56` |
| `2020-01-01 12:34:56` `WS=U+3000` `+08:00` | `null` | `2019-12-31
20:34:56` |
| `2020-01-01 12:34:56` `WS=U+2009` `+08:00` | `null` | `2019-12-31
20:34:56` |
| `2020-01-01 12:34:56` `WS=U+00A0` `UTC` | `null` | `2020-01-01 04:34:56` |
| `2020-01-01 12:34:56` `WS=U+00A0` `Z` | `null` | `2020-01-01 04:34:56` |
| `2020-01-01 12:34:56.123` `WS=U+00A0` `+08:00` | `null` | `2019-12-31
20:34:56.123` |
| `2020-01-01 12:34:56` `WS=0x01` `+08:00` | `2019-12-31 20:34:56` | `null` |
The first six are silent wrong values, and under ANSI Comet returns a row
where Spark raises. They are new here, because before this PR there was no
`trim_end()` on the TIMESTAMP path and all six fell through to NULL. The NTZ
path already had one, so those are older.
I know @sunchao routed the whitespace-set mismatch to #5149, and that is a
fair call in general. The epic does cover the class and the compatibility guide
already documents it for the padded case. What tips it for me here is that the
fix is one line on a line this PR is adding, and that the PR moves the
divergence from a NULL to a wrong value. Would you be willing to take:
```rust
let stripped = stripped.trim_end_matches(|c: char| c <= '\u{20}');
```
at `string.rs:1471`, and the same at the two NTZ sites at `:1823` and
`:1835`? I applied exactly that and all eleven inputs I probed then match
Spark, including `0x7F` staying rejected and `0x0B` and `0x0C` staying
accepted, which is what Spark's split between `getTrimmedEnd` and `String.trim`
produces. 662 spark-expr tests and all 26 `CometNativeCastSuite` timestamp
tests still pass, so it does not disturb anything you added.
While you are in there, `2021-11-22 10:54:27 +08:00` is currently the only
fixture for the whole family that `trim_end()` makes reachable, and there is no
negative case in the same shape. The one I would most want covered is a zone
after a non-seconds segment with whitespace in front, since that is precisely
what `trim_end()` could have broken. `2020-01-01 12:34 UTC` covers the named
form but not the bare-offset form. I ran these against Spark 4.1.3 and all
sixteen pass on your head, so they are free regression coverage:
```scala
// sparkSegmentRuleTimestamps
"2020-01-01 12:34:56 Z",
"2020-01-01 12:34:56\t+08:00",
"2020-01-01 12:34:56\n+08:00",
"2020-01-01 12:34:56.123 +08:00",
"2020-01-01 12:34:56. +08:00",
"2020-01-01 12:34:56 +08:00",
"2020-01-01 12:34:56 -08:00",
"2020-01-01 12:34:56 Europe/Moscow",
"-0001-01-01T12:34:56 +08:00",
"-0001-01-01T12:34:56-08:00",
// sparkSegmentRuleMalformedTimestamps
"2020-01-01 12:34 +08:00",
"2020-01-01 12 +08:00",
"2020-01-01 +08:00",
"2020-01-01 Z",
"2020-01 +08:00",
"2020 +08:00",
```
The last two valid ones also cover the interaction between a leading `-`
year sign and `extract_offset_suffix` picking the rightmost sign, which nothing
tests today.
On the docs, the #5716 bullet landed between the `## Known result-value
divergences` heading and the "The following native paths silently return values
that differ from Spark" paragraph that introduces that section's list, so it
renders as an orphan bullet above its own introduction. Could you move it down
into the list next to the #5149 entry? That entry already describes a
NULL-in-Comet direction, so it fits there.
I re-ran `cast_string_to_timestamp` at your head against the merge base,
three interleaved rounds with run-to-run spread of 1.5% or less on your side.
`canonical` at -36.6% and `microseconds` at -44.1% still match what I measured
at `0a1fc76a0`, but `offset_suffix` is now -18.6% rather than -25%, which makes
sense given `7c449fd9c` added a `trim_end()` and an
`ends_with_seconds_segment()` call to that exact path. Worth updating that one
number so the description matches the head. Two things it undersells, though.
`89e5ff5cc` is a real speedup in its own right: swapping the four anchored
regex matches for `starts_with('T')` gives -43.4% on `spark4_legacy/padded`.
And the only shape that got slower is `date_only_zone` at +2.6% on UTC and
+3.2% on non-UTC, which is the shape that went from returning a wrong value on
main to returning NULL, so the extra `ends_with_seconds_segment` pass is what
buys the correctness fix. That is worth saying explicitly, because it is the
one number a fu
ture reader might otherwise try to optimise away. Related,
`ends_with_seconds_segment` also fixes a silent wrong value the description
does not mention at all: on main `CAST('2020-01-01Z' AS TIMESTAMP)` returns
`2020-01-01 00:00:00`, and it is NULL now.
Two comments in the benchmark drifted out of date. The one above `batches`
lists the shapes starting at `canonical`, but the three new ones went in above
it, so it now describes only part of the array. And the `spark4_legacy` comment
says it measures "the inputs where that check can fire", but the guard needs
leading whitespace and a `T` prefix together, and neither `padded` nor `mixed`
has both, so it measures the check's cost without ever taking its rejection
branch. The cost is the interesting part given it just moved 43%, so it is
worth saying that instead.
Last thing, and not something to hold this PR for. While probing your new
fraction guards I noticed they both return `Ok(None)`, which on the NTZ side
lands on `timestamp_ntz_parser_inner`'s `None => Ok(None)` arm at
`string.rs:1871` and returns NULL even under ANSI. So if a future pattern edit
ever does make those guards reachable, the invariant they exist to protect
comes back as a silently swallowed ANSI error rather than a panic. That
asymmetry is already reachable today through the year-range guard at `:1147`,
where `CAST('294249-01-01' AS TIMESTAMP_NTZ)` under ANSI raises
`CAST_INVALID_INPUT` in Spark and returns NULL in Comet while the TIMESTAMP
version raises correctly. It predates this PR so I have filed it separately as
#5749.
--
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]
