From ea343799cfd9608d149fef9a6511d718ea413296 Mon Sep 17 00:00:00 2001 From: Richard Baah Date: Fri, 23 Oct 2026 13:37:18 -0400 Subject: [PATCH 1/4] feat(buffer): add NullBufferBuilder::append_word Append packed validity words while preserving lazy allocation for all-valid data. Include focused correctness tests and per-bit comparison benchmarks. --- arrow-buffer/Cargo.toml | 4 ++ arrow-buffer/benches/null_buffer_builder.rs | 67 +++++++++++++++++++ arrow-buffer/src/builder/null.rs | 73 +++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 arrow-buffer/benches/null_buffer_builder.rs diff --git a/arrow-buffer/Cargo.toml b/arrow-buffer/Cargo.toml index 39bdf82120b4..1d71d5c288a3 100644 --- a/arrow-buffer/Cargo.toml +++ b/arrow-buffer/Cargo.toml @@ -68,5 +68,9 @@ harness = false name = "mutable_buffer_extend" harness = false +[[bench]] +name = "null_buffer_builder" +harness = false + [lints] workspace = true diff --git a/arrow-buffer/benches/null_buffer_builder.rs b/arrow-buffer/benches/null_buffer_builder.rs new file mode 100644 index 000000000000..415b3224f094 --- /dev/null +++ b/arrow-buffer/benches/null_buffer_builder.rs @@ -0,0 +1,67 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use arrow_buffer::NullBufferBuilder; +use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main}; +use std::hint; + +const WORDS: usize = 512; +const LEN: usize = WORDS * 64; + +fn criterion_benchmark(c: &mut Criterion) { + let sparse_nulls = (0..WORDS) + .map(|i| if i % 8 == 0 { u64::MAX - 1 } else { u64::MAX }) + .collect(); + let inputs = [ + ("all_valid", vec![u64::MAX; WORDS]), + ("sparse_nulls", sparse_nulls), + ("alternating", vec![0xAAAA_AAAA_AAAA_AAAA; WORDS]), + ]; + + let mut group = c.benchmark_group("NullBufferBuilder append"); + group.throughput(Throughput::Elements(LEN as u64)); + + for (name, words) in inputs { + group.bench_with_input(BenchmarkId::new("bit", name), &words, |b, words| { + b.iter(|| { + let mut builder = NullBufferBuilder::new(LEN); + for &word in words { + let word = hint::black_box(word); + for i in 0..64 { + builder.append(word & (1 << i) != 0); + } + } + hint::black_box(builder.finish()) + }) + }); + + group.bench_with_input(BenchmarkId::new("word", name), &words, |b, words| { + b.iter(|| { + let mut builder = NullBufferBuilder::new(LEN); + for &word in words { + builder.append_word(hint::black_box(word), 64); + } + hint::black_box(builder.finish()) + }) + }); + } + + group.finish(); +} + +criterion_group!(benches, criterion_benchmark); +criterion_main!(benches); diff --git a/arrow-buffer/src/builder/null.rs b/arrow-buffer/src/builder/null.rs index ee58ab591ec2..87fa23078f84 100644 --- a/arrow-buffer/src/builder/null.rs +++ b/arrow-buffer/src/builder/null.rs @@ -149,6 +149,23 @@ impl NullBufferBuilder { } } + /// Appends the low `count` bits from `word` as validity values. + /// + /// `word` is treated as a packed LSB-first bitmap, where set bits indicate + /// non-null values. Only the lowest `count` bits are appended; higher bits + /// are ignored. + /// + /// This does not materialize the null buffer when all appended bits are set. + #[inline] + pub fn append_word(&mut self, word: u64, count: usize) { + debug_assert!(count <= 64); + if word.trailing_ones() as usize >= count { + self.append_n_non_nulls(count); + } else { + self.materialize_if_needed().append_word(word, count) + } + } + /// Sets a bit in the builder at `index` /// /// # Panics @@ -339,6 +356,62 @@ mod tests { assert_eq!(&[0b1011_u8], buf.validity()); } + fn check_append_word(initial_bits: usize, word: u64, count: usize) { + let mut got = NullBufferBuilder::new(0); + let mut expected = NullBufferBuilder::new(0); + got.append_n_non_nulls(initial_bits); + expected.append_n_non_nulls(initial_bits); + got.append_word(word, count); + for i in 0..count { + expected.append(word & (1 << i) != 0); + } + assert_eq!(got.len(), expected.len()); + assert_eq!(got.finish(), expected.finish()); + } + + #[test] + fn test_append_word_does_not_materialize_all_valid() { + let mut builder = NullBufferBuilder::new(0); + builder.append_word(u64::MAX, 64); + builder.append_word(0b111, 3); + builder.append_word(0, 0); + + assert_eq!(builder.len(), 67); + assert_eq!(builder.allocated_size(), 0); + assert!(builder.finish().is_none()); + } + + #[test] + fn test_append_word_aligned() { + for count in [1, 5, 8, 17, 64] { + check_append_word(0, 0xDEAD_BEEF_CAFE_BABE, count); + } + } + + #[test] + fn test_append_word_unaligned() { + for offset in 1..=7 { + check_append_word(offset, 0xDEAD_BEEF_CAFE_BABE, 64); + } + } + + #[test] + fn test_append_word_after_materialization() { + let mut got = NullBufferBuilder::new(0); + let mut expected = NullBufferBuilder::new(0); + got.append_null(); + expected.append_null(); + + for (word, count) in [(u64::MAX, 64), (0b1010, 4), (0b10, 1)] { + got.append_word(word, count); + for i in 0..count { + expected.append(word & (1 << i) != 0); + } + } + + assert_eq!(got.finish(), expected.finish()); + } + #[test] fn test_null_buffer_builder_is_valid() { let mut builder = NullBufferBuilder::new(0); From bb433590816f0949798e8da10d2eb26abc846a0a Mon Sep 17 00:00:00 2001 From: Richard Baah Date: Fri, 23 Oct 2026 14:10:56 -0400 Subject: [PATCH 2/4] bench(buffer): cover append_word validity patterns --- arrow-buffer/benches/null_buffer_builder.rs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/arrow-buffer/benches/null_buffer_builder.rs b/arrow-buffer/benches/null_buffer_builder.rs index 415b3224f094..dcb09f18d418 100644 --- a/arrow-buffer/benches/null_buffer_builder.rs +++ b/arrow-buffer/benches/null_buffer_builder.rs @@ -23,12 +23,17 @@ const WORDS: usize = 512; const LEN: usize = WORDS * 64; fn criterion_benchmark(c: &mut Criterion) { + let very_sparse_nulls = (0..WORDS) + .map(|i| if i % 64 == 0 { u64::MAX - 1 } else { u64::MAX }) + .collect(); let sparse_nulls = (0..WORDS) .map(|i| if i % 8 == 0 { u64::MAX - 1 } else { u64::MAX }) .collect(); let inputs = [ ("all_valid", vec![u64::MAX; WORDS]), + ("very_sparse_nulls", very_sparse_nulls), ("sparse_nulls", sparse_nulls), + ("dense_nulls", vec![0x0000_0000_0000_FFFF; WORDS]), ("alternating", vec![0xAAAA_AAAA_AAAA_AAAA; WORDS]), ]; @@ -36,19 +41,6 @@ fn criterion_benchmark(c: &mut Criterion) { group.throughput(Throughput::Elements(LEN as u64)); for (name, words) in inputs { - group.bench_with_input(BenchmarkId::new("bit", name), &words, |b, words| { - b.iter(|| { - let mut builder = NullBufferBuilder::new(LEN); - for &word in words { - let word = hint::black_box(word); - for i in 0..64 { - builder.append(word & (1 << i) != 0); - } - } - hint::black_box(builder.finish()) - }) - }); - group.bench_with_input(BenchmarkId::new("word", name), &words, |b, words| { b.iter(|| { let mut builder = NullBufferBuilder::new(LEN); From 2ae1e95b190cad247c4e33c06978ca6d27395423 Mon Sep 17 00:00:00 2001 From: Richard Baah Date: Fri, 23 Oct 2026 14:18:30 -0400 Subject: [PATCH 3/4] test(buffer): refine append_word cases --- arrow-buffer/benches/null_buffer_builder.rs | 2 +- arrow-buffer/src/builder/null.rs | 52 ++++----------------- 2 files changed, 10 insertions(+), 44 deletions(-) diff --git a/arrow-buffer/benches/null_buffer_builder.rs b/arrow-buffer/benches/null_buffer_builder.rs index dcb09f18d418..00d1ded9d147 100644 --- a/arrow-buffer/benches/null_buffer_builder.rs +++ b/arrow-buffer/benches/null_buffer_builder.rs @@ -24,7 +24,7 @@ const LEN: usize = WORDS * 64; fn criterion_benchmark(c: &mut Criterion) { let very_sparse_nulls = (0..WORDS) - .map(|i| if i % 64 == 0 { u64::MAX - 1 } else { u64::MAX }) + .map(|i| if i % 32 == 0 { u64::MAX - 1 } else { u64::MAX }) .collect(); let sparse_nulls = (0..WORDS) .map(|i| if i % 8 == 0 { u64::MAX - 1 } else { u64::MAX }) diff --git a/arrow-buffer/src/builder/null.rs b/arrow-buffer/src/builder/null.rs index 87fa23078f84..e04974c6bf67 100644 --- a/arrow-buffer/src/builder/null.rs +++ b/arrow-buffer/src/builder/null.rs @@ -356,25 +356,11 @@ mod tests { assert_eq!(&[0b1011_u8], buf.validity()); } - fn check_append_word(initial_bits: usize, word: u64, count: usize) { - let mut got = NullBufferBuilder::new(0); - let mut expected = NullBufferBuilder::new(0); - got.append_n_non_nulls(initial_bits); - expected.append_n_non_nulls(initial_bits); - got.append_word(word, count); - for i in 0..count { - expected.append(word & (1 << i) != 0); - } - assert_eq!(got.len(), expected.len()); - assert_eq!(got.finish(), expected.finish()); - } - #[test] - fn test_append_word_does_not_materialize_all_valid() { + fn test_append_word_all_valid() { let mut builder = NullBufferBuilder::new(0); builder.append_word(u64::MAX, 64); - builder.append_word(0b111, 3); - builder.append_word(0, 0); + builder.append_word(0b0111, 3); assert_eq!(builder.len(), 67); assert_eq!(builder.allocated_size(), 0); @@ -382,34 +368,14 @@ mod tests { } #[test] - fn test_append_word_aligned() { - for count in [1, 5, 8, 17, 64] { - check_append_word(0, 0xDEAD_BEEF_CAFE_BABE, count); - } - } - - #[test] - fn test_append_word_unaligned() { - for offset in 1..=7 { - check_append_word(offset, 0xDEAD_BEEF_CAFE_BABE, 64); - } - } - - #[test] - fn test_append_word_after_materialization() { - let mut got = NullBufferBuilder::new(0); - let mut expected = NullBufferBuilder::new(0); - got.append_null(); - expected.append_null(); - - for (word, count) in [(u64::MAX, 64), (0b1010, 4), (0b10, 1)] { - got.append_word(word, count); - for i in 0..count { - expected.append(word & (1 << i) != 0); - } - } + fn test_append_word_mixed() { + let mut builder = NullBufferBuilder::new(0); + builder.append_n_non_nulls(3); + builder.append_word(0b1100_1010, 6); + builder.append_word(0b1111, 4); - assert_eq!(got.finish(), expected.finish()); + let buffer = builder.finish().unwrap(); + assert_eq!(&[0b0101_0111, 0b0001_1110], buffer.validity()); } #[test] From 338b37021cad087fa482c132a65a5d0fae7ab4e8 Mon Sep 17 00:00:00 2001 From: Richard Baah Date: Fri, 23 Oct 2026 15:07:16 -0400 Subject: [PATCH 4/4] fix(buffer): validate append_word count Reject counts above 64 in optimized builds as well as debug builds. Document the public contract and cover the invalid input with a regression test. --- arrow-buffer/src/builder/null.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/arrow-buffer/src/builder/null.rs b/arrow-buffer/src/builder/null.rs index e04974c6bf67..5d9331146a83 100644 --- a/arrow-buffer/src/builder/null.rs +++ b/arrow-buffer/src/builder/null.rs @@ -156,9 +156,13 @@ impl NullBufferBuilder { /// are ignored. /// /// This does not materialize the null buffer when all appended bits are set. + /// + /// # Panics + /// + /// Panics if `count` is greater than 64. #[inline] pub fn append_word(&mut self, word: u64, count: usize) { - debug_assert!(count <= 64); + assert!(count <= 64, "count must be less than or equal to 64"); if word.trailing_ones() as usize >= count { self.append_n_non_nulls(count); } else { @@ -378,6 +382,13 @@ mod tests { assert_eq!(&[0b0101_0111, 0b0001_1110], buffer.validity()); } + #[test] + #[should_panic(expected = "count must be less than or equal to 64")] + fn test_append_word_count_too_large() { + let mut builder = NullBufferBuilder::new(0); + builder.append_word(u64::MAX, 65); + } + #[test] fn test_null_buffer_builder_is_valid() { let mut builder = NullBufferBuilder::new(0);