[Experimental][WIP] Pfor delta encoding - #51150
Draft
prtkgaur wants to merge 84 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.
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.
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.
Portable C++ port of FastLanes (Afroozeh & Boncz, VLDB '23) for int32_t
columnar data. No SIMD intrinsics in the kernels — the inner lane loop
is structured (contiguous loads from packed[w*kLanes + lane], contiguous
stores to transposed[r*kLanes + lane]) so the compiler auto-vectorizes
to 4-wide NEON / 8-wide AVX2 / 16-wide AVX512 without source changes.
Layout: lane-interleaved 1024-bit format per the paper. 1024 values
pack as w u32 rows of 32 u32 lanes. FL_ORDER (8x16 -> 16x8 sub-block
transpose + 3-bit-reversal sub-block reorder) is applied OUTSIDE the
kernel: FastLanesForCodec::Encode gathers input[fromTransposed32(t)]
before packing; Decode produces output in transposed order (no scatter,
output[t] == input[fromTransposed32(t)] + min within each 1024-block).
FastLanesForCodec adds Frame-of-Reference on top:
- 2048-value chunks (2 FastLanes blocks per chunk)
- Per-chunk 5-byte header: [min(4B int32 LE)] [bit_width(1B)]
- Subtract min before packing; add back on decode
- bit_width=0 path stores no payload (constant chunk)
Files:
cpp/src/arrow/util/fastlanes/fastlanes_kernels.h
- PackBlock<W>(in, out) / UnpackBlock<W>(packed, out)
- W=32 fast path: std::memcpy
- fromTransposed32 helper
cpp/src/arrow/util/fastlanes/fastlanes_for.{h,cc}
- FastLanesForCodec::{Encode,Decode}
cpp/src/arrow/util/fastlanes/fastlanes_for_test.cc
- 5 round-trip tests (narrow range, single value, full int32 range,
multiple chunks, boundary values) — all passing
CMakeLists.txt wires the test as arrow-fastlanes-for-test.
Wires the new FastLanesForCodec into the existing pfor_comparison_benchmark harness alongside PFOR, DeltaBitPack, ZSTD, LZ4, RleBitPack, and Bss codecs. New BM_FastLanesEncode / BM_FastLanesDecode functions follow the same Gen32 + ::Apply(CustomArgs) shape; REGISTER_DATASET macro picks them up for every ClickBench dataset. Notes on the comparison: - FastLanes decoder produces output in TRANSPOSED order (output[chunk*2048 + block*1024 + t] == input[chunk*2048 + block*1024 + fromTransposed32(t)] + min). PFOR/DeltaBitPack produce flat output. The benchmark measures decoder throughput head-to-head; consumers of FastLanes output must be permutation-aware (which is the FastLanes paper's intended architecture). - num_values is rounded down to a multiple of 2048 (FastLanes chunk size) inside BM_FastLanesEncode / BM_FastLanesDecode for compatibility with the existing 102400-value test sizes. Also guards add_executable(parquet-pfor-comparison-benchmark) with if(ARROW_BUILD_BENCHMARKS) so non-benchmark configurations don't fail the cmake configure step. Bench numbers on aarch64 (102400 int32, 3-run median): EventDate decode: FastLanes 20us vs PFOR 36us vs Delta 122us EventTime decode: FastLanes 24us vs PFOR 56us vs Delta 140us GoodEvent decode: FastLanes 20us vs PFOR 34us vs Delta 119us Compression ratios match or slightly beat PFOR on every dataset tested.
FastLanesForCodec::DecodeFlat unpacks into a transposed scratch buffer
per chunk and then scatters via fromTransposed32 to produce output in
original input order — output[i] == input[i] for the encoded input.
This is the FL_ORDER inverse of the gather step in Encode.
Adds:
- DecodeFlat method + round-trip test (DecodeFlatIsIdentity) covering
4 chunks of random data. All 6 round-trip tests still pass.
- BM_FastLanesDecodeFlat in pfor_comparison_benchmark, registered in
the per-dataset macro for apples-to-apples vs PFOR / DeltaBitPack
(both of which produce flat output).
Bench (102400 int32, 3-run median, aarch64):
Dataset FL Decode FL DecodeFlat PFOR Decode Delta Decode
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 by 1.5-7x. The
flat-output decode pays an ~85 us scatter cost per 100K values that
makes it slower than PFOR but still faster than DeltaBitPack. The gap
is exactly the FL_ORDER scatter — the reason FastLanes' intended
architecture keeps data in transposed order through the query.
The 8x16 -> 16x8 within-sub-block transpose is mutual-inverse with the 16x8 -> 8x16 transpose, NOT self-inverse. The previous docstring on fromTransposed32 said "Self-inverse: fromTransposed32 is also toTransposed32" — that was wrong. fromTransposed32(fromTransposed32(t)) does not equal t in general; e.g. fromTransposed32(1) = 16, fromTransposed32(16) = 2. Add the actual toTransposed32 (forward-direction mapping) and fix the docstring. Callers that need to invert a gather computed with fromTransposed32 (i.e. read out[i] = transposed["the t whose fromTransposed32(t) = i"]) must use toTransposed32(i).
Adds an additive packing-mode option to PFOR. Existing vectors round-trip
unchanged (default PackingMode::BitPack); new vectors can opt in to the
FastLanes lane-interleaved bit-packing layout via the per-vector flag.
On-disk format change (backwards-compatible):
- The 1-byte bit_width field of PforVectorInfo now packs two values:
bits 0..5 = the actual bit width (range 0..32 fits in 6 bits)
bit 7 = packing-mode flag (0 = BitPack, 1 = FastLanes)
bit 6 = reserved
- Legacy encoders only wrote the bit width, leaving high bits clear,
so they decode as PackingMode::BitPack via the new Load.
- PFOR header (page-level) is unchanged.
API:
- New enum class arrow::util::pfor::PackingMode { BitPack, FastLanes }.
- PforVectorInfo gains a packing_mode field and getter/setter.
- PforCompression<T>::EncodeVector takes an optional PackingMode (default
BitPack). FastLanes mode is only honored when num_elements equals the
FastLanes block size (1024) and T is 32-bit; otherwise it falls back
to BitPack per-vector (so tails and 64-bit values continue to work).
- PforCompression<T>::DecodeVector reads the per-vector flag and
dispatches between arrow::internal::unpack and the FastLanes kernel.
- PforWrapper<T>::Encode takes an optional PackingMode threaded down to
EncodeVector.
Decode-side perf (fused gather + FOR-add + SafeCopy):
The FL_ORDER inverse needs toTransposed32(i) — note: NOT
fromTransposed32(i), the two are mutual inverses, not self-inverse.
The scalar gather over can't be SIMD-vectorized, so
PFOR+FastLanes decode is ~2-3x slower than PFOR+BitPack end-to-end
despite the kernel itself being competitive. The win is only available
when the downstream consumer can work with data in FastLanes transposed
order (i.e. relax the flat-output contract).
Tests: 5 new tests in PforPackingModeTest cover round-trip identity for
both modes, the partial-tail fallback to BitPack, mixed-mode round-trip
through PforWrapper, and the bit_width=0 (constant vector) path. All 30
PFOR tests pass.
Benchmark: BM_PforFastLanesEncode / BM_PforFastLanesDecode added to
pfor_comparison_benchmark.cc, registered per dataset alongside the
existing 8 codec variants.
For FastLanes-encoded vectors the decoder previously always paid a
1024-element scalar FL_ORDER gather to produce flat output. That gather
is what made pfor+fastlanes 2-3x slower than pfor+bitpack overall, even
though the FastLanes unpack kernel itself is competitive.
The FastLanes paper's intended decode path is to NOT do that scatter at
all: keep the data in FastLanes stream order and let downstream
operators be permutation-aware (apply fromTransposed32 lazily, when
they need original index). This commit exposes that path.
API:
- New enum class arrow::util::pfor::OutputOrder { Flat, Transposed }.
- PforCompression<T>::DecodeVector and PforWrapper<T>::Decode take an
optional OutputOrder (default Flat, backwards-compatible).
- OutputOrder::Transposed only affects FastLanes-encoded vectors.
BitPack vectors have no permutation to skip, so they always produce
flat output regardless of the argument (mixed pages with a BitPack
tail end up flat in the tail, transposed in the full blocks).
Decoder paths in DecodeVector when packing_mode == FastLanes:
- Flat (existing): unpack -> scratch transposed[] -> fused
values[i] = SafeCopy(transposed[toTransposed32(i)] + FOR)
The toTransposed32 gather is scalar, breaks auto-vec.
- Transposed (new): unpack -> scratch transposed[] -> sequential
values[t] = SafeCopy(transposed[t] + FOR)
Pure sequential read/write, auto-vectorizes cleanly. Exceptions are
patched at toTransposed32(pos) so the stored-flat positions land in
the right transposed slots.
Tests: 4 new tests in PforOutputOrderTest cover (a) transposed output
satisfies the FL_ORDER relation, (b) manual inversion of the
permutation reconstructs the input, (c) BitPack vectors ignore the
Transposed request, (d) wrapper-level transposed decode across many
vectors. All 34 PFOR tests pass.
Benchmark: BM_PforFastLanesDecodeTransposed added, registered per
dataset. On 18 ClickBench-style datasets (102400 int32 each):
pfor+bitpack 33-57 us
pfor+fastlanes (flat) 98-108 us (0.34-0.53x — slower)
pfor+fastlanes (transp) 20-23 us (1.6-2.5x faster than bitpack)
The transposed path beats every other codec measured in the comparison
benchmark on every dataset.
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.
Positions came off the wire and indexed the output buffer directly, so a corrupt page wrote past its end; num_exceptions was unchecked against it too.
A decoder is reused across the data pages of a column chunk, and a non-empty decoded_values_ marked the page as already decoded, so page 2 returned page 1.
LoadView now makes the same checks DecodeVector does, and GetMaxCompressedSize and SerializeVector return Result so their sizes are enforced, not assumed.
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.
kHeaderSize and kVectorInfoSize were written out by hand; static_asserts now tie them to the members, and the exception count has a named wire type.
PforEncodedVectorView was reachable only from its own test and duplicated DecodeVector's validation. SerializeVector now rejects a vector whose info disagrees with the sections it carries.
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.
A caller that reads a page in pieces decodes all of it up front, and the page layout can do better than that. Say so where the scratch buffer is, so the next reader of this code does not have to work it out.
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.
PFOR has always used the minimum as its frame of reference, which makes every exception an overshoot: one value far below the cluster drags the packed window down and nothing can patch it back. And it has only ever packed values, so a column whose values are spread but whose steps are small had no representation that fit it. Both are encoder-side choices, so the cost model now makes them per vector: whether to difference the values first, and where to put the frame. The frame becomes any lower bound rather than the lowest, which costs nothing on the wire -- the field already holds a full-width value and the decoder only ever adds it -- and lets a value below the window fail the same unsigned mask test as one above it, so patching works on both sides with no second test and no sign to track. The delta flag is a bit of its own in the bit-width byte rather than another code in a mode field, because differencing is orthogonal to how the payload is laid out and the two have to be able to combine. A delta vector carries one extra full-width field, its first value, which is what keeps it decodable without the vector before it -- storing the first value as an exception instead would cost more (a position and a value) and only conditionally. Measured against the previous encoder over 18 distributions at n=102400, int64: the sawtooth packs 16.6x smaller, monotonic ids with gaps 5.4x, sorted keys 3.9x, event timestamps 2.8x, a random walk 1.63x, and a sampled continuous signal 1.48x. Nine distributions are untouched, and none regressed. Decode costs 2.6x where a vector is differenced, from the running sum, and is unchanged elsewhere. Encode costs 2-3x across the board, which is the searches, and is the next thing to fix. The decision logic lives in its own header, free of Arrow, so it can be exercised on its own without going through a page or a buffer.
The frame search left encode at 2-3x its old cost, all of it in two extra
traversals per vector. Both are now conditional or fused:
- The bit-width histogram and the bucket counts are gathered in one walk.
Each needs the same offset, and computing it twice was the larger half
of the search.
- A constant vector returns from min/max alone. Equal values all land in
one histogram bin, where the read-modify-write serializes, so this case
was running at a fraction of the usual rate for an answer that is
already at the floor. Constant now encodes 1.5x (int64) to 3.3x (int32)
faster than it did before the search existed.
- The scan is seeded with the minimum-frame cost, so a window has to beat
the incumbent to register. On the columns a frame cannot help -- 13 of
18 here -- everything after the scan is skipped.
- ComputeDeltas returns the bounds of what it wrote, so the differences
are not walked again to find their range.
The scan picks a bucket, and bucket boundaries stand 2^shift apart, which
on a wide column is thousands. A cluster sitting just above a boundary was
paying those bits for nothing, so the winning window is now walked once to
lower the frame onto the smallest value it actually covers. Per-bucket
minima kept inside the counting pass would avoid the walk but cost every
vector a compare and a store per element, including the vectors that
discard them -- about 30% of encode throughput to improve one column.
Behind the seeded scan, only a vector the search has already won pays.
Against the previous commit: sensor_dropouts is 1.20x smaller and
tcp_sawtooth 1.04-1.07x, no column grows, and encode moves within +-10%
-- faster where the scan now bails early, slower on the five columns whose
refinement walk buys those bits.
pfor_benchmark.cc gains the ten distributions this work was measured on.
The ones already there are either unordered or perfectly regular, so none
of them separates a plan that differences from one that does not, and none
puts the frame anywhere but the minimum. pfor_test.cc gains eleven tests
covering the delta mode, the two-sided frame, and the width/flag packing.
The comparison benchmark's generators are all either unordered or perfectly regular, so none of them separates an encoding that differences neighbouring values from one that packs them, and none of them puts a frame of reference anywhere but the minimum. That left PFOR's delta mode and DELTA_BINARY_PACKED untested against each other on the columns where they actually make different choices. Register the ten shapes from pfor_benchmark.cc here too, at both widths: timestamps regular and bursty, a sawtooth, bounded-rate series, monotonic ids with and without gaps, a low sentinel below the cluster, and two clusters no single window covers. These are templates rather than a pair of per-width functions, which keeps the int32 and int64 arms on provably the same distribution. They sit in their own namespace because two of them build on base distributions whose names this file already uses for columns drawn from different seeds; the figures are therefore comparable with pfor_benchmark.cc. Benchmark count goes from 476 to 756 (20 datasets x 14 codec arms).
The delta mode was costed by writing every difference out and running a frame search over them, which is most of what encoding a vector costs, and it was charged on every vector including the ones that went on to decline it. Estimate first from a strided sample of the differences and drop the mode there when the estimate cannot reach the incumbent, so a vector that will not use it pays a fraction of a pass instead of two full ones. The estimate samples widths rather than a span. A gate on the span of the differences was tried first and had to go: a sawtooth is a tight cluster of small positive differences with a handful of large negative ones, so its span is as wide as its raw span while its cost is a fraction of it. Zigzagging is what lets a histogram stand in for a search that has not run, since differences in [-k, k] zigzag into the same [0, 2k] a frame at -k would produce. On 20 distributions at both widths, encode throughput on the vectors that decline the mode is 1.55-1.93x what it was, and the vectors that accept it pay 1.4-5.2%. Two earlier shapes were measured and dropped: accumulating the histogram over every difference inside the differencing walk costs 6-27% on accepting vectors, and moving it to a pass of its own changes that by under 3%, so the cost is the histogram work and not a lost vectorization. A test pins the estimate against the ungated chooser -- same mode, width, frame and cost -- over every distribution the benchmark covers, so an estimate that turns pessimistic shows up as a lost delta rather than as a silent ratio regression.
PFOR is a Preview feature in the Parquet format, so a writer must not emit it unless the user has asked for it. Selecting Encoding::PFOR without calling enable_pfor_encoding() now makes WriterProperties::Builder::build() throw, naming whichever column asked for it; the Builder's constructor is the only way to reach a WriterProperties, so the check cannot be bypassed. Decoding is unaffected -- a file that exists is always readable. The delta mode gets its own property, on by default. It is part of PFOR rather than a separate encoding: every reader has to handle it, because each vector says in its own header which mode it used, so a page written with the mode disabled reads back through the same decoder. The property exists because differencing costs encode time on vectors it then declines, so a writer that knows its data is not sequential can skip the search. The option travels as a PforEncodeOptions struct rather than a bare bool, so a later mode does not change these signatures again. MakeEncoder and MakeTypedEncoder take the WriterProperties, which is how the per-column delta setting reaches the encoder; a caller with no properties to hand gets the defaults. Tests: 14 property tests covering the opt-in, the per-column overrides and the interaction between the global and per-column maps; and an end-to-end test that writes an arithmetic run twice and requires the column chunk to be smaller with the mode left on. A negative control -- the encoder factory ignoring the property -- makes that end-to-end test fail with both sizes equal, so it does reach the encoder rather than stopping at build().
Two cases the corruption tests did not reach, both specific to the flag in bit 7 of the bit-width byte. Setting the flag on a vector that was written without one makes the decoder consume sizeof(T) extra bytes of metadata. On a page sized exactly, as the encoder writes it, that pushes the vector past the end of the buffer and the decode has to fail rather than read on. Truncating a page inside a delta vector's start value leaves enough bytes for the info block and not enough for the vector. The bound that catches this is the one place a length check reads is_delta(), and it sits before the start value is loaded -- so without it the load runs sizeof(T) bytes past the end and only the later payload bound reports the problem. Removing it as a negative control leaves the test failing on the error message, which is all a non-sanitizer build can see of an overread; the assertion is on the message for that reason. Store also asserts, in debug builds, that the width it is about to pack fits the seven bits it has. A wider one would lose its high bits to the mask and set the delta flag on the way out, which is the failure the seven-bit mask was introduced to prevent -- Load rejects it on the way in, and this reports it at the encoder instead.
The sum is one dependent add per value and uses no lanes, which is why a delta vector does not pick up the speed a narrower type gives the unpack. A scan across lanes plus a carry broadcast would shorten the chain, but only for a vector with no exceptions: patching sits between the frame add and the sum, so a vector holding even one exception keeps this loop.
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. 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 After updating the title, you can mark the pull request as ready for review. See also: |
prtkgaur
changed the title
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.)