[Parquet] ALP encoder/decoder support - #9372
Conversation
|
amaaazing |
|
@alamb I worked with codex on replicating the c++ implementation reviewing commit by commit. I'll do one final read myself, but this should be ready for initial review. |
|
Thank you -- I will put this on my short list to reivew I was out last week |
|
@devanbenz Sorry for taking so long to address your comments. I'll make sure to work on it this week! |
devanbenz
left a comment
There was a problem hiding this comment.
Comment about header regarding publication of spec.
|
Thanks @devanbenz, I've finally addressed your reviews! |
Please cargo fmt the code 🫡 |
|
|
|
Thanks @sdf-jkl -- I will try and find tiume to review this (probably not until the weekend though as I have to sort out the next object store release before |
StreamingPage<F> encodes pages 2..N vector-by-vector as values arrive via put(), holding only a <1024 carry plus the compressed body instead of buffering the whole page. Page 1 still buffers to build the preset. Output is provably byte-identical to the buffered encoder (pinned by test_streaming_matches_buffered / test_streaming_irregular_puts). Adds an #[ignore]d rdtsc throughput bench (mod throughput) reporting cyc/val for encode buffered/streaming and decode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@alamb @devanbenz PTAL ( ~1400 locs are tests, so it's less scary than it looks like 😃 ) We might want to push apache/parquet-testing#105 first ... |
alamb
left a comment
There was a problem hiding this comment.
I spent a bunch of a plane ride today reviewing this PR. I found it really easy to read, understand and follow. Very Nice work @sdf-jkl and @devanbenz
Only thing required: a few more tests
The only thing I think we need is some more testing, focused on end-to-end and round trips
- Tests to read the other ALP files that are in apache/parquet-testing#105
- Add some round trip tests for the ALP csv files (so we are testing encode path across more values)
Also, before we merge we should run llvm-cov and ensure nothing critical is uncovered.
All in all I can't emphasize enough how nice it was to read.
I will work on getting apache/parquet-testing#105 ready to merge, as you suggest
Performance
I am sure we can improve the performance in subsequent iterations, but I think we should focus on getting this implementation in first, and then collect follow on performance improvements in other tickets
High level comments:
The plumbing looks good, though I wonder if we really need the new GetEncoder trait
Ideas for follow on work (perhaps I can file a follow on epic)
- Add a benchmark (perhaps bsed on the paruqet-testing dataset...) as well (as a separate PR) -- QA for performance
- Support multiple vector lengths (now the encoder only supports 1024 element vectors
- QA for encoding effectiveness: To test the effectiveness of the sampling / compression heuristics, we could brute force try different exponent / factor combinations for different vectors and ensure the exponent / factor chosen by the algorithm is the best (empirically verify)
| 0.000000000000000001, | ||
| ]; | ||
|
|
||
| pub(crate) trait AlpFloat: |
There was a problem hiding this comment.
I think we should add some comments on the AlpFloat trait explaining that it represents the floating point representation and the there is a corresponding AlpExact for each AlpFloat and cross link it. Otherwise I was confused at first trying to understand what the difference was
| /// for negative `x` too. Values large enough to be mis-rounded just fail | ||
| /// the caller's round-trip check and become exceptions. The add/sub must | ||
| /// not be simplified away: it *is* the rounding. | ||
| fn fast_round(self) -> <Self::Exact as AlpExact>::Signed; |
There was a problem hiding this comment.
I think we could probbaly provide some more rationale about using fast_round before getting into the binade details. For example, perhaps we can explains that it is a way to round quickly using floating point math in a way that maximizes chance that multplying / dividing by factor and exponent will result in the same value (and thus avoiding exceptions)
| ); | ||
| assert_eq!(actual.num_rows(), expected.num_rows(), "row count mismatch"); | ||
|
|
||
| for col_idx in 0..actual.num_columns() { |
There was a problem hiding this comment.
This is a lot of code to compare record batches and it will be much slower than the normal kernels.
I recommend using normal record batch comparison here rather than explicit row-by-row (aka assert_eq!(expected == actual)
| #[test] | ||
| fn test_read_f32_alp() { | ||
| let data_dir = PathBuf::from(parquet_test_data()); | ||
| let parquet_path = data_dir.join("alp_float_arade.parquet"); |
There was a problem hiding this comment.
Also, I think we should test reading all the example ALP files (not just alp_float_arade.parquet)
| /// larger than PLAIN here, a property of the data rather than the encoder. | ||
| /// ALP's win on arade in the paper is on the `f64` version of the dataset. | ||
| #[test] | ||
| fn test_write_f32_alp_roundtrip() { |
There was a problem hiding this comment.
As this is writing data, I think it would make make sense to move it to the arrow_writer tests either in parquet/tests/arrow_writer.rs or in parquet/src/arrow/arrow_writer/mod.rs. parquet/src/arrow/arrow_writer/mod.rs is probably the most consistent but it is already a massive module 🤔
In general I like the test pattern of round tripping the data used for alp encoding (aka read the csv and then round trip it through parquet with ALP enabled) to increase coverage. We could both:
- Verify correctness
- Compare compression (data page size) and that it was comparable to the checked in results
| let actual = read_parquet_bytes(alp_bytes); | ||
| assert_eq!(actual.num_rows(), expected.num_rows(), "row count mismatch"); | ||
|
|
||
| for col_idx in 0..expected.num_columns() { |
There was a problem hiding this comment.
this comparison logic I think can be avoided if we just compare the record batches
| /// unencoded value counts differ from the level count in two different ways at | ||
| /// once. | ||
| #[test] | ||
| fn test_alp_roundtrip_page_versions_with_nulls() { |
There was a problem hiding this comment.
Likewise this appears to a roudnteip writer test, so it probably makes sense to move it with the other round trip writer tests
| /// floating-point columns. | ||
| /// | ||
| /// [`ParquetValueType`]: crate::data_type::private::ParquetValueType | ||
| pub trait GetEncoder { |
There was a problem hiding this comment.
Why do we need a new GetEncoder trait? I think the ColumnDescPtr physical_type.logical_type
For example the fallback encoder depends on type
https://github.com/apache/arrow-rs/blob/8042ea288e084107b602f9e25a850314942567b6/parquet/src/column/writer/mod.rs#L1735-L1734
Alternately, maybe we could pass a &DataType as a parameter into the system
| // positional overwrite, so it is independent of exception ordering. | ||
| let lo = cur.delivered; | ||
| let hi = cur.delivered + out.len(); | ||
| for (pos_chunk, value_chunk) in cur |
There was a problem hiding this comment.
In this code, would it make sense to check / assert that the chunks_exact has no remainder (aka debug assert that remainder is null?)?
Which issue does this PR close?
Rationale for this change
check issue
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?