[Parquet] ALP encoder/decoder support

[sdf-jkl]

Which issue does this PR close?

• Part of Implement ALP encoding #10541
• Closes [Parquet] Prototype ALP encoding #8748 .
• Related spec PR: GH-533: Add ALP (Adaptive Lossless floating-Point) encoding specification parquet-format#557

Rationale for this change

check issue

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[alamb]

amaaazing

[sdf-jkl]

@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.

[alamb]

Thank you -- I will put this on my short list to reivew I was out last week

[devanbenz]

A few comments

[sdf-jkl]

@devanbenz Sorry for taking so long to address your comments. I'll make sure to work on it this week!

[alamb]

The final spec is

• GH-533: Add ALP (Adaptive Lossless floating-Point) encoding specification parquet-format#557

[devanbenz]

Comment about header regarding publication of spec.

[sdf-jkl]

Thanks @devanbenz, I've finally addressed your reviews!

[devanbenz]

Thanks @devanbenz, I finally addressed your reviews!

Please cargo fmt the code 🫡

[sdf-jkl]

I swear I did 😿
Had to run

cargo fmt -p parquet --  --config skip_children=true `find ./parquet -name "*.rs" \! -name format.rs`

5e5c8d2

[sdf-jkl]

BTW I tested this branch with the file from the new parquet-testing repo and it seems to work well (no changes to the reader are needed 🎉 )

* [Add ALP test data parquet-testing#119](https://github.com/apache/parquet-testing/pull/119)

🚀

[alamb]

Update:

• I created a smaller parquet-testing file that exercises more corner cases here: apache/parquet-testing#119
• I started trying to consolidate the parquet reader / round trip tests: #10540

@sdf-jkl for this PR i would like to propose:

1. Update the tests to use apache/parquet-testing#119 (I will be working on getting some more consensus about that on the parquet mailing list)
2. Add a little more end to end test coverage (round trip more data through ALP and ensure the data comes through completely)

I am happy to work on the testing (and push directly to this branch) if that is ok with you, but I wanted to check with you first

Feedback addressed, re-review requested

[sdf-jkl]

Also, before we merge we should run llvm-cov and ensure nothing critical is uncovered.

Added more coverage here - 5a10e68

┌────────────────┬─────────┬───────────┬────────┐
│      File      │ Regions │ Functions │ Lines  │
├────────────────┼─────────┼───────────┼────────┤
│ alp.rs         │ 100%    │ 100%      │ 100%   │
├────────────────┼─────────┼───────────┼────────┤
│ alp_decoder.rs │ 98.80%  │ 96.97%    │ 99.26% │
├────────────────┼─────────┼───────────┼────────┤
│ alp_encoder.rs │ 97.68%  │ 94.20%    │ 98.37% │
└────────────────┴─────────┴───────────┴────────┘

[devanbenz]

I will have some time to review this tomorrow or friday FYI :)

[alamb]

I merged up from main to get the most recent changes, and I am now reviewing this one more time

[alamb]

The new clippy lints claim this value is out of the precision range of f32 (and similarly for the others)

I did what it told me in 68010c9

   --> parquet/src/encodings/alp.rs:496:40
    |
496 |     const ENCODING_UPPER_LIMIT: Self = 2147483520.0;
    |                                        ^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.97.0/index.html#lossy_float_literal
    = note: requested on the command line with `-W clippy::lossy-float-literal`
help: consider changing the type or replacing it with
    |
496 -     const ENCODING_UPPER_LIMIT: Self = 2147483520.0;
496 +     const ENCODING_UPPER_LIMIT: Self = 2_147_483_500.0;
    |

[alamb]

Thank you @sdf-jkl and @devanbenz -- I went through this some more and I think it is really nice. THank you so much.

I will wait for @devanbenz 's review as well, but I think this is now ready to merge once the upstream parquet-testing PR has merged

👏 -- I think we will have the first open source ALP implementation delivered

In terms of next steps, I plan to focus on the blog post next

• apache/parquet-site#195

[alamb]

I asked claude about this -- it says this trait is needed because

