Review of apache/arrow PR #55577

• PR: #55577
• Head reviewed: 6578d602fbfb07a727e19e128b27f50611ea934d (branch read-fixed-list, base main), created 2026-11-12. Draft, +841/−11, 8 files, 0 reviews at time of writing.
• Author: MashrafeeAryan (first-time contributor, per the PR).
• Reviewer note: I did not build or run the code: this sandbox cannot build Arrow C++ (2 CPU, 7.8 GB RAM), so every claim below comes from reading the full diff against current main and from reading the GitHub Actions logs for this head. Where I could not verify something by reading, I say so. Line numbers in section 1 are from the CI merge build (PR head merged into current main), which is what CI compiles; on the PR head alone they differ slightly.
• Verdict: Comment (not ready to merge). The direction is sound, the fallback design is conservative, and the matcher work plus the tests are unusually careful for a first PR. But the PR does not compile, so no test in it has run anywhere, and the headline speedup has no measured evidence in the PR. Fix the build first; everything else below is early feedback for the next revision.

1. Blocking: the PR does not compile (stale vs. main)

The PR branched from a main where ChunksToSingle took one argument. Current main changed it to take a MemoryPool* as well (cpp/src/parquet/arrow/reader.cc:90 on main):

::arrow::Result<std::shared_ptr<ArrayData>> ChunksToSingle(const ChunkedArray& chunked,
                                                           MemoryPool* pool)

and main's two existing call sites pass ctx_->pool (reader.cc:677 and reader.cc:930). The PR's new code in FixedSizeListReader::BuildArray still uses the old one-argument form, so the CI merge build fails:

cpp/src/parquet/arrow/reader.cc:760:

ARROW_ASSIGN_OR_RAISE(auto child_data, ChunksToSingle(*child));

The compiler error from CI (AMD64 Ubuntu 24.04 C++ ASAN UBSAN, job 131359958653, run 43268942694) is exactly this, and it is the only error: in that job's log:

reader.cc:760:44: error: no matching function for call to 'ChunksToSingle'
note: candidate function not viable: requires 2 arguments, but 1 was provided

The fix is one line, following the two existing call sites in the same file (and please rebase onto current main so the branch stops drifting from the signature it calls):

ARROW_ASSIGN_OR_RAISE(auto child_data, ChunksToSingle(*child, ctx_->pool));

Consequences, all verified from the check runs on this head:

• Every C++, Python, R and GLib job fails (all from the same build failure; the Python/R jobs depend on the C++ build). Only the label-check, integration and source-release jobs pass, because they do not compile the C++ code.
• No test added by this PR has executed anywhere — not the round-trip tests, not the record-reader differential test, not the matcher unit tests, not the benchmark. Any statement about what the tests prove is, at this head, unproven.
• The lint job also fails, for a separate reason (see section 2).

2. Lint fails: one clang-format issue

The Lint C++, Python, R, Docker, RAT job fails only on the C++ Format pre-commit hook, and only in one file: cpp/src/parquet/level_decoder_internal.h. clang-format wants the CountUpTo trailing return type on one line:

