sdf-jkl commented on code in PR #11004:
URL: https://github.com/apache/arrow-rs/pull/11004#discussion_r3980189290
##########
parquet/src/encodings/encoding/alp_encoder.rs:
##########
Review Comment:
```suggestion
// 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.
```
We should change the doc comments around the impl to show that only a
nonempty flush builds the preset.
##########
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:
We can move this check above, and if the page is empty encode without
building a preset
##########
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:
```suggestion
assert_eq!(
after_empty, baseline,
"a leading empty page must not affect encoding of the first
nonempty page"
);
```
If the preset is deterministic, why not compare directly to the baseline?
--
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]
