[WIP][POC] Pfor encoding - #50088
Draft
prtkgaur wants to merge 64 commits into
Draft
[WIP][POC] Pfor encoding#50088prtkgaur wants to merge 64 commits into
prtkgaur wants to merge 64 commits into
Conversation
|
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
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 - Round-trip tests for int32 and int64 at the vector and page level, covering outliers, all-identical and all-zero runs, negative values, the int min/max edges, a single element, and a page whose length is an exact multiple of the vector size
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.
Defaults to kPforVectorSize, so existing callers are unaffected.
Load() now returns Result<PforVectorInfo> after the Status/Result refactoring. Use ASSERT_OK_AND_ASSIGN to properly unwrap the result in tests.
prtkgaur
force-pushed
the
pfor-encoding
branch
3 times, most recently
from
October 2, 2026 22:46
9aa7578 to
d179a6e
Compare
Make LoadHeader fallible, use SafeLoadAs and SafeStore for header and offset fields, and validate the packing mode, element width, vector size, and element count at the page boundary.
Use int64_t for encoded sizes, uint8_t for byte buffers, and fully qualified PFOR names at Parquet call sites. Remove the pointer casts made unnecessary by the consistent buffer type.
Convert the vector metadata and encoded-vector types to classes with explicit accessors. Validate widths and exception counts while loading metadata, and construct metadata atomically on encode.
Compiles the PFOR sources into the parquet target and adds the parquet-pfor-comparison-benchmark executable.
Replaces hand-rolled power-of-two and ceiling-division arithmetic with bit_util::IsPowerOf2 and bit_util::CeilDiv, and records where incremental encode and decode would hook in.
Commit 00b6318 introduced ARROW_DCHECK(bit_util::IsPowerOf2(vector_size)) in PforWrapper<T>::Encode, but vector_size is int32_t and bit_util has overloads only for int64_t and uint64_t -- the call is ambiguous and the file no longer compiles. Cast to int64_t to disambiguate. CeilDiv calls in the same file already promote to int64_t implicitly via its int64_t-only signature.
Implement the lane-interleaved 1024-value layout with compiler-vectorizable loops and a full-width memcpy path. Add a frame-of-reference wrapper, tests, and comparison-benchmark registrations.
DecodeFlat unpacks into a transposed scratch buffer per chunk and then scatters via fromTransposed32, so output[i] == input[i]. This is the FL_ORDER inverse of the gather step in Encode, which makes FastLanes directly comparable against PFOR and DeltaBitPack, both of which produce flat output. A round-trip test covers 4 chunks of random data. Decode on aarch64, 102400 int32, 3-run median: Dataset FL transposed FL flat PFOR Delta EventDate 20 us 108 us 38 us 123 us EventTime 23 us 113 us 57 us 140 us GoodEvent 20 us 107 us 35 us 119 us The transposed-kernel decode beats every other codec in the harness by 1.5-7x. The flat path pays roughly 85 us of scatter per 100K values, which leaves it behind PFOR and still ahead of DeltaBitPack. That gap is the FL_ORDER permutation alone, which is why the paper's architecture keeps data in transposed order through the query.
Add toTransposed32 as the inverse of fromTransposed32 and correct the mapping documentation. The two transforms are mutual inverses, not self-inverse.
Pass nonzero frames to unpack_bias so PFOR avoids a second output traversal. Use Arrow's portable restrict qualifier to preserve vectorization on supported compilers.
Remove a redundant move and propagate the status returned by PforWrapper::Decode.
MakeEncoder and MakeDecoder already accept it, so the omission failed the SupportedEncodings consistency tests.
std::bit_width is 0 for a 0 input, so the explicit zero guards in BitsRequired go away too.
Also silences a cpplint false positive on the semicolon that closes a requires-expression initializer.
An element count over the output capacity, or an offset array or vector offset past the end of the page, no longer reads or writes out of bounds.
Every element of a maximum-size vector can be an exception, so the count reaches 32768, one past what the signed 16-bit field held.
Decode took its output first while Encode took its output last; both now read input, size, then output.
The preconditions were debug-only assertions, so a release build encoded with a vector size the page header cannot describe and said nothing.
Nullable INT32/INT64 columns raised NotImplemented; the encoder already writes non-null values densely, so spread them over the valid runs.
Both sides buffer a whole page today; say in each TODO that the streaming rework is a follow-up change rather than an open question.
They named bits 0..5 and reserved 6..7, contradicting both kBitWidthMask and the class doc four lines above.
Validate exception positions, counts, vector sections, and output bounds before decoding. Clear cached page state when a decoder receives new input so values cannot leak across pages.
pfor.cc and pfor_wrapper.cc were in no library source list, so the test, the benchmark and libparquet each compiled their own copy.
It declared its own data_ and never called Base::SetData, so len_ stayed 0 and a full-page Decode now unpacks into the caller's buffer directly.
Derive stored sizes from their field types, remove the unused encoded-vector view, and validate that serialized vector metadata matches its payload sections.
An incompressible column encodes to its plain size plus the per-vector metadata, and only ColumnWriterImpl can relabel the page.
43 tests become 80. Ten cases that only ran at 32 bits now run at 64, which caught corrupt-page tests whose outlier the cost model declined to patch once an exception cost a 64-bit value.
A parquet page header counts nulls, and a PFOR page stores only the non-null values, so the level count the reader hands SetData is an upper bound on the values in the payload rather than the number of them. The decoder was using it as both: as the amount to decode and as the amount of decoded output to hand back, which for any nullable column decoded past the end of what the page actually holds. Read the count from the page header instead, where the encoder wrote it, and keep the level count only as the bound it is. Decoding a whole page in one call still writes straight into the caller's buffer; a partial read now decodes the page once into a pool-backed scratch buffer and serves the rest of the batch from it. DecodeArrow reserves once and then either advances over the values it just wrote or expands them leftward into their null positions, which drops the separate null-aware copy loop.
Two ways a malformed page got through. A header count short of the caller's capacity filled part of the output buffer and returned OK, leaving the rest of it holding whatever was there before; the count is now required to equal the expected value count, not merely fit inside it. And the offsets were each checked in isolation, only for being inside the buffer, so a page whose offsets overlap or run backwards decoded part-way and emitted values built out of another vector's bytes. Check the array as a chain up front instead: the first offset lands just past the array, each later one is strictly greater than the one before it, and all of them are inside the payload.
An all-null optional page buffers no values and is still written, and the empty payload the encoder emitted for it has no header for the reader to load, so reading such a column threw. Zero values now writes the header and nothing after it, and the wrapper accepts that count on both sides. The encoding test that covered this passed a level count of zero to SetData, which no real reader does -- the count it passes includes the nulls. It now passes the full count, and the writer tests round-trip an optional PFOR column with some nulls and with nothing but nulls.
Every multi-byte wire field -- the header's element count, the offset array, each vector's frame of reference and exception count, and the exception positions and values -- now converts explicitly, so the big-endian static_assert is gone. The bit-packed deltas already needed no conversion: BitWriter writes them little-endian and the unpacker reads them back the same way, so those bytes are copied verbatim.
The CMake build compiles them and runs pfor-test; meson knew about neither. The two translation units carry the only definitions of the PforWrapper and PforCompression instantiations parquet's encoder and decoder call, so a meson build has nothing to link them against.
Record that partial reads currently decode a complete page into scratch space, identifying the allocation that a vector-at-a-time reader should replace.
Everything PFOR had so far drove the encoder and decoder classes directly, so nothing covered the path a real reader takes: writer properties, page headers, row group boundaries, null expansion. These 19 tests write real files and read them back, over the distributions the encoding is built for (a tight cluster far from zero, a constant vector, a cluster with outliers), the ones it has to survive (values at the bounds of each type, all-null and leading-null columns, row groups that end mid-vector, batches smaller than a page), and each supported compression codec. Every round trip also asserts the file's column chunks report PFOR. Without that a round trip passes just as happily when the writer picks some other encoding, and the test would be checking the default instead. arrow_reader_writer_test.cc is already long, so these go in arrow/arrow_encoding_test.cc, which later encodings can share.
The names now say what the install rules already do: arrow_install_all_headers globs a single directory and skips anything matching "internal", so nothing under arrow/util/pfor is installed and none of it carries a backward-compatibility obligation.
DecodeVector left UnpackOptions::max_read_bytes at its -1 default, which tells the bit-unpacker it may not read a byte past the vector's own packed payload. The vector kernels load a fixed-size window per step, wider than a step consumes at most bit widths, so under that bound the last step is refused and the tail of the vector falls to the scalar epilog -- 32 of 1024 values at bit width 3. The span DecodeVector is handed runs from this vector to the end of the page, so the true bound is already in hand; pass it. Whole-page int32 decode gains a median 1.13x over a 29-column corpus and 1.9x on the columns that pack to 3 bits, and nothing at widths 1, 2, 4, 8, 16 and 31, whose kernels strand no values to begin with. The gain is flat from a 78 KiB data page to a 4 MB destination, which is what a per-vector cost looks like.
The tests and benchmarks drew their section headings as boxes: a row of equals signs above the title and, in the benchmark and integration files, a second one below. Arrow writes a single rule of seventy dashes and then the title, in 121 places under cpp/src; boxed headings appear in seven files, four of them vendored. This converts twenty-nine headings to the house rule and drops the twelve closing rules. Two comments also shouted: three spellings of "ARE the signed values" in the decoder, and the bias comment in the bit-packing kernel announcing "The whole point:" before making it. One more narrated the branch's own history, calling the second add-FOR pass what the code "used to do". The cost that pass measured is why the add is folded into the unpacker, so the figures stay and the history goes. Comments only. Stripping comments leaves all six files byte-identical to their previous revision, clang-format reports no new warnings, and no line crosses ninety columns.
Add a validated VectorReader over the page offset array and replace the page-sized decode cache with a reusable one-vector cache. Track page consumption across dense and spaced reads and validate negative arguments.
prtkgaur
force-pushed
the
pfor-encoding
branch
from
October 3, 2026 01:25
d179a6e to
1298e2b
Compare
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.
Doc : https://docs.google.com/document/d/1ZZOtxmq6K8pNU0npijfSglTJVkspXL5GLDKPSGj9HlA/edit?tab=t.0
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.)