fix(parquet): Skip fixed-length byte arrays without a length prefix - #18195
meta-codesync[bot] merged 3 commits into
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
Bot
added
the
CLA Signed
Selective Build Plan
Selective build plan |
CI Failure Analysis
🟡 Window Fuzzer with Presto as source of truth — FUZZER Failure View logsFuzzer failure: Instance 4 (seed 3 of 4 fuzzer instances passed. Instance 4 ran 7 iterations but only 3 were verified (42.86% < 50% threshold), triggering the Correlation with PR changes: Known issues:
Reproduce locally: ./velox_window_fuzzer_test \
--seed 851970327 \
--duration_sec 300 \
--batch_size=50 \
--minloglevel=0 \
--enable_window_reference_verification \
--presto_url=http://127.0.0.1:8080 \
--req_timeout_ms=10000(Requires a local Presto server running on port 8080.) Recommended fix: |
e5db09d to
95cf696
Compare
|
Separated into its independent PR as requested by @PingLiuPing |
PingLiuPing
left a comment
PingLiuPing
left a comment
There was a problem hiding this comment.
Thanks.
I verified the test failed without the fix.
| ->valueAt(0)); | ||
| } | ||
|
|
||
| // Skipping over FIXED_LEN_BYTE_ARRAY values must advance the decoder by a |
There was a problem hiding this comment.
Let's move this explanation to dwio/parquet/tests/examples/README.md
There was a problem hiding this comment.
Done. Moved the fixture explanation to velox/dwio/parquet/tests/examples/README.md as a flba_skip.parquet entry (metadata + purpose), matching the format used for the other fixtures. The inline comment in the test is now a short pointer to that entry.
b911a42 to
59c86ce
Compare
|
cudf now requires CMake 4.0 ( The setup scripts that install CMake 4.3.2 only re-run when a PR touches |
Full scans of plain-encoded BOOLEAN columns without nulls decode about 14x faster (51 us -> 3.6 us for 100K values on an AMD EPYC core at -O3). Reads with nulls, sparse rows, or filters are unchanged. BooleanDecoder previously called readBoolean() once per value, paying a branch and a remaining-bits update per bit. readWithVisitor() now takes a fast path for dense null-free reads that loads one byte and hands its 8 bits to the visitor through an unrolled loop, dropping the per-bit branch. On an early atEnd exit it records how many bits of the current byte are still unread, so a dense read whose row count is not a multiple of 8 resumes at the correct bit on the next call. A micro-benchmark (BooleanDecoderBenchmark) isolates the decoder from the reader pipeline to measure the gain, following the existing NestedStructureDecoderBenchmark convention. The O(1) FIXED_LEN_BYTE_ARRAY skip originally bundled here was split out into facebookincubator#18195. Part of facebookincubator#17994.
Full scans of plain-encoded BOOLEAN columns without nulls decode about 14x faster (51 us -> 3.6 us for 100K values on an AMD EPYC core at -O3). Reads with nulls, sparse rows, or filters are unchanged. BooleanDecoder previously called readBoolean() once per value, paying a branch and a remaining-bits update per bit. readWithVisitor() now takes a fast path for dense null-free reads that loads one byte and hands its 8 bits to the visitor through an unrolled loop, dropping the per-bit branch. On an early atEnd exit it records how many bits of the current byte are still unread, so a dense read whose row count is not a multiple of 8 resumes at the correct bit on the next call. A micro-benchmark (BooleanDecoderBenchmark) isolates the decoder from the reader pipeline to measure the gain, following the existing NestedStructureDecoderBenchmark convention. The decoded output is guarded with a checksum excluded from timing so the optimizer cannot discard the decode work. The O(1) FIXED_LEN_BYTE_ARRAY skip originally bundled here was split out into facebookincubator#18195. Part of facebookincubator#17994.
When a Parquet FIXED_LEN_BYTE_ARRAY column is read as VARBINARY or VARCHAR and some rows are skipped, for example under a filter on a sibling column, the reader returned wrong values or crashed. `StringDecoder::skip()` advanced the buffer by treating the first four bytes of each value as a length prefix, but fixed-length values carry no prefix, so a value whose leading bytes encode a large number ran the read pointer off the page. `skip()` now advances by `numValues * fixedLength_` for fixed-length columns, the same stride `readFixedString()` uses when it reads them.
Per review feedback, document the flba_skip.parquet fixture in examples/README.md alongside the other fixtures, and replace the inline explanation in the test with a short pointer to it.
59c86ce to
532779f
Compare
|
This one also seems to be ready to go @PingLiuPing PTAL again |
PingLiuPing
added
the
ready-to-merge
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this in D116320333. |
2 participants
Reading a Parquet
FIXED_LEN_BYTE_ARRAYcolumn asVARBINARYorVARCHARreturned wrong values or crashed whenever rows had to be skipped, for example under a filter on a sibling column.StringDecoder::skip()advanced the buffer by treating the first four bytes of each value as a length prefix (the variable-lengthBYTE_ARRAYlayout). Fixed-length values carry no prefix, so a value whose leading bytes encode a large number ran the read pointer off the page.readWithVisitor()already handles fixed-length values viareadFixedString(); onlyskip()was missing the corresponding branch.skip()now advances bynumValues * fixedLength_for fixed-length columns, matching the stridereadFixedString()uses. This is both a correctness fix and an O(1) skip (previously O(N)).Test:
ParquetReaderTest.fixedLenByteArraySkipWithFilterreads a 4-byte fixed binary column whose value equals the row index, filters to the even rows, and checks alignment. It segfaults on the previous code and passes with the fix.Split out of #17991 per review feedback.
Part of #17994.