Abdallah-Afifi commented on code in PR #11004:
URL: https://github.com/apache/arrow-rs/pull/11004#discussion_r3999094467
##########
parquet/src/encodings/encoding/alp_encoder.rs:
##########
@@ -703,8 +703,14 @@ where
None => {
let built = build_preset(values);
let page = encode_page(values, &built, scratch)?;
+ let had_values = !values.is_empty();
Review Comment:
Done — the empty case is its own match arm now, so `build_preset` isn't
called at all. `encode_page` only touches the preset inside the vector loop, so
the empty slice is fine.
##########
parquet/src/encodings/encoding/alp_encoder.rs:
##########
Review Comment:
Updated, thanks.
##########
parquet/src/encodings/encoding/alp_encoder.rs:
##########
@@ -1078,4 +1084,47 @@ 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!(
+ after_empty.len() < values.len() * std::mem::size_of::<f64>(),
+ "page after an empty first page ({} bytes) is no smaller than raw
f64 ({} bytes); \
+ the same values encoded as the first page take {} bytes",
+ after_empty.len(),
+ values.len() * std::mem::size_of::<f64>(),
+ baseline.len()
+ );
+
+ assert!(
+ after_empty.len() <= baseline.len() + baseline.len() / 10,
+ "empty first page poisoned the preset: {} bytes vs {} bytes when
encoded first ({:.1}x larger)",
+ after_empty.len(),
+ baseline.len(),
+ after_empty.len() as f64 / baseline.len() as f64
+ );
Review Comment:
Good call, they're byte-identical. Switched to `assert_eq!` — stricter and
much shorter.
--
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]