auto PageLevelDecoder<BitDec, RleDec>::CountUpTo(int16_t value,
                                                 int32_t batch_size) -> CountUpToResult {

instead of the PR's two-line split. pre-commit run --all-files fixes it. (No other file is flagged.)

3. The PR description is the unfilled template, and the speedup claim has no in-PR evidence

The PR body is the template with every section blank (Rationale, What changes, Are these changes tested, user-facing changes) and the AI-disclosure checkboxes unchecked. Only the GitHub Issue: #55576 line is filled. For a first PR this is understandable, but a reviewer cannot tell from the PR itself what was measured or how.

About the 2.5–3.9x figure: it does not come from this PR. Issue #55576 (same author) attributes "about 2.5x faster column reads and up to about 3.9x faster row reads" to the Hardwood blog post, and "about 2.3x on 4k-float arrays" to a Parquet dev-list thread. The issue's own acceptance criteria ask for a benchmark "comparing reading fixed_size_list<float, n> against reading the same values as a flat column, before and after the change". The PR adds that benchmark (BM_ReadFloatLayout in reader_writer_benchmark.cc) but reports no numbers at all — no before/after, no per-configuration results.

Assessment of the benchmark as written (read, not run):

• The design is reasonable: same float values as a flat column vs. as fixed-size lists, BENCHMARK_SIZE values, list sizes {1, 3, 8, 9, 15, 16, 768}, V1/V2, ZSTD on/off, 1 MB pages plus 8 KB pages for sizes 3 and 768, single-threaded ReadTable, warm-up read outside timing with a correctness check, and peak_read_bytes / read_allocations / file_bytes counters. Comparing equal value counts between layouts is the right comparison.
• But it only measures the best case for the fast path: null_probability = 0.0, PLAIN encoding, dictionary disabled, non-nullable lists and elements. It does not measure columns with nulls (generic path), dictionary-encoded pages, DELTA encodings, small streaming batch sizes, or multi-column tables.
• It also times reader setup and result destruction per iteration (a fresh reader is opened inside the timed loop), which dilutes the read-path difference; the before/after comparison the issue asks for is what would make the number meaningful.
• Suggestion: when the build is fixed, paste the before/after table into the PR description for the main configurations, and state the hardware. Until then, please do not quote 2.5–3.9x as a result of this PR; it is someone else's measurement of a different reader.

4. Correctness: what I verified by reading

I read the full diff and the surrounding code on main. The overall design is sound and conservative: the fast path only engages when the encoded level streams provably describe full, non-null, fixed-width lists, and anything unrecognized falls back to the existing generic path. I found no correctness bug in the logic I could trace, but none of it has been executed, so this section is a reading assessment, not a verification.

4.1 When the fast path engages — the gate is tight

TypedRecordReader::SetFixedSizeList (column_reader.cc, new) accepts only Int32/Int64/Float/Double, requires width > 0, max_rep_level() == 1 (so a list nested inside another list is excluded), and !read_dictionary(). FixedSizeListReader's constructor (reader.cc) calls it with the Arrow type's list_size(), so a mismatch between the Parquet leaf and the Arrow type cannot silently engage the path.

MatchFixedPage (column_reader.cc, new) additionally requires, for the page being entered:

• the page is fresh (available_values_current_page() == page_value_count()), i.e. no values consumed yet — except that a page already matched is memoized in fixed_page_, so later batches on the same page re-enter without re-checking;
• both level decoders are RLE (is_rle()), so V1 pages with BIT_PACKED levels fall back;
• AllPresent on the def levels: a single RLE run of exactly max_def_level, of length exactly page_value_count();
• Repetitions on the rep levels: the exact 0, 1, 1, …, 1 pattern for the width, starting from the carried fixed_phase_.

Consequences I checked:

• Null lists, null elements, and empty lists cannot take the fast path. A null list has def level below max_def_level; a null element likewise; a FixedSizeList has no empty form in Arrow, and a null list is the only "empty" case. All of them make AllPresent fail. This matters because the dense assembly path (FixedSizeListReader::BuildArray) builds the list ArrayData with buffers {nullptr} and null count 0 — i.e. no list-level validity bitmap at all. That is correct only because the fast path guarantees no nulls. The invariant is real but implicit; a comment at BuildArray stating "only reachable when the whole batch was fast-path, hence no null lists" would make it auditable.
• Dictionary-encoded numeric columns are handled correctly, though not for the reason the code suggests. SetFixedSizeList's read_dictionary() check is vacuous for the four supported types: only ByteArrayDictionaryRecordReader returns true from read_dictionary() (column_reader.cc:3079), and DispatchTypedRecordReader ignores params.read_dictionary for numeric types, always building a plain TypedRecordReader with a ValueSinkBuffer. But that is harmless: DictDecoderImpl::Decode (decoder.cc:921) materializes real values via GetBatchWithDict, so ReadValuesDense on a dictionary decoder writes values, not indices. The check is dead code for these types; either drop it or add a comment explaining why dictionary encoding is safe here (the PopNewDictionary / OnNewDictionary handling is a no-op for ValueSinkBuffer, which is correct for the same reason).
• The deferred record count at page boundaries is resolved inside ReadRecords. When a batch ends exactly at a page boundary with a complete list, the fast branch un-counts the last record (--records_read; at_record_start_ = false) and re-counts it on the next page's first fast iteration (or at EOF, or via the generic path if the next page does not match). I traced the exits of ReadRecords and could not find a path where the deferral survives the call, so the Reset() assignment generic_batch_ = has_buffered_levels() || !at_record_start_ does not lose the re-add. This is subtle; a test that asserts the record count (not just array equality) for a batch ending exactly at a page boundary would pin it down.
• UseGeneric re-synchronizes the level decoders correctly in both cases I could construct. The fast path never touches the level decoders, so on fallback it skips behind = def_levels_decoder_.remaining() - available_values_current_page() levels. In the pure-fast case behind equals the number of fast-consumed values; in the mixed case (buffered levels from an earlier generic batch) behind <= 0 and the behind > 0 guard skips the sync, which is correct because the generic path had already advanced the decoders. PageLevelDecoder::remaining() is num_values_remaining_ (level_decoder_internal.h:160), matching the formula's assumption.

4.2 The matcher (fixed_size_list_internal.h, new file)

I walked Repetitions, ExpectedByte, and ReadHeader by hand. The logic I checked:

• Bit-packed groups are LSB-first, matching the Parquet RLE/bit-packed hybrid layout; ExpectedByte clears exactly the bits at positions phase == 0 ? 0 : width - phase, stepping by width — the list-start positions within each byte. width == 1 correctly yields byte 0.
• The 64-bit word comparisons use a cached pattern table for width <= 16 and regenerate the expected bytes otherwise; phase advances by 64 per word. The trailing-bits loop masks the final byte so padding bits beyond the page's logical count are ignored, and a flipped final logical bit is rejected (the unit test covers this).
• The stride shortcut (width > 8, attempted once) only fires when the validated prefix returns to the incoming phase, and only consumes whole strides (remaining >= period && bytes.size() >= stride), so a partial final repetition re-enters the walker. This looks correct.
• The RLE-run branch is strict in the right places: a run of 0 is only a single list start unless width == 1 (two consecutive 0s would mean an empty list, impossible at fixed width), and a run of 1s must fit inside the current list (run <= width - phase) and cannot start a list (phase != 0). Any rep value other than 0/1 is rejected. A trailing garbage byte makes the final bytes.empty() check fail.
• ReadHeader caps the varint at 35 bits and rejects an overlong encoding. AllPresent requires the run length to equal the page value count exactly and the value bytes to be exactly 1 (or 2 for max_level > 255) and equal to max_level.

One coverage limitation, not a bug: AllPresent only recognizes a single RLE run. If a writer splits the def-level run (I did not verify whether the C++ writer does this for very large pages), the fast path silently does not engage and the column reads through the generic path. The parquet-java fixtures in the PR itself (single runs up to 24,576 values) are reassuring for that writer. Repetitions does not have this limitation — it walks arbitrary run splits. If the C++ writer does split long def runs, relaxing AllPresent to accept consecutive runs of the same value would widen the fast path's applicability; at minimum, please confirm the fast path actually engages on C++-written output (see section 5).

One open question I could not settle by reading: for a required list of required elements (max_def_level == 0), AllPresent returns bytes.empty(). Whether the encoded RLE span is empty for V1 pages in that case depends on whether the writer emits level bytes at all when the max level is 0, and whether SetDataV1 is even reached. If it is not empty, required/required lists never take the fast path on V1 — a missed optimization, not a bug. Worth confirming with an engagement assertion for that configuration.

4.3 The dense assembly (FixedSizeListReader::BuildArray, reader.cc)

• rows = child_data->length / type.list_size() with the % list_size() != 0 check returning Invalid("Incomplete fixed-size list at end of column") is the right guard for a truncated file. ReadRecords only returns at record boundaries, so a well-formed file should never trip it.
• The dense path is gated on dense_fixed_size_list(), i.e. fixed_width_ > 0 && !generic_batch_ && fixed_phase_ == 0. A batch that mixed fast and generic pages (or ended mid-list at EOF, where ReadRecords sets at_record_start_ = true without resetting fixed_phase_) falls back to ListReader<int32_t>::BuildArray. That is the safe choice.
• The one-line fix in section 1 is the only defect I found here.

4.4 def_levels() / rep_levels() now materialize lazily

The accessors call MaterializeFixedLevels() through a const_cast, synthesizing the level buffers from fixed_levels_ (AppendFixed generates max_def_level for def levels and i % width != 0 for rep levels) and folding the count into levels_position_. This is what lets the generic path and the differential test observe the fast path's levels. It works, but mutation inside a const accessor is surprising; a short comment explaining that the accessors are the materialization point (and that Reset() discards the un-materialized count, which is safe because the buffers were never populated) would help future readers. I checked the Reset() interaction: if Reset() is called with fixed_levels_ > 0, it zeroes the count while the level buffers are still empty, so nothing is lost; has_buffered_levels() uses the member levels_position_ (not the virtual one), so it stays false in fast mode, which is consistent.

5. Test coverage: strong, with one gap that matters

The tests are the strongest part of the PR. What is covered (read, not run):

• Matcher unit tests (column_reader_test.cc, FixedSizeListLevels): packed phases and near-misses (flipped final bit, truncated stream) across widths {1, 3, 8, 9, 15, 16, 768}, phases {0, width/2, width−1}, counts {1, 7, 8, 9, 63, 64, 65, 4097}; mixed RLE runs and packed groups; malformed inputs; AllPresent across def levels {1, 2, 3, 256, 32767}.
• Real parquet-java 1.16.0 streams (ParquetJavaStreams): encoded level bytes from an independent writer for required LIST<required INT32>, 32 rows, widths {1, 3, 8, 9, 15, 16, 768}, including long streams (24,576 values, exercising the stride path) — and the test drives the real record reader over them in ReadRecords(3) batches, asserting dense_fixed_size_list() and exact values. This is exactly the evidence the issue asked for ("must match what Parquet C++, parquet-java and other major writers actually produce").
• Differential test (FixedSizeListRecordReader.DifferentialPagesBatchesAndSkips): fast vs. generic record reader over 100 rows, widths {1, 3, 8, 9, 15, 16, 768}, pages split inside records and exactly at record boundaries, batch size 4, 35 steps, with interleaved SkipRecords(2); it compares record counts, values_written, null_count, validity bitmaps, levels_position, and the full def/rep level buffers. The irregular variants are good: row 30 has width + 1 values (malformed list length, must fall back) and row 60 has def level 2 instead of 3 (null element, must fall back). It also asserts dense_fixed_size_list() on the first batch.
• Arrow round-trip tests (arrow_reader_writer_test.cc): FixedSizeListFastPathPagesAndBatches (widths × V1/V2 × dictionary on/off × nullable on/off, small pages, batch sizes {1, 7, 32, 1024}, full-table equality) and FixedSizeListFastPathMixedNullPages (one null list at row 60, one list with a null element at row 120, V1/V2, batch sizes {1, 13, 200}).

The gap: the Arrow-level round-trip tests assert equality, not engagement. Nothing at the Arrow level proves the fast path was taken on C++-written output — the tests would pass identically if every page silently fell back to the generic path. The record-reader tests do assert dense_fixed_size_list(), but only for parquet-java streams and synthetic pages. Suggested: assert engagement for at least one C++-written configuration (e.g. via a test-only counter, or by reusing the record-reader-level assertion on a C++-written page), including the required/required (max_def_level == 0) case from section 4.2. Smaller gaps: no test with a null list exactly at a page boundary (the mixed test's nulls sit mid-page); no sliced input; no width above 768 (fine — the logic is width-generic, and 768 already exercises the stride path).

6. Smaller notes

• fixed_size_list_internal.h uses std::span and a function-local static const pattern cache (thread-safe initialization, ~2 KB). Fine for Arrow's C++20 baseline. The // Non-consuming recognition … header comment states the fallback contract well.
• MatchFixedPage's memo (fixed_page_ == page_number()) is reset by SetPageReader (fixed_page_ = -1), and page_number_ increments per data page within a column chunk, so the memo cannot leak across pages or chunks. Good.
• SkipRecords calls UseGeneric() when fixed_width_ > 0, so filtered/skipped reads never take the fast path. Conservative and correct; the differential test covers the skip interleaving.
• The MultiplyWithOverflow check on remaining_records * fixed_width_ in the fast branch is good defensive practice, though remaining_records is bounded by the batch size in practice.
• The PR adds #include <span> to level_decoder_internal.h for the new encoded_rle() accessor; the accessor itself is minimal and the span points into the page buffer, which is alive while the page is current. Fine.

7. Suggested order of work

1. Fix ChunksToSingle(*child, ctx_->pool) and the clang-format issue, then let CI run — until then nothing else can be evaluated.
2. Fill in the PR description (rationale, what changed, how it was tested, AI disclosure) and paste the benchmark's before/after numbers for the main configurations.
3. Add an engagement assertion for C++-written output (section 5) and confirm the max_def_level == 0 case.
4. Consider the two documentation nits: the no-validity-bitmap invariant in BuildArray, and why dictionary encoding is safe despite the dead read_dictionary() check.

Overall: a promising, carefully designed first contribution — the matcher analysis and the differential/parquet-java tests show real care — held back at this head by a one-line compile error and by having no measured results yet. Once it builds and the tests pass, I would be glad to re-review the numbers and the remaining details.