Since get_encoder<T: DataType> is generic, every arm of its match must type-check for every T it's monomorphized with. So this doesn't compile:

  Encoding::ALP => match T::get_physical_type() {
      Type::FLOAT | Type::DOUBLE => Box::new(AlpEncoder::new()), // error: `i32: AlpFloat` not satisfied
      ...
  }

Because we basically need to only invoke this for certain T types (f32 and f64)

[alamb]

Here are some follow ups suggested by claude

1. If the first data page of a column chunk has zero non-null values, build_preset(&[]) fixes the scale at 10^0, so every non-integer value in all later pages of that chunk becomes an exception

Since an all-null leading page is a realistic input, this is probably something we should fix (as a follow on PR)

2. parquet-rewrite CLI (parquet/src/bin/parquet-rewrite.rs:147) doesn't expose ALP as an encoding option.

[sdf-jkl]

Before we proceed to the benchmarks for the blog, should we work on some follow up Perf 🚀🚀🚀 PRs?

I have a related PR for improving bit packing speed. I started working on it to bring it to ALP later.

• [Parquet] Vectorise Pack Bits within BitWriter #10432

[alamb]

Before we proceed to the benchmarks for the blog, should we work on some follow up Perf 🚀🚀🚀 PRs?

I have a related PR for improving bit packing speed. I started working on it to bring it to ALP later.

• [Parquet] Vectorise Pack Bits within BitWriter #10432

Well, clearly having better performnce for the blog would be good :) But at the moment I have no idea where we stand compared to ZSTD in the rust implementation so I would probably build the benchmark scripts first and then decide if we needed to optimize more before publishing

It would also be interesting to show the effect of hardware (aka can this implementaton take advantage of SIMD instructions)

[sdf-jkl]

It would also be interesting to show the effect of hardware (aka can this implementaton take advantage of SIMD instructions)

I was trying to follow the ALP spirit as much as possible and make everything vectorizable. Bit packing should be the last thing not vectorized yet.

[sdf-jkl]

Well, clearly having better performnce for the blog would be good :) But at the moment I have no idea where we stand compared to ZSTD in the rust implementation so I would probably build the benchmark scripts first and then decide if we needed to optimize more before publishing

The results would be dataset dependent. Not sure we can get a reliable benchmark using random generated ones.

[alamb]

The results would be dataset dependent. Not sure we can get a reliable benchmark using random generated ones.

Yes for sure -- I think we can start with using data from the paper as a starting point (they list several datasets)

[sdf-jkl]

Yes for sure -- I think we can start with using data from the paper as a starting point (they list several datasets)

Some of them are here - https://github.com/cwida/ALP/tree/main/benchmarks

I'll work on the benchmarks PR

[alamb]

Yes for sure -- I think we can start with using data from the paper as a starting point (they list several datasets)

Some of them are here - https://github.com/cwida/ALP/tree/main/benchmarks

I'll work on the benchmarks PR

I think there is two usecases:

1. A benchmark for the blog post (probably we can create this with a custom CLI / repo, though it could be a PR we never merge in this repo too)
2. A benchmark (cargo bench style) for optimizing performance in this repo

[devanbenz]

Just the one comment, spent the morning looking over this PR and playing around with it locally. It is a very large body of work, looks good to me. My comment is non-blocking but maybe something to look in to.

[devanbenz]

Can this ever be called when an encoder has no values set or null value? For example, if you create a new encoder with AlpEncoder::<DoubleType>::new() and call flush_buffer all while later pages have well behaved data. Will it cause issues? Could you add a test for this if it's possible?

[devanbenz]

Maybe something like:

#[test]
fn test_empty_first_page() {
    let mut encoder = AlpEncoder::<DoubleType>::new();
    // First page flushed with no values
    let empty = encoder.flush_buffer().unwrap();
    assert_eq!(empty.len(), ALP_HEADER_SIZE);

    // Later pages carry well-behaved decimal data.
    let values: Vec<f64> = (0..1500).map(|i| (i as f64) * 0.01).collect();
    encoder.put(&values).unwrap();
    let page = encoder.flush_buffer().unwrap();

    assert!(page.len() < values.len() * 8);
}