This is an automated email from the ASF dual-hosted git repository.
sdf-jkl pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new fbdbf9367a fix(parquet): do not cache an ALP preset built from an
empty first page (#11004)
fbdbf9367a is described below
commit fbdbf9367a260090c07381c95c80d9bee456d979
Author: Abdallah Afifi <[email protected]>
AuthorDate: Tue Sep 15 00:13:53 2026 +0300
fix(parquet): do not cache an ALP preset built from an empty first page
(#11004)
# Which issue does this PR close?
- Closes #10644.
# Rationale for this change
When a column chunk's first data page has no values, `build_preset`
returns the fallback pair (exponent 0, factor 0). `flush_buffer` cached
that for the whole chunk, and `select_params` short-circuits on a
single-candidate preset, so every later page was stuck on an integer
scale and every fractional value became an exception.
It's reachable from the normal write path, not just the encoder API: the
column writer counts levels rather than values, so a first page of all
nulls flushes an empty buffer.
Encoding 3000 values of `i * 0.01` (raw f64 would be 24,000 bytes):
| | before | after |
| --- | --- | --- |
| values as the first page | 3,808 | 3,808 |
| values after an empty first page | 31,258 | 3,808 |
Before the fix the chunk ends up bigger than leaving it unencoded.
After, a leading empty page costs only its 7-byte header.
# What changes are included in this PR?
`flush_buffer` only caches the preset when the page had values. `put`
already branches on whether the preset is set, so the next page with
values builds it.
The issue asks only for regression coverage and @alamb suggested the fix
as a follow-up — this has both. Happy to split them if you'd rather.
# Are these changes tested?
`test_empty_first_page_does_not_poison_preset`, which fails on main.
Full `cargo test -p parquet` passes (1558 tests); fmt and clippy clean.
One thing I'd like a second opinion on: with the preset left unset, an
all-null first page no longer arms the streaming path, so the second
page takes the buffered path. The tests agree that's correct, but it's
the part of this I'm least certain about.
---
Per the AI policy in CONTRIBUTING.md: I used Claude Code to trace the
encoder and draft the fix and test. I've reviewed all of it and verified
the behaviour myself — the numbers above are measured locally on this
branch and on main.
---------
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
parquet/src/encodings/encoding/alp_encoder.rs | 38 +++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/parquet/src/encodings/encoding/alp_encoder.rs
b/parquet/src/encodings/encoding/alp_encoder.rs
index 0a6be5dfb1..95c3a55c84 100644
--- a/parquet/src/encodings/encoding/alp_encoder.rs
+++ b/parquet/src/encodings/encoding/alp_encoder.rs
@@ -697,9 +697,13 @@ where
streaming,
} = self;
- // The first flush builds the preset from the whole buffered page and
- // encodes it in one pass; that also arms streaming for later pages.
+ // The first nonempty flush builds the preset from the whole buffered
+ // page and encodes it in one pass; that also arms streaming for later
pages.
let page = match preset {
+ // Nothing to sample, so no preset to build. Leaving it unset
keeps the
+ // chunk off the fallback parameters, which would make every later
+ // fractional value an exception.
+ None if values.is_empty() => encode_page(values, &[], scratch)?,
None => {
let built = build_preset(values);
let page = encode_page(values, &built, scratch)?;
@@ -1078,4 +1082,34 @@ mod tests {
// round-trip proves the page survives both paths losslessly.
assert_bits_eq(&roundtrip::<DoubleType>(&values), &values);
}
+
+ /// An empty first data page must not pin the chunk's preset to exponent 0
/
+ /// factor 0: `flush_buffer` caches the first page's preset for the whole
+ /// chunk, so a degenerate one makes every later fractional value an
exception.
+ #[test]
+ fn test_empty_first_page_does_not_poison_preset() {
+ let values: Vec<f64> = (0..3000).map(|i| (i as f64) * 0.01).collect();
+
+ // Baseline: the same values encoded as the first page of a chunk.
+ let mut baseline_encoder = AlpEncoder::<DoubleType>::new();
+ baseline_encoder.put(&values).unwrap();
+ let baseline = baseline_encoder.flush_buffer().unwrap();
+
+ // The same values, but preceded by an empty first page.
+ let mut encoder = AlpEncoder::<DoubleType>::new();
+ let empty = encoder.flush_buffer().unwrap();
+ assert_eq!(
+ empty.len(),
+ ALP_HEADER_SIZE,
+ "an empty page should be header-only"
+ );
+
+ encoder.put(&values).unwrap();
+ let after_empty = encoder.flush_buffer().unwrap();
+
+ assert_eq!(
+ after_empty, baseline,
+ "a leading empty page must not affect encoding of the first
nonempty page"
+ );
+ }
}
