andygrove commented on PR #5150:
URL:
https://github.com/apache/datafusion-comet/pull/5150#issuecomment-5145951681
Thanks @mbutrovich @comphead — all of this is addressed in 434efec. Point by
point:
**Verbose docs (@comphead, @mbutrovich).** The `trim.rs` module doc keeps
the regime table and the footnote; both paragraphs that restated it are gone,
replaced by one line. `is_whitespace_or_iso_control`, `trim_all` and
`trim_java_string` are now pointers at that table rather than re-listing the
byte ranges and cast targets, so the mapping has one place to go stale. I did
keep the two things that aren't derivable from the code: the signed-byte
widening note, and the JDK call paths that justify the `String.trim` regime
(`Double.parseDouble` / `Decimal.stringToJavaBigDecimal`) — the latter moved up
into the module doc so it isn't duplicated in a function doc. Same treatment
for `trimPadding`'s Scaladoc in `CometCastSuite`, which now points at
`conversion_funcs::trim` instead of restating the regimes in Scala.
**`is_java_trim_byte` docstring (@comphead).** You're right, and the correct
invariant is the reverse direction. Reworded to: "In valid UTF-8 a byte `<=
0x20` is always a single-byte codepoint, since the lead and continuation bytes
of a multi-byte sequence are all `>= 0x80`."
**`to_time` / `try_to_time` (@mbutrovich).** Added to the not-yet-migrated
footnote in `trim.rs`, so the boundary is in the code and not just in the PR
description. I also added them to the user-facing divergence bullet in
`compatibility/index.md`, which was only naming timestamp.
**Bench duplication (@mbutrovich).** New `benches/common/mod.rs`, pulled in
with `#[path]` from all three cast-from-string benches. It lives in a
subdirectory so Cargo's bench auto-discovery (which only scans `benches/*.rs`)
doesn't try to build it as a target. One parameterized `string_batch(rows,
null_modulus, value_fn)` replaces the `create_batch` copy shared by
`cast_string_to_date.rs` / `cast_string_to_timestamp.rs` **and** all five
near-copies in `cast_from_string.rs` — that file is 185 lines lighter.
`EVAL_MODES` is hoisted to a single const and used at all three sites. All
three benches smoke-tested with `--test`.
**Pure-padding / empty input (@comphead).** Both `assert_trim_parity` (Rust)
and `trimPaddedValues` (Scala) now generate `input == pad` and `input == ""` in
addition to the seeded cases, so the empty-slice / `j == str_end_trimmed`
branches are hit directly rather than incidentally.
**Timestamp divergence untested (@comphead).** Agreed this was a real gap,
but an `ignore`d Scala test wouldn't close it, and `castTest` can't either —
Spark is the oracle there and Comet doesn't fall back, it silently returns a
value, so the test would just fail. Pinned it as a Rust test instead
(`test_cast_string_to_timestamp_unicode_whitespace_divergence`), asserting the
*current* divergent behaviour for `U+0085` / `U+00A0` / `U+2028` / `U+3000`
across both timestamp and timestamp_ntz. The failure message says what to do
when it flips: delete the test and extend `assert_trim_parity` to the timestamp
targets. That makes #5149's remaining work impossible to land silently.
**SQL file tests (@comphead).** Added
`expressions/cast/cast_string_trim.sql`. The padding is materialized into the
Parquet table via `chr()` / `decode(X'…', 'utf-8')` at insert time so the file
stays pure ASCII and the cast operands stay inside expressions Comet runs
natively — the coverage assertion confirms they do. It covers both regimes (DEL
trimmed for boolean/integral/date but not float/double/decimal, `U+00A0` /
`U+3000` trimmed for nothing) plus the padding-only and interior-padding rows,
across the dictionary on/off matrix.
**Divergence in both directions (@comphead).** The description already
covers this ("The divergence went in both directions: spurious failures where
Spark parses fine, and silent wrong results…") with the four-line SQL repro, so
I left it as is — happy to expand if you'd like the `UTF8String.java:983-999`
reference in there explicitly.
Verified: full `CometCastSuite` (165 passed), the new SQL fixture on both
dictionary settings, all 554 `datafusion-comet-spark-expr` unit tests, and
`cargo clippy --all-targets -D warnings` clean, on Spark 3.5.
--
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]
