andygrove opened a new issue, #5128: URL: https://github.com/apache/datafusion-comet/issues/5128
## 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::<Tz>()` 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. -- 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]
