comphead opened a new pull request, #6861: URL: https://github.com/apache/datafusion-comet/pull/6861
## Which issue does this PR close? There is no issue for this change. It is a sweep of the native Rust code for two allocation patterns. ## Rationale for this change Two patterns allocate more than needed across `native/`: - A collection is created empty (`Vec::new()`, `HashMap::new()`, `String::new()`, an Arrow builder) and then filled to a size that is already known, so it reallocates as it grows. - A loop over rows or batches allocates a fresh `Vec`, `String` or writer in every iteration, where one could be reused, or the data borrowed or moved instead. This PR fixes the instances in production code where the bound is exact or the reuse is local. The largest wins remove a heap allocation per row. ## What changes are included in this PR? Allocations removed per row: - `columnar_to_row.rs`: `i128_to_spark_decimal_bytes` returned a `Vec<u8>` for every decimal with precision above 18. It now returns a `[u8; 16]` and a start index. - `parquet_support.rs`: converting an Iceberg UUID (`FixedSizeBinary(16)`) to `Utf8` called `to_string()` per value. It now encodes into one reused stack buffer and appends to a presized `StringBuilder`. - `next_day.rs`: `day_of_week_from_string` called `to_uppercase()` per row. ASCII input is now matched with `eq_ignore_ascii_case`. Non-ASCII input still goes through `to_uppercase()`, because Unicode uppercasing maps a few non-ASCII letters, such as `U+017F`, to ASCII ones. - `conversion_funcs/string.rs`: `tz_from_offset_secs` used `format!` for every timestamp string with a zone suffix such as `Z`. It now formats into a stack buffer. - `bloom_filter_agg.rs`: `update_batch` built a `ScalarValue` per row, which allocates a `String` for `Utf8` input. String arrays are now hashed in place. - `split.rs`: with `limit = 0`, each row's parts were collected into a `Vec` so that trailing empty parts could be dropped. Empty parts are now held back until a non-empty part follows. Allocations removed per batch: - `spark_unsafe/row.rs`: `process_sorted_row_partition` built a `ShuffleBlockWriter`, which pre-encodes the IPC schema, for every batch. The writer is now kept until the batch schema changes, which happens when a dictionary-encoded column falls back to a plain array. - `spark_unsafe/row.rs`: the field-major struct conversion allocated three vectors for every nested struct field. They are now allocated once per call and cleared for each field. - `explode.rs`: `flatten_struct_cols` and the column merge in `build_batch` allocated a one-element `Vec` per column before flattening. Both now append into one vector of known capacity. - `multi_partition.rs` (hash-all round robin), `schema_align.rs` and `parquet_writer.rs` borrow the batch's columns, or move them out with `into_parts()`, instead of copying them into a new `Vec`. - `spark_bloom_filter.rs`: serialization cloned the bit array and then copied it twice more. It now writes each word into one presized buffer, and `SparkBitArray::data()` returns a slice. - Smaller ones: presized output vectors in `expand.rs` and `SparkPercentileGroupsAccumulator::state`, an all-true MERGE mask built without a temporary `Vec<bool>` in `merge_rows.rs`, and no `class_name` clone per batch in `JvmScalarUdfExpr`. Cold paths, changed only where the bound is exact: presized maps and sets in parquet schema matching (`parquet_support.rs`, `schema_adapter.rs`), `iceberg_dictionary.rs`, the builders in `process_sorted_row_partition`, explode's per-batch maps, a loop-invariant `Schema::empty()` hoisted in `planner.rs`, `SparkPlan::new_with_additional` moving its `Vec` instead of copying it, and a reused line buffer in `analyze_trace`. Left alone on purpose: - The RSS writer's per-frame buffers, which are sized and released inside each frame's memory reservation. - Reusing arrow `Rows` across batches in range partitioning and `RankLimitStream`, which would keep memory that the pools do not track alive between batches. - The per-call vector of completed batches in `BufBatchWriter::write`. Removing it changes which batches are dropped when a write fails. ## How are these changes tested? None of the changes alter output, so they are covered by existing tests, for example `test_i128_to_spark_decimal_bytes`, `test_day_of_week_from_string_no_trim`, `test_split_limit_zero`, the bloom filter round-trip tests, the string-to-timestamp offset cases in `conversion_funcs/string.rs`, and `CometIcebergNativeSuite` for UUID columns. `test_i128_to_spark_decimal_bytes` was updated for the new return type. `cargo clippy --all-targets --workspace -- -D warnings` passes locally. The test suites were not run locally, and there are no benchmark numbers for these changes. -- 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]
