feat(parquet): fuse level encoding passes and compact level representation#9653
Draft
HippoBaro wants to merge 3 commits into
Draft
feat(parquet): fuse level encoding passes and compact level representation#9653HippoBaro wants to merge 3 commits into
HippoBaro wants to merge 3 commits into
Conversation
HippoBaro
force-pushed
the
faster_sparse_columns_encoding
branch
from
335fb81 to
44dae05
Compare
Contributor
Author
|
This is a continuation of the work done in #9447 to improve runtime performance around sparse and/or highly uniform columns. As such this may be of interest to @alamb and @etseidl. 5a1d3d7 adds three benchmarks that exercise the code path this series optimizes. I created a PR (#9654) to merge those separately if needed so the benchmark bot can have a baseline to compare against. Thanks! |
HippoBaro
force-pushed
the
faster_sparse_columns_encoding
branch
from
44dae05 to
7902e69
Compare
etseidl
reviewed
Apr 2, 2026
Contributor
|
Thanks @HippoBaro, this looks impressive. I'm still looking, but haven't found any obvious problems yet. Gads, every time I delve this deep into parquet I go a little mad 😵💫. I think the RLE encoder could use a little refactoring/comment improvements to make the flow a little more obvious. Not as part of this PR though. |
etseidl
reviewed
Apr 2, 2026
Contributor
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Flushing a few comments. More tomorrow.
| let mut values_to_write = 0usize; | ||
| let max_def = self.descr.max_def_level(); | ||
| self.def_levels_encoder | ||
| .put_with_observer(levels, |level, count| { |
Contributor
There was a problem hiding this comment.
❤️ When I added the histograms I wasn't happy with the redundancy here. Nice fix!
etseidl
reviewed
Apr 2, 2026
jhorstmann
reviewed
Apr 7, 2026
HippoBaro
force-pushed
the
faster_sparse_columns_encoding
branch
from
7902e69 to
c891c35
Compare
Contributor
Author
|
Thanks for the reviews! I've reworked the branch to address all feedback. Sorry for the delay, it took me a while to experiment. The main structural change is a The enum LevelData {
Absent,
Materialized(Vec<i16>),
Uniform { value: i16, count: usize },
}
The resulting refactor has a larger LoC footprint, but the API is arguably much cleaner and robust. Also, rebased as per #9656 (review) |
Contributor
|
Thanks @HippoBaro. I'll try to make some time to review the changes. Probably not today but hopefully tomorrow... 🤞 |
HippoBaro
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
alamb
pushed a commit
that referenced
this pull request
Apr 9, 2026
# Which issue does this PR close? - None, but relates to #9653 # Rationale for this change #9653 introduces optimizations related to non-null uniform workloads. This adds benchmarks so we can quantify them. # What changes are included in this PR? Add three new benchmark cases to the arrow_writer benchmark suite for evaluating write performance on struct columns at varying null densities: * `struct_non_null`: a nullable struct with 0% null rows and non-nullable primitive children; * `struct_sparse_99pct_null`: a nullable struct with 99% null rows, exercising null batching through one level of struct nesting; * `struct_all_null`: a nullable struct with 100% null rows, exercising the uniform-null path through struct nesting. Baseline results (Apple M1 Max): ``` struct_non_null/default 29.9 ms struct_non_null/parquet_2 38.2 ms struct_non_null/zstd_parquet_2 50.9 ms struct_sparse_99pct_null/default 7.2 ms struct_sparse_99pct_null/parquet_2 7.3 ms struct_sparse_99pct_null/zstd_p2 8.1 ms struct_all_null/default 83.3 µs struct_all_null/parquet_2 82.5 µs struct_all_null/zstd_parquet_2 106.6 µs ``` # Are these changes tested? N/A # Are there any user-facing changes? None Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
Contributor
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
I am surprised by the few regressions above, such as: I can't reproduce these locally. I get: Are these known to be noisy? |
Contributor
Yes. They are extremely twitchy. I always take them with a grain of salt or ten. 😅 |
Contributor
|
I've now run multiple passes of the arrow_writer bench on my workstation and there appear to be no regressions due to this PR. And the speed ups are quite impressive 😄 Details |
Contributor
|
@kszucs do you have time to look at this PR? It touches on your CDC code. |
Contributor
|
I am hoping to review this tomorrow |
HippoBaro
force-pushed
the
faster_sparse_columns_encoding
branch
from
fdc9bbe to
8252f31
Compare
alamb
pushed a commit
that referenced
this pull request
May 14, 2026
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from #9653 - Contributes to #9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See #9731 # What changes are included in this PR? When an entire list, struct, fixed-size list, or leaf array is null, skip per-row iteration and emit bulk uniform def/rep levels via `extend_uniform_levels` in O(1). # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> All tests passing + additional all null unit tests. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
alamb
pushed a commit
that referenced
this pull request
May 20, 2026
## Which issue does this PR close? - Contributes to #9731. ## AI assistance Implementation drafted with AI assistance and iterated against the benchmarks below. I've reviewed and own the code, including the gate threshold which I picked from the sweep in [Threshold (`BULK_FILL_MIN_LEN`)](#threshold-bulk_fill_min_len). Per the project's [CONTRIBUTING guidance on AI-generated submissions](https://github.com/apache/arrow-rs/blob/main/CONTRIBUTING.md#ai-generated-submissions). ## Rationale for this change When writing a nullable leaf (primitive) Arrow array, `write_leaf` builds the definition-level buffer one element at a time, mapping each null bit to a level. For columns that are mostly null this does ~`num_rows` of branchy work and allocates a `num_rows`-element level buffer even though almost every produced level is the same value. #9954 adds an O(1) fast path for the *entirely* null case; this PR covers the *sparse* (mostly-but-not-entirely null) case it doesn't handle, the literal subject of #9731 ("a column that is 99% null … ~100x more work than necessary"). ## What changes are included in this PR? A single popcount pass over the null mask (`Buffer::count_set_bits_offset`, O(`num_rows`/64)) counts the valid values in the range. When the slice is majority-null, the definition-level buffer is bulk-filled with the null level (a vectorized `Vec::resize` memset) and only the non-null positions (from `NullBuffer::valid_indices()`) are overwritten. The existing per-row path is kept for non-majority-null slices, so balanced and null-light columns are unaffected. Both branches share the same `let range_nulls = nulls.slice(range.start, len)` slicing idiom; the slow path uses `range_nulls.iter()` for the def-level map and `range_nulls.valid_indices().map(|i| i + range.start)` for `non_null_indices`, with no `unsafe`. Output is byte-identical: the level *values* are unchanged, just produced via memset+scatter (fast path) or via the high-level `NullBuffer` iterators (slow path) instead of a manual `BitIndexIterator` walk. ## Threshold (`BULK_FILL_MIN_LEN`) The bulk-fill fast path is gated on two conditions: - `len >= BULK_FILL_MIN_LEN` (currently 64). Per-call slice/popcount/iterator overhead only amortizes on sizable sub-ranges. List/struct paths call `write_leaf` many times with tiny ranges (avg list length 1-5); paying any per-call popcount there would regress them. A threshold sweep at T = {0, 16, 32, 64, 128, 256} on Ryzen 9 9950X shows the regression floor settles by T=32, and the choice of 64 gives ~12x margin over the average list length without losing the flat-primitive wins. - `nulls.null_count() * 2 >= nulls.len()`. The cached `null_count()` is O(1), so this check is free. We use the buffer-wide density as a heuristic for the sub-range; for full-array writes (the primary target, flat primitive columns) it's exact. Even when the gate skips the fast path, evaluating it across high-frequency call sites (~10K calls in some list benchmarks) is a small structural cost (~1-2% on list-sparse cases). The wins on the targeted shapes (-35% sparse-primitive, -66% all-null primitive) far outweigh that. Reducing the cost further would require hoisting the decision into the caller. ## Are these changes tested? Existing tests cover this path: `cargo test -p parquet --features arrow --lib arrow_writer` is green (136 tests, full of nulls and roundtrips); full `cargo test -p parquet --features arrow` green modulo the pre-existing `PARQUET_TEST_DATA` submodule failures (unrelated, same on `main`). `cargo clippy -p parquet --features arrow --lib` and `cargo fmt --check` clean. The `unsafe get_unchecked_mut` flagged in the original revision was replaced via `NullBuffer::valid_indices()`; the slow-path also dropped its `unsafe value_unchecked` for the same reason. ## Are there any user-facing changes? None. ## Benchmarks `cargo bench -p parquet --bench arrow_writer`, 1M rows × 7 nullable primitive columns, local Ryzen 9 9950X: ``` primitive_sparse_99pct_null/default 11.88 ms -> 9.13 ms (-23%) <- the case #9731 calls out primitive_all_null/default 5.65 ms -> 2.33 ms (-59%) (subsumed by #9954's O(1) path if that lands first) struct_sparse_99pct_null/default 5.67 ms -> 5.32 ms (-6%) struct_all_null/default 1.52 ms -> 1.31 ms (-14%) list_primitive_sparse_99pct_null, primitive (25% null), primitive_non_null, bool, string: within noise (no regression) ``` The CI benchmark bot (GKE `c4a-highmem-16`, Neoverse-V2) on the post-fixup revision shows the same shape with stronger relative wins on the targeted cases: ``` primitive_all_null/default 2.47x (11.0ms -> 4.4ms) primitive_sparse_99pct_null/default 1.60x (16.8ms -> 10.5ms) primitive_all_null/{bloom_filter,cdc,parquet_2,zstd,zstd_parquet_2} 1.38x to 2.48x primitive_sparse_99pct_null/{...} 1.28x to 1.59x list_primitive*, list_primitive_sparse_99pct_null*: 1.00x to 1.01x (within noise) ``` Microbench of the definition-level fill in isolation: 10.3x @ 100%-null, 8.6x @ 99%, 5.2x @ 90%, 1.9x @ 50%, 0.93x @ 10%, 0.81x @ 0%. Crossover ≈ 12-15% null, clean win above ~25%; the `>= 50% null` guard is conservative. This is the *materialization*-cost half of #9731 (~30% of the 99%-null write); the *walk*-cost half, a run-length input to the level encoder so the column writer doesn't even iterate all `num_rows` levels, is the larger structural change #9653 is heading toward. This PR is deliberately small and isolated so it lands independently of and rebases cleanly under that work. --------- Co-authored-by: Ryan Stewart <noreply@example.com>
alamb
reviewed
May 20, 2026
| } | ||
|
|
||
| /// Computes the min and max for the provided values | ||
| #[inline] |
Contributor
There was a problem hiding this comment.
I wonder if you foudn that inlining actually helps in all these cases? We have found that in some cases inlining actually makes the performance worse (as there are a bunch of optimizations in LLVM that are disabled once the function gets too big)
Contributor
Author
There was a problem hiding this comment.
I haven't. For these small methods I tend to default to #[inline] out of habit (and that seems to be common practice in this code base AFAICT.) I'll do some benchmarking to test that.
Contributor
Author
There was a problem hiding this comment.
Based on string/default I can't measure any difference with and without forced inlining. I'm removing it 👍
| } | ||
| } | ||
|
|
||
| /// Zero-allocation iterator over the indices in a [`ValueSelection`]. |
Contributor
There was a problem hiding this comment.
this now has dispatch overhead for each item (the match self) -- we could probably make it even faster by changing the callsite to instantiate different loops based on the different iterators rather than checking each value 🤔
Contributor
Author
There was a problem hiding this comment.
Hum that's great feedback let me iterate on that
Contributor
Author
There was a problem hiding this comment.
I rebased and updated the code so that the call sites now dispatch once on ValueSelectionRef and then run the appropriate loop. Thanks!
alamb
pushed a commit
that referenced
this pull request
May 20, 2026
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from #9653 - Contributes to #9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See #9731 # What changes are included in this PR? Changes `byte_array` encoder methods (`FallbackEncoder::encode`, `DictEncoder::encode`, etc) and all `get_*_array_slice` functions from `&[usize]` to `impl ExactSizeIterator<Item = usize>`. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> All tests passing. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
Introduce `ArrayLevelsView` and use it when writing content-defined chunks, so CDC chunking can borrow sliced definition levels, repetition levels, and value selections while keeping the original Arrow leaf array. This avoids constructing per-chunk ArrayLevels instances and makes chunked writes operate on views of the existing level data. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
Thread value selection through level building and column writing as either empty, dense, or sparse instead of always materializing non-null indices. This lets dense selections use the regular `write` path with an offset and length, while sparse selections continue to use `write_gather`. It avoids unnecessary index allocation for common contiguous cases, preserves the all-null fast path, and keeps nullable/sparse writes explicit at the writer boundary. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
The dense column-writer path lets primitive arrays avoid materializing value indices and write contiguous values directly. Byte arrays still missed most of that win because their generic writer path had to call `ArrayAccessor::value` for each logical row. For `Utf8`/`Binary` arrays, each value access re-enters the Arrow array abstraction, loads adjacent offsets, bounds-checks, and constructs a slice one value at a time. That overhead is specific to offset-backed byte arrays: unlike primitive arrays, the physical values are not fixed-width slots where a dense range maps directly to a typed slice. The data lives in one values buffer, with per-row boundaries stored in an offsets buffer. Add a dense byte-array path for `Utf8`, `LargeUtf8`, `Binary`, and `LargeBinary` that walks those Arrow offsets and values buffers directly. This preserves the existing sparse/generic path for null-filtered writes, views, dictionaries, and fixed-size binary, while making the common non-null contiguous case behave like the optimized primitive dense path. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
HippoBaro
force-pushed
the
faster_sparse_columns_encoding
branch
from
8252f31 to
02c964f
Compare
Contributor
|
run benchmark arrow_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing faster_sparse_columns_encoding (02c964f) to 4b80f0e (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing faster_sparse_columns_encoding (02c964f) to 4b80f0e (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing faster_sparse_columns_encoding (02c964f) to 4b80f0e (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
The above regressions to CDC-related benchmarks are real. I'll look into it. Sorry! |
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
# Which issue does this PR close? - None, but relates to apache#9653 # Rationale for this change apache#9653 introduces optimizations related to non-null uniform workloads. This adds benchmarks so we can quantify them. # What changes are included in this PR? Add three new benchmark cases to the arrow_writer benchmark suite for evaluating write performance on struct columns at varying null densities: * `struct_non_null`: a nullable struct with 0% null rows and non-nullable primitive children; * `struct_sparse_99pct_null`: a nullable struct with 99% null rows, exercising null batching through one level of struct nesting; * `struct_all_null`: a nullable struct with 100% null rows, exercising the uniform-null path through struct nesting. Baseline results (Apple M1 Max): ``` struct_non_null/default 29.9 ms struct_non_null/parquet_2 38.2 ms struct_non_null/zstd_parquet_2 50.9 ms struct_sparse_99pct_null/default 7.2 ms struct_sparse_99pct_null/parquet_2 7.3 ms struct_sparse_99pct_null/zstd_p2 8.1 ms struct_all_null/default 83.3 µs struct_all_null/parquet_2 82.5 µs struct_all_null/zstd_parquet_2 106.6 µs ``` # Are these changes tested? N/A # Are there any user-facing changes? None Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
…pache#9751) # Which issue does this PR close? - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change The literal `8` appeared in two distinct roles throughout `RleEncoder`, `RleDecoder`, and their tests. # What changes are included in this PR? Replacing each with a named constant makes the intent explicit and prevents the two meanings from being confused. * `BIT_PACK_GROUP_SIZE = 8` The Parquet RLE/bit-packing hybrid format always bit-packs values in multiples of this count (spec: "we always bit-pack a multiple of 8 values at a time"). Every occurrence related to the staging buffer size, the repeat-count threshold that triggers the RLE decision, and the group-count arithmetic in bit-packed headers now uses this name. * `u8::BITS` (= 8, from std) Used wherever a bit-count is divided by 8 to obtain a byte-count (e.g. `ceil(bit_width, u8::BITS as usize)`). This is a bits-per-byte conversion, a fundamentally different concept from the packing-group size. No behaviour change. # Are these changes tested? All tests passing. # Are there any user-facing changes? None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
…che#9752) # Which issue does this PR close? - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change See apache#9731 # What changes are included in this PR? Restructure `write_list()` to accumulate consecutive null and empty rows and flush them in a single `visit_leaves()` call using `extend(repeat_n(...))`, instead of calling `visit_leaves()` per row. With sparse data (99% nulls), a 4096-row batch previously triggered ~4000 individual tree traversals, each pushing a single value per leaf. Now consecutive null/empty runs are collapsed into one traversal that extends all leaf level buffers in bulk. This follows the same pattern already used by `write_struct()`. The `write_non_null_slice` path is unchanged since each non-null row has different offsets and cannot be batched. # Are these changes tested? All tests passing; existing tests give 100% coverage. # Are there any user-facing changes? N/A Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
apache#9795) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change See apache#9731 # What changes are included in this PR? Add `put_with_observer()` to `LevelEncoder` that calls an `FnMut(i16, usize)` observer for each value during encoding. This allows callers to piggyback counting and histogram updates into the encoding pass without extra iterations over the level buffer. Previously, `write_mini_batch()` made 3 separate passes over each level array: one to count non-null values or row boundaries, one to update the level histogram, and one to RLE-encode. Now all three operations happen in a single pass via the observer closure. Replace `LevelHistogram::update_from_levels()` with a new `LevelHistogram::increment_by()` that accepts a count, and remove the now-unnecessary `update_definition_level_histogram()` and `update_repetition_level_histogram()` methods from PageMetrics. # Are these changes tested? All tests passing; existing tests give 100% coverage. # Are there any user-facing changes? None --------- Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com> Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
…#9830) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See apache#9731 # What changes are included in this PR? Add `is_accumulating_rle()` and `extend_run()` methods to `RleEncoder` that allow callers to detect when the encoder is in RLE accumulation mode and bulk-extend runs without per-element overhead. Upgrade `put_with_observer()` in `LevelEncoder` to exploit this: after each `put()`, it checks whether the encoder entered accumulation mode. If so, it scans ahead for the rest of the run, calls `extend_run()` to batch it in O(1), and fires the observer once with the full run length. This turns the previous O(n) per-value encoding + observation into O(1) amortized per RLE run, which is a significant improvement for sparse columns where long runs of identical levels are common. # Are these changes tested? All tests passing + added coverage around RLE accumulation mode trigger. # Are there any user-facing changes? None. --------- Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com> Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
…tch (apache#9831) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See apache#9731 # What changes are included in this PR? Represent definition and repetition levels as `LevelData`/`LevelDataRef` with `Absent`, `Materialized`, and `Uniform` variants, and thread this through Arrow level generation, CDC chunking, and the generic column writer. Uniform level runs, such as required fields and all-null pages, can now be encoded without materializing dense `Vec<i16>` buffers. Add bulk run support to `LevelEncoder`/`RleEncoder` so repeated levels are encoded in amortized O(1) after the RLE warmup, while preserving histogram, row count, null count, page splitting, and CDC chunk accounting. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> All tests passing. Coverage exercises bulk RLE level encoding, compact/uniform `LevelData` slicing and writer roundtrips across Parquet v1/v2, and CDC/Arrow writer behavior including all-null and nested-level cases. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> None. --------- Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See apache#9731 # What changes are included in this PR? When an entire list, struct, fixed-size list, or leaf array is null, skip per-row iteration and emit bulk uniform def/rep levels via `extend_uniform_levels` in O(1). # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> All tests passing + additional all null unit tests. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
## Which issue does this PR close? - Contributes to apache#9731. ## AI assistance Implementation drafted with AI assistance and iterated against the benchmarks below. I've reviewed and own the code, including the gate threshold which I picked from the sweep in [Threshold (`BULK_FILL_MIN_LEN`)](#threshold-bulk_fill_min_len). Per the project's [CONTRIBUTING guidance on AI-generated submissions](https://github.com/apache/arrow-rs/blob/main/CONTRIBUTING.md#ai-generated-submissions). ## Rationale for this change When writing a nullable leaf (primitive) Arrow array, `write_leaf` builds the definition-level buffer one element at a time, mapping each null bit to a level. For columns that are mostly null this does ~`num_rows` of branchy work and allocates a `num_rows`-element level buffer even though almost every produced level is the same value. apache#9954 adds an O(1) fast path for the *entirely* null case; this PR covers the *sparse* (mostly-but-not-entirely null) case it doesn't handle, the literal subject of apache#9731 ("a column that is 99% null … ~100x more work than necessary"). ## What changes are included in this PR? A single popcount pass over the null mask (`Buffer::count_set_bits_offset`, O(`num_rows`/64)) counts the valid values in the range. When the slice is majority-null, the definition-level buffer is bulk-filled with the null level (a vectorized `Vec::resize` memset) and only the non-null positions (from `NullBuffer::valid_indices()`) are overwritten. The existing per-row path is kept for non-majority-null slices, so balanced and null-light columns are unaffected. Both branches share the same `let range_nulls = nulls.slice(range.start, len)` slicing idiom; the slow path uses `range_nulls.iter()` for the def-level map and `range_nulls.valid_indices().map(|i| i + range.start)` for `non_null_indices`, with no `unsafe`. Output is byte-identical: the level *values* are unchanged, just produced via memset+scatter (fast path) or via the high-level `NullBuffer` iterators (slow path) instead of a manual `BitIndexIterator` walk. ## Threshold (`BULK_FILL_MIN_LEN`) The bulk-fill fast path is gated on two conditions: - `len >= BULK_FILL_MIN_LEN` (currently 64). Per-call slice/popcount/iterator overhead only amortizes on sizable sub-ranges. List/struct paths call `write_leaf` many times with tiny ranges (avg list length 1-5); paying any per-call popcount there would regress them. A threshold sweep at T = {0, 16, 32, 64, 128, 256} on Ryzen 9 9950X shows the regression floor settles by T=32, and the choice of 64 gives ~12x margin over the average list length without losing the flat-primitive wins. - `nulls.null_count() * 2 >= nulls.len()`. The cached `null_count()` is O(1), so this check is free. We use the buffer-wide density as a heuristic for the sub-range; for full-array writes (the primary target, flat primitive columns) it's exact. Even when the gate skips the fast path, evaluating it across high-frequency call sites (~10K calls in some list benchmarks) is a small structural cost (~1-2% on list-sparse cases). The wins on the targeted shapes (-35% sparse-primitive, -66% all-null primitive) far outweigh that. Reducing the cost further would require hoisting the decision into the caller. ## Are these changes tested? Existing tests cover this path: `cargo test -p parquet --features arrow --lib arrow_writer` is green (136 tests, full of nulls and roundtrips); full `cargo test -p parquet --features arrow` green modulo the pre-existing `PARQUET_TEST_DATA` submodule failures (unrelated, same on `main`). `cargo clippy -p parquet --features arrow --lib` and `cargo fmt --check` clean. The `unsafe get_unchecked_mut` flagged in the original revision was replaced via `NullBuffer::valid_indices()`; the slow-path also dropped its `unsafe value_unchecked` for the same reason. ## Are there any user-facing changes? None. ## Benchmarks `cargo bench -p parquet --bench arrow_writer`, 1M rows × 7 nullable primitive columns, local Ryzen 9 9950X: ``` primitive_sparse_99pct_null/default 11.88 ms -> 9.13 ms (-23%) <- the case apache#9731 calls out primitive_all_null/default 5.65 ms -> 2.33 ms (-59%) (subsumed by apache#9954's O(1) path if that lands first) struct_sparse_99pct_null/default 5.67 ms -> 5.32 ms (-6%) struct_all_null/default 1.52 ms -> 1.31 ms (-14%) list_primitive_sparse_99pct_null, primitive (25% null), primitive_non_null, bool, string: within noise (no regression) ``` The CI benchmark bot (GKE `c4a-highmem-16`, Neoverse-V2) on the post-fixup revision shows the same shape with stronger relative wins on the targeted cases: ``` primitive_all_null/default 2.47x (11.0ms -> 4.4ms) primitive_sparse_99pct_null/default 1.60x (16.8ms -> 10.5ms) primitive_all_null/{bloom_filter,cdc,parquet_2,zstd,zstd_parquet_2} 1.38x to 2.48x primitive_sparse_99pct_null/{...} 1.28x to 1.59x list_primitive*, list_primitive_sparse_99pct_null*: 1.00x to 1.01x (within noise) ``` Microbench of the definition-level fill in isolation: 10.3x @ 100%-null, 8.6x @ 99%, 5.2x @ 90%, 1.9x @ 50%, 0.93x @ 10%, 0.81x @ 0%. Crossover ≈ 12-15% null, clean win above ~25%; the `>= 50% null` guard is conservative. This is the *materialization*-cost half of apache#9731 (~30% of the 99%-null write); the *walk*-cost half, a run-length input to the level encoder so the column writer doesn't even iterate all `num_rows` levels, is the larger structural change apache#9653 is heading toward. This PR is deliberately small and isolated so it lands independently of and rebases cleanly under that work. --------- Co-authored-by: Ryan Stewart <noreply@example.com>
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Spawn off from apache#9653 - Contributes to apache#9731 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> See apache#9731 # What changes are included in this PR? Changes `byte_array` encoder methods (`FallbackEncoder::encode`, `DictEncoder::encode`, etc) and all `get_*_array_slice` functions from `&[usize]` to `impl ExactSizeIterator<Item = usize>`. # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> All tests passing. # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> None. Signed-off-by: Hippolyte Barraud <hippolyte.barraud@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
See issue for details. The Parquet column writer currently does per-value work during level encoding regardless of data sparsity, even though the output encoding (RLE) is proportional to the number of runs.
What changes are included in this PR?
Three incremental commits, each building on the previous:
Fuse level encoding with counting and histogram updates.
write_mini_batch()previously made three separate passes over each level array: count non-nulls, update the level histogram, and RLE-encode. Now all three happen in a single pass via an observer callback onLevelEncoder. When the RLE encoder enters accumulation mode, the loop scans ahead for the full run length and batches the observer call. This makes counting and histogram updates O(1) per run.Batch consecutive null/empty rows in
write_list. Consecutive null or empty list entries are now collapsed into a singlevisit_leaves()call that bulk-extends all leaf level buffers, instead of one tree traversal per null row. Mirrors the approach already used bywrite_struct().Short-circuit entirely-null columns. When every element in an array is null, skip
Vec<i16>level-buffer materialization entirely and store a compact(def_value, rep_value, count)tuple. The writer encodes this viaRleEncoder::put_n()in O(1) amortized time, bypassing the normal mini-batch loop.Are these changes tested?
All tests passing. I added some benchmark to exercice the heavy and all-null code paths, alongside the existing 25% sparseness benchmarks:
Non-nullable column benchmarks are within noise, as expected since they have no definition levels to optimize.
Are there any user-facing changes?
None.