andygrove opened a new issue, #5104: URL: https://github.com/apache/datafusion-comet/issues/5104
### What is the problem the feature request solves? This epic tracks the follow-ups from an audit of the native Rust code (spark-expr, core, shuffle, common) looking for hand-rolled logic that replicates existing arrow-rs compute kernels and could be replaced by calling the kernel. The workspace is on arrow 58.4.0; some of the duplicated code predates public availability of the arrow API it copies. Every candidate was judged for Spark-semantics equivalence before being filed: much of Comet's native code intentionally diverges from arrow for Spark compatibility (ANSI errors, HALF_UP rounding, Java string formats, timezone DST rules, byte-format compatibility), and those cases are listed at the bottom as verified-intentional rather than filed as issues. Kernel equivalences below were verified against the vendored arrow 58.4.0 sources where noted in the individual issues. ### Delete duplicated arrow code (zero risk) - [ ] #5088 Remove `timezone.rs` copy of arrow-array `Tz` (now public in arrow 58; the crate currently carries two parallel `Tz` types) - [ ] #5089 Remove `is_valid_decimal_precision` copy from `utils.rs` (its own comment says to remove it once arrow-rs #6419 ships, which it has) ### Drop-in kernel replacements - [ ] #5090 Use arrow `cast` for hand-rolled temporal unit conversion loops (micros to millis, Date32 to TimestampNtz, millis to micros, Int32 to Date32) - [ ] #5091 Replace small hand-rolled element loops with arrow kernels and arity helpers (decimal to boolean via `neq`, validity masks via `is_not_null`/`and`, `days_to_date`, pow/make_decimal arity, covariance null masking, columnar_to_row decimal reinterpret) ### Arrow kernel core plus thin Spark wrapper - [ ] #5092 checked_arithmetic: delegate ANSI integer arithmetic to arrow checked kernels and the Datum path - [ ] #5093 negative: ANSI overflow check reads null slots and can raise spurious overflow errors (fixed by switching to arrow's checked `neg`) - [ ] #5094 decimal_rescale_check: replace fused rescale and precision check with arrow decimal cast - [ ] #5095 Delegate int/float/boolean to decimal cast arms to arrow safe cast (algorithms verified line-for-line identical) - [ ] #5096 Use arrow dictionary casts instead of hand-rolled dictionary handling in cast paths - [ ] #5097 cast_map_to_map drops entries null buffer and ignores target sorted flag (fixed by arrow's map cast for the rename-only case) - [ ] #5098 sum_int: use arrow sum kernels and collapse integer type dispatch - [ ] #5099 size: compute list sizes with the arrow `length` kernel - [ ] #5100 list_extract: replace per-row MutableArrayData gather with `take` and `zip` - [ ] #5101 Use arrow `make_comparator` for nested structural equality in arrays_overlap and array_position - [ ] #5102 rlike: consider arrow `regexp_is_match` kernel and fix non-StringArray panic ### Upstream DataFusion consolidation (secondary) - [ ] #5103 Consolidate date/timestamp truncation and xxhash64 with upstream DataFusion / datafusion-spark implementations ### What the audit verified as intentional (no action needed) - **Cast string parsing and formatting** (`conversion_funcs/string.rs`, float/decimal to string): ports of `UTF8String.toInt`, `stringToDate`, `stringToTimestamp`, Java `Double.toString` and `BigDecimal.toString` semantics; `arrow_cast::parse`/display are not Spark-compatible for any of them. - **Legacy wrap-around int casts, seconds-based numeric to timestamp, NTZ/TZ timestamp handling**: arrow either has no wrapping mode, interprets values in target-unit ticks, or wall-clock-adjusts where Spark needs a relabel or session-tz conversion with Spark DST rules. - **Shuffle**: whole-stream compression framing (arrow IPC per-buffer compression is a different byte format and lacks Snappy), pre-encoded schema fast path (byte-identical to StreamWriter, exists to avoid re-encoding), radix sort over packed record pointers (not arrow arrays), counting-sort partition grouping (O(n) versus O(n log n) for the kernel route); row gather already uses `interleave_record_batch`. - **columnar_to_row**: UnsafeRow/UnsafeArrayData writers are Spark byte-format; `arrow_row::RowConverter` produces arrow's row format and is not applicable. - **Aggregates**: decimal sum/avg per-row overflow checks (batch-level checking would change which inputs go null), Spark ROUND_HALF_UP average, Welford recurrences, percentile/HLL++/QuantileSummaries (Spark sketch formats), GroupsAccumulator per-group loops (arrow has no grouped kernels). - **Hashes and bloom filter**: seed-chained murmur3/xxhash64 dispatch and Spark `BitArray`/bloom serialization formats are Spark-mandated (see #5103 for the xxhash64 upstream option). - **Strings**: base64 (MIME 76-char CRLF chunking), regexp_extract family (Spark group/no-match/error semantics plus all-matches extraction that arrow lacks), split (Spark limit semantics), read-side padding (no arrow lpad/rpad; deliberate performance design), is_nan (null to false is Spark-specific), get_json_object (Spark JSONPath). - **Math**: abs/ceil/floor/round/decimal div/wide decimal binary (no arrow kernels for these shapes, or Spark type-widening and rounding rules; array paths already use arity helpers), checkoverflow (needs the offending value in ANSI errors), normalize_nan, unscaled_value. - **Datetime**: unix_timestamp (needs floor division, arrow cast truncates), hours/seconds_to_timestamp/make_date/make_time/next_day/to_time/day_month_name (Spark grammars, error texts, and locale tables), extract_date_part already uses arrow `date_part`. - **Copy/scan operators**: already built on `MutableArrayData` and `cast_with_options`; extra copies exist to break FFI buffer ownership. ### Related - The audit found that `contains.rs` and `array_slice.rs` each document an upstream DataFusion issue that, once fixed, allows deleting the Comet file; those are tracked in the files themselves. - #1371 (float to decimal precision) is shared behavior with arrow's cast, so #5095 neither fixes nor worsens it. -- 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]
