apache / apache/datafusion-comet

[EPIC] Replace hand-rolled native code with existing arrow-rs kernels

Open
#5,104 0 comments 2 reactions 0 assignees View on GitHub
enhancement EPIC good first issue
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 6h
Merged PRs (30d)
190

Description

### 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.

Contributor guide

Open the contributing guide

Research direction

Start with the checklist of follow-up issues and the named native areas: spark-expr, core, shuffle, and common. Read the relevant vendored arrow 58.4.0 sources and the individual issue before choosing one candidate. Done means the applicable checklist items are completed while the documented Spark-specific exceptions remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
data-engineering
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.