From a5f8b4113dd88a3774f670fe85b597fac74ddcd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 20 Jul 2026 18:21:29 +0200 Subject: [PATCH 1/3] fix(parquet): Skip fixed-length byte arrays without a length prefix 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. --- velox/dwio/parquet/reader/StringDecoder.h | 12 ++++- .../parquet/tests/examples/flba_skip.parquet | Bin 0 -> 942 bytes .../tests/reader/ParquetReaderTest.cpp | 48 ++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 velox/dwio/parquet/tests/examples/flba_skip.parquet diff --git a/velox/dwio/parquet/reader/StringDecoder.h b/velox/dwio/parquet/reader/StringDecoder.h index 65c04f4cf04..99fbb520573 100644 --- a/velox/dwio/parquet/reader/StringDecoder.h +++ b/velox/dwio/parquet/reader/StringDecoder.h @@ -47,8 +47,16 @@ class StringDecoder { if (hasNulls) { numValues = bits::countNonNulls(nulls, current, current + numValues); } - for (auto i = 0; i < numValues; ++i) { - bufferStart_ += lengthAt(bufferStart_) + sizeof(int32_t); + if (fixedLength_ > 0) { + // FIXED_LEN_BYTE_ARRAY values carry no length prefix, so the byte + // offset is a constant multiple of the fixed width. This is both an + // O(1) skip and a correctness fix: the variable-length branch below + // would read the value bytes as a bogus 4-byte length. + bufferStart_ += static_cast(numValues) * fixedLength_; + } else { + for (auto i = 0; i < numValues; ++i) { + bufferStart_ += lengthAt(bufferStart_) + sizeof(int32_t); + } } } diff --git a/velox/dwio/parquet/tests/examples/flba_skip.parquet b/velox/dwio/parquet/tests/examples/flba_skip.parquet new file mode 100644 index 0000000000000000000000000000000000000000..a6f02d904fb846948b7da14958e686eb99260e54 GIT binary patch literal 942 zcmd6m+fExX5QfceSi&ikgBQCjG_+&^DV!ppRTVB~A)pEYn@AMF-6~oI1T7&P0x!a2 zaLE&J$pdl8Ki)>I3gQ}TzwwL@zdc?%q{@O6^3lSVmmyq7I-W{R3L#VqMz~_?pQyhd zO%@2-xRVV>HsI0izyTc~4LU&==mtHY7i2&m_yAaK01Sd5Ab|^pfdU$Q1S7x$qhJht z0$DH)K7%h{0_1=Xc>6EYT1G0|nI!gKD(HB(*~pYGE(ztFR}bS~kxU6`${~(~kBvalueAt(0)); } +// Skipping over FIXED_LEN_BYTE_ARRAY values must advance the decoder by a +// constant multiple of the fixed width. flba_skip.parquet holds 40 rows of a +// 4-byte fixed binary column whose value equals the row index (big-endian), +// plus an int32 key. Filtering the key to the even rows forces the FLBA +// decoder to skip the odd rows one value at a time. A skip that instead read +// the value bytes as a length prefix would misalign or read out of bounds. +TEST_F(ParquetReaderTest, fixedLenByteArraySkipWithFilter) { + const std::string filename("flba_skip.parquet"); + const auto fileSchema = ROW({"key", "value"}, {INTEGER(), VARBINARY()}); + + constexpr int32_t kNumRows = 40; + std::vector evenKeys; + for (int64_t i = 0; i < kNumRows; i += 2) { + evenKeys.push_back(i); + } + const auto kNumSelected = static_cast(evenKeys.size()); + FilterMap filters; + filters.insert( + {"key", + std::make_unique( + 0, kNumRows - 2, std::move(evenKeys), false)}); + + // Backing storage for the expected 4-byte big-endian values. + std::vector valueStore; + for (int32_t i = 0; i < kNumRows; i += 2) { + const auto u = static_cast(i); + std::string bytes(4, '\0'); + bytes[0] = static_cast((u >> 24) & 0xffU); + bytes[1] = static_cast((u >> 16) & 0xffU); + bytes[2] = static_cast((u >> 8) & 0xffU); + bytes[3] = static_cast(u & 0xffU); + valueStore.push_back(std::move(bytes)); + } + auto expected = makeRowVector( + {"key", "value"}, + { + makeFlatVector( + kNumSelected, [](auto row) { return row * 2; }), + makeFlatVector( + kNumSelected, + [&](auto row) { return StringView(valueStore[row]); }, + nullptr, + VARBINARY()), + }); + + assertReadWithFilters(filename, fileSchema, std::move(filters), expected); +} + TEST_F(ParquetReaderTest, readBinaryAsStringFromNation) { const std::string filename("nation.parquet"); From 532779f447930738fa321556c718a12bc069b9de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Wed, 22 Jul 2026 12:16:42 +0200 Subject: [PATCH 2/3] docs(parquet): Move flba_skip fixture explanation to examples README 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. --- velox/dwio/parquet/tests/examples/README.md | 10 ++++++++++ velox/dwio/parquet/tests/reader/ParquetReaderTest.cpp | 8 ++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/velox/dwio/parquet/tests/examples/README.md b/velox/dwio/parquet/tests/examples/README.md index 85dc66d0f4c..e0e60d2dc0a 100644 --- a/velox/dwio/parquet/tests/examples/README.md +++ b/velox/dwio/parquet/tests/examples/README.md @@ -180,6 +180,16 @@ annotation, definition level, repetition level, and compression when useful. - Purpose: Tests reading FIXED_LEN_BYTE_ARRAY as VARBINARY and validates mixed primitive physical type handling. +### `flba_skip.parquet` + +- Metadata: `created_by=parquet-cpp-arrow version 24.0.0`, 40 rows, 1 row group, + columns `key: INT32` and `value: FIXED_LEN_BYTE_ARRAY` (4-byte width), both + optional, uncompressed. `value` equals the row index encoded big-endian. +- Purpose: Regression fixture for skipping FIXED_LEN_BYTE_ARRAY values. Filtering + `key` to the even rows forces the FLBA decoder to skip the odd rows, which must + advance by a constant multiple of the fixed width. A skip that instead read the + value bytes as a 4-byte length prefix would misalign or read out of bounds. + ### `uuid.parquet` - Metadata: `created_by=parquet-mr version 1.12.2`, 3 rows, 1 row group, diff --git a/velox/dwio/parquet/tests/reader/ParquetReaderTest.cpp b/velox/dwio/parquet/tests/reader/ParquetReaderTest.cpp index 9eea4538849..238fb00f472 100644 --- a/velox/dwio/parquet/tests/reader/ParquetReaderTest.cpp +++ b/velox/dwio/parquet/tests/reader/ParquetReaderTest.cpp @@ -1422,12 +1422,8 @@ TEST_F(ParquetReaderTest, readVarbinaryFromFLBA) { ->valueAt(0)); } -// Skipping over FIXED_LEN_BYTE_ARRAY values must advance the decoder by a -// constant multiple of the fixed width. flba_skip.parquet holds 40 rows of a -// 4-byte fixed binary column whose value equals the row index (big-endian), -// plus an int32 key. Filtering the key to the even rows forces the FLBA -// decoder to skip the odd rows one value at a time. A skip that instead read -// the value bytes as a length prefix would misalign or read out of bounds. +// Regression test for skipping FIXED_LEN_BYTE_ARRAY values under a filter on a +// sibling column. See flba_skip.parquet in examples/README.md for the fixture. TEST_F(ParquetReaderTest, fixedLenByteArraySkipWithFilter) { const std::string filename("flba_skip.parquet"); const auto fileSchema = ROW({"key", "value"}, {INTEGER(), VARBINARY()}); From b5504e7f1b87d27033564e2f03b19871e2ed0ee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Wed, 12 Aug 2026 11:21:42 +0200 Subject: [PATCH 3/3] ci: Re-trigger checks (flaky window fuzzer)