apache / apache/datafusion-comet
[EPIC] Optimize native cast expression kernels
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## What is the problem the feature request solves?
A review of the native cast kernels in `native/spark-expr/src/conversion_funcs/` found a number of remaining per-row costs: element-wise builder loops that could be vectorized Arrow passes, batch-constant work recomputed per row, per-row heap allocations, and redundant regex matching. This epic tracks optimizing them, following the methodology in `docs/source/contributor-guide/optimizing_expressions.md` (baseline first, bit-identical output, no regression on any benchmark shape).
### Already tuned (do not re-tread)
- `spark_cast_int_to_int` narrowing casts (#4920)
- float/double to decimal (#4940)
- `parse_string_to_decimal` (#4916)
- `date_parser` / string to date (#4917)
- `spark_cast_float64_to_utf8` (#4918)
- `cast_decimal128_to_utf8` (#4924)
- `cast_binary_to_string` (#4763, #4912)
### High impact
- [ ] **String to timestamp / timestamp_ntz: eliminate double regex matching.** `timestamp_parser` (`string.rs`) ORs `is_match` across 14 patterns to detect a direct match, then `timestamp_parser_with_tz` re-matches the same patterns sequentially to dispatch, up to ~28 regex executions per row (~21 for the NTZ variant). A single `regex::RegexSet` pass returning the matched index, reused for both decisions, removes most of this; a byte classifier (the patterns are digit-count/separator shapes, like the tuned `date_parser`) would go further. No benchmark exists for string to timestamp; one must be added first. Traps: first-match-wins pattern order, `RE_YEAR` accepts 4-6 digits while the others accept 4-7, ANSI errors must carry the raw untrimmed value.
- [ ] **Decimal to int: hoist the per-row `10^scale` and vectorize.** The `cast_decimal_to_int*` macros (`numeric.rs`) evaluate `10_i128.pow(scale)` inside the per-row closure in every arm, then collect element by element. Hoist the divisor, then use `unary` for Legacy/Try (both wrap via `as`) and `try_unary` for ANSI. Traps: ANSI error text formats the value via `format_decimal_str` with a `BD` suffix; int8/int16 targets truncate through i32 first. No benchmark exists.
- [ ] **Int to decimal: batch-level infallibility check plus vectorized pass.** `cast_int_to_decimal128_internal` (`numeric.rs`) is a per-element builder loop, but overflow feasibility is decidable once per batch from `T::MIN/MAX * multiplier` against the target precision. In the common case the whole batch is one infallible `unary` multiply carrying the null buffer; otherwise fall back to the `unary_opt` plus O(1) null-count ANSI pattern from #4940. Traps: the ANSI error value string is the unscaled input; Legacy and Try both produce null on overflow. No benchmark exists.
- [ ] **Date/timestamp casts: fixed-offset timezone fast path.** `cast_date_to_timestamp` (`temporal.rs`) calls `resolve_local_datetime` per row; for constant-offset zones (`TzInner::Offset`, including UTC) this collapses to `d * MICROS_PER_DAY - offset_us` in one `unary` pass. The same hoist applies to timestamp to date via `pre_timestamp_cast` (`utils.rs`), which also makes a redundant second full pass through the Arrow cast kernel. The NTZ branch of `cast_date_to_timestamp` is a builder loop for a bare multiply. Traps: `unary` closures execute on garbage under null slots, so use `wrapping_mul` and keep unclamped values out of chrono (it panics out of range); DST resolution semantics (earlier occurrence on ambiguity, pre-transition offset on gap) must be preserved for named zones. No benchmarks exist for either direction, and existing temporal benches only cover UTC.
### Medium impact
- [ ] **`remove_trailing_zeroes` runs on every timestamp-to-string batch** (`utils.rs`) and rebuilds the whole array through a per-row Option collect. Trimming is pure ASCII suffix removal computable from the offsets buffer, with a zero-copy no-op fast path when nothing shrinks. Trap: preserve the current quirk that a fully-trimmed fraction keeps the trailing dot.
- [ ] **String to boolean allocates per row.** `spark_cast_utf8_to_boolean` (`string.rs`) calls `value.to_ascii_lowercase()` per row just to compare against 10 literals; `eq_ignore_ascii_case` after `trim` is allocation-free and bit-identical. Trap: the ANSI error must keep the original untrimmed value.
- [ ] **Timezone re-parsed and cloned every batch.** `cast_array` clones the timezone `String` and downstream helpers re-run `tz.parse::()` per batch (acknowledged TODO on `SparkCastOptions`). Cache a parsed `Tz` lazily. Trap: `test_cast_invalid_timezone` expects the parse error to surface from `cast_array`, so parsing must stay lazy.
- [ ] **Float to int macros are element-wise collects** (`numeric.rs`). Non-ANSI arms are infallible (`as` saturates, NaN maps to 0) and become `unary`; ANSI becomes `try_unary`. Trap: keep the saturation-based overflow detection exactly as written.
- [ ] **`Z`/fixed-offset timestamp suffixes rebuild a `Tz` per row.** `tz_from_offset_secs` (`string.rs`) does `format!` plus `Tz::from_str` for every suffix-bearing row; statics for UTC and the `SHORT_IDS` offsets, or a small memo, fix the very common ISO-8601 `Z` shape.
### Low impact / trivial cleanups
- [ ] Identity cast wraps the array in a nested Arc: `Ok(Arc::new(array))` where `array` is already an `ArrayRef` (`cast.rs`), adding an allocation and permanent double indirection on the schema-adaptation path. One-line fix.
- [ ] `dict_from_values` (`cast.rs`) builds keys `0..len` through two collects with per-row Result/Option wrapping; an iota `ScalarBuffer` plus the cloned input null buffer suffices.
- [ ] Builder-loop to `unary` cleanups, all the same shape: int to timestamp, bool to timestamp, bool to decimal, float to timestamp (`unary_opt`), decimal to boolean.
- [ ] Decimal to timestamp does i256 multiply and divide per row (`numeric.rs`); choosing mul by `10^(6-scale)` or div by `10^(scale-6)` once per batch stays in i128. Trap: preserve truncation-toward-zero and wrapping `as i64` semantics.
- [ ] Array/struct to string (`cast.rs`) assembles each row in a scratch `String` then copies it again via `append_value`; the builder implements `fmt::Write`, so writing directly into it halves the byte traffic. Trap: append-null must happen before any bytes are written for a row.
- [ ] `write_i8` invokes the full `fmt` machinery per byte in Spark 4 pretty-print binary styles (`cast.rs`); a 256-entry static string table replaces it.
### Benchmark gaps
No criterion coverage exists today for: string to timestamp (both variants), decimal to int, int to decimal, float to int, date to timestamp, timestamp to date, or any non-UTC timezone shape. Per the no-regression gate, each optimization needs its benchmark added and a `main` baseline captured before the change, covering no-null/dense-null, valid/invalid, and (for temporal work) fixed-offset and named-DST zones.
### Bookkeeping
- [ ] The audit page `docs/source/contributor-guide/expression-audits/conversion_funcs.md` records only #4920 and #4940; add the missing `Performance (tuned ...)` lines for #4912, #4916, #4917, #4918, and #4924.
## Describe the potential solution
Work through the checklist above, one focused PR per item, each following `docs/source/contributor-guide/optimizing_expressions.md`: benchmark first, keep output bit-identical (values, null buffer, error text), and submit only with no meaningful regression on any shape. Record each merged tuning in the expression audit page.
## Additional context
Findings come from a per-row cost review of `cast.rs`, `numeric.rs`, `string.rs`, `boolean.rs`, `temporal.rs`, and `utils.rs` under `native/spark-expr/src/conversion_funcs/`, scaffolded by the repo's `optimize-comet-expression` skill.
Contributor guide
Research direction
Start with docs/source/contributor-guide/optimizing_expressions.md and the relevant checklist item in native/spark-expr/src/conversion_funcs/, then inspect the named Rust helper and existing benchmarks. Capture a main baseline before changing one focused item, preserve bit-identical values, null buffers, and error text, and add coverage for the listed null, validity, and timezone shapes. Done means no meaningful benchmark regression and an entry in docs/source/contributor-guide/expression-audits/conversion_funcs.md.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design, performance, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100