[WIP][POC] Pfor encoding#50088
Draft
prtkgaur wants to merge 23 commits into
Draft
Conversation
Implements the PFOR (Patched Frame of Reference) integer compression algorithm as a standalone utility library in arrow/util/pfor/. Includes: - Cost model for optimal bit width selection (histogram-based) - Vector-level encode/decode with FOR + bit-packing + exceptions - Page-level wrapper with header, offset array, and multi-vector layout - Comprehensive unit tests covering edge cases and round-trips
Adds PFOR = 11 to the Encoding enum and wires it into the parquet read/write pipeline: - PforEncoder<DType> in encoder.cc (buffers values, calls PforWrapper::Encode) - PforDecoder<DType> in decoder.cc (decodes all values on first access) - PFOR case in column_reader.cc InitializeDataDecoder - Encoding string mapping in types.cc Supports INT32 and INT64 column types.
Benchmarks encode/decode throughput for int32/int64 across 10 data distributions inspired by Snowflake's NumericComprBenchmark: constant, sequential, small range, high-base-small-range (timestamps), with outliers (exception path), random, TPC-DS date/store/item/quantity keys. Each distribution runs at 1K/10K/100K/1M elements. Reports bytes/s, items/s, and compression ratio.
Load() now returns Result<PforVectorInfo> after the Status/Result refactoring. Use ASSERT_OK_AND_ASSIGN to properly unwrap the result in tests.
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
Make LoadHeader fallible: move the header-size check from Decode into LoadHeader, return Result<PforHeader>, and update Decode to use ARROW_ASSIGN_OR_RAISE. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
Replace std::memcpy / raw byte writes in PforWrapper::StoreHeader, LoadHeader, and the offset-array read/write paths with util::SafeLoadAs and util::SafeStore. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
Reject invalid packing_mode, value_byte_width mismatch, log_vector_size out of [kMin, kMax] range, and negative num_elements when loading the PFOR page header. Removes the redundant packing_mode and value_byte_width checks from Decode now that they live in LoadHeader. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
…sites Replace size_t with int64_t for max_size/comp_size to match the PforWrapper API signature, and qualify pfor::PforWrapper as ::arrow::util::pfor::PforWrapper to avoid ADL ambiguity.
Aligns with Arrow buffer conventions (Buffer::data() returns uint8_t*). Removes the reinterpret_cast<char*> at the parquet encoder/decoder call sites and switches std::vector<char> compressed buffers to std::vector<uint8_t> in the unit test and benchmark. Also fixes a pre-existing size_t / int64_t* mismatch in pfor_benchmark.cc that surfaced once the buffer pointer type was tightened. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
…th validation Per Google C++ style, replace the PforVectorInfo struct with a class that has private trailing-underscore members and getter/setter accessors. Replace std::memcpy calls in Store/Load and the exception patch loop in DecodeVector with util::SafeLoadAs / util::SafeStore. Add bit_width range validation inside Load() so callers don't have to repeat the check. Updates all access sites in pfor.cc and pfor_test.cc to go through the new accessors. Caches num_exceptions() in a local in DecodeVector so the #pragma GCC unroll can still see a constant loop bound. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
Per Google C++ style, both types become classes with private trailing-underscore members and const getters, mutable getters, and setters. Updates all access sites in pfor.cc (EncodeVector, LoadView, SerializedVectorSize, SerializeVector) and pfor_test.cc to go through the new accessors. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
…, use ctor in EncodeVector - Move the num_exceptions < 0 check from DecodeVector into PforVectorInfo::Load alongside the bit_width range check, so all loaded-data invariants are enforced at the same layer. - Use PforVectorInfo's parameterized constructor in EncodeVector instead of three separate setter calls on a default-constructed instance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how to contribute here:
Please remove this line and the above text before creating your pull request.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
This PR includes breaking changes to public APIs. (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)