fix(parquet): prevent batch-induced oversized BYTE_ARRAY pages by olemon111 · Pull Request #747 · bytedance/bolt

olemon111 · 2026-07-16T12:59:32Z

What problem does this PR solve?

The Parquet writer could produce oversized data pages for BYTE_ARRAY columns, including VARCHAR and VARBINARY.

Previously, a complete write batch was appended to the encoder before checking the configured data page size. A batch containing large variable-width values could therefore make the buffered page significantly exceed the page-size target. In extreme cases, the compressed or uncompressed page size could exceed the int32 fields in the Parquet PageHeader, producing invalid Parquet files and causing excessive reader-side allocations.

The same problem could occur after dictionary encoding falls back to PLAIN encoding.

Issue Number: close #612

Type of Change

🐛 Bug fix (non-breaking change which fixes an issue)
✨ New feature (non-breaking change which adds functionality)
🚀 Performance improvement (optimization)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🔨 Refactoring (no logic changes)
🔧 Build/CI or Infrastructure changes
📝 Documentation only

Description

This change prevents batch-induced oversized Parquet BYTE_ARRAY pages before values are appended to the page encoder.

The writer now:

Estimates the PLAIN-encoded size of a BYTE_ARRAY chunk using Arrow binary offsets before calling the encoder.
Writes a normal chunk directly when it fits in the current page.
Flushes the current page and writes the whole chunk when it fits in an empty page.
Performs per-level splitting only when the chunk itself cannot fit in an empty page.
Preserves record boundaries for nested columns when required by DataPageV2 or page-index writing.
Allows a single oversized value or record to make progress, while relying on the final PageHeader size validation for the hard Parquet format limit.

The no-split fast path applies to flat, nullable, and nested BYTE_ARRAY columns. It reuses the validity metadata required by the existing write path and avoids an additional per-level value-length scan for normal chunks.

Dictionary encoding is also covered:

Dictionary encoded size accounting now uses int64_t.
Dictionary growth is checked using a byte budget before appending a potentially large batch.
Repeated dictionary values do not unnecessarily trigger fallback.
After dictionary fallback, PLAIN-encoded values use the same data-page splitting protection.

As a final safety boundary, compressed, uncompressed, and encrypted data/dictionary page sizes are validated before narrowing them to the Parquet PageHeader int32 fields.

Tests cover:

Flat PLAIN-encoded VARCHAR.
Nullable PLAIN-encoded VARBINARY.
Nested ARRAY<VARCHAR> with parent and element nulls.
DataPageV2 record-boundary splitting.
A final oversized nested record.
Dictionary page byte budgeting.
Dictionary fallback followed by PLAIN page splitting.
Repeated dictionary values.
A single oversized BYTE_ARRAY value.
Data and dictionary PageHeader size overflow guards.

The Parquet writer benchmark was also updated to:

Honor Folly benchmark iterations.
Exclude writer construction, data generation, metadata inspection, and destruction from the measured interval.
Use an in-memory sink for targeted no-split cases.
Verify that baseline and fixed implementations produce one data page.
Report output size and data page count.
Cover short flat, nullable, VARBINARY, nested ARRAY<VARCHAR>, and normal
dictionary-encoded inputs.

Performance Impact

No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).
Positive Impact: I have run benchmarks.

Negative Impact: Targeted benchmarks show up to 3.22% overhead for
short VARCHAR batches with no actual null values; other tested no-split
cases show no clear regression.

Click to view Benchmark Results

Benchmark target:
bolt_dwio_parquet_writer_benchmark

Note:
Both versions used the same benchmark harness. Writer construction, input
generation, metadata inspection, and filesystem I/O are excluded. Results
are 5-run medians, and both versions produced the same output size and one
data page for every case.

Targeted BYTE_ARRAY no-split path:
--bm_regex='NoSplit'
--bm_max_secs=1
--bm_min_usec=100000

VarcharDictionary 4k null20       607.84us -> 603.63us  -0.69%
VarcharDictionary 32k null20        4.14ms -> 4.12ms    -0.53%

VarbinaryPlain 4k null20             395us -> 394.69us  -0.08%
VarbinaryPlain 32k null20           2.45ms -> 2.45ms    +0.11%
VarbinaryPlain 256k null20         20.41ms -> 20.07ms   -1.62%

VarcharListPlain 4k null20          1.96ms -> 1.96ms    +0.23%
VarcharListPlain 32k null20        15.64ms -> 15.30ms   -2.16%

VarcharPlain 4k null0             332.12us -> 335.63us  +1.06%
VarcharPlain 32k null0               2.00ms -> 2.07ms   +3.22%
VarcharPlain 256k null0             15.38ms -> 15.57ms  +1.26%

VarcharPlain 4k null20            379.97us -> 380.63us  +0.17%
VarcharPlain 32k null20              2.46ms -> 2.46ms   +0.10%
VarcharPlain 256k null20            20.63ms -> 20.49ms  -0.68%

Result:
No broad performance regression was observed; the largest measured overhead
was 3.22%.

Release Note

Release Note:

Release Note:
- Fixed oversized Parquet BYTE_ARRAY data pages for VARCHAR and VARBINARY columns, including dictionary fallback to PLAIN encoding.
- Added Parquet PageHeader size validation to prevent invalid files caused by int32 page-size overflow.

Checklist (For Author)

I have added/updated unit tests (ctest).
I have verified the code with local build (Release/Debug).
I have run clang-format / linters.
(Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
No need to test or manual test.

Breaking Changes

No

Yes (Description: ...)

Click to view Breaking Changes

Breaking Changes:
- Description of the breaking change.
- Possible solutions or workarounds.
- Any other relevant information.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Weixin-Xu · 2026-07-20T07:28:41Z

    if (data_encryptor_.get()) {
      PARQUET_THROW_NOT_OK(encryption_buffer_->Resize(
-          data_encryptor_->CiphertextSizeDelta() + output_data_len, false));
+          data_encryptor_->CiphertextSizeDelta() + compressed_page_size,


When the compressed page is close to INT32_MAX, this addition can overflow before the post-encryption checkPageHeaderSize() is reached. The same issue exists in the dictionary-page path.

Thanks, fixed both data and dictionary paths by validating the encrypted buffer size in int64 before allocation and encryption. Regression tests were added for both paths.

Weixin-Xu · 2026-07-20T07:39:52Z

+  std::unique_ptr<PageWriter> pageWriter_;
+};
+
+TEST_F(PageWriterSizeGuardTest, rejectsUncompressedDataPageSizeOverflow) {


These tests verify the final PageWriter guard, but they do not exercise the BYTE_ARRAY splitter against the PageHeader hard limit.

Added a ColumnWriter test with a reduced PageHeader limit. It writes real Arrow VARCHAR batches with the configured page size above the hard limit and verifies both PLAIN and dictionary/fallback paths keep emitted pages within that limit. Production still uses INT32_MAX.

luozenglin · 2026-07-20T07:59:36Z

+  // the hard format boundary, so cap byte-budget arithmetic at INT32_MAX and
+  // leave the final encoded/compressed size checks to PageWriter.
+  auto PageByteLimit = [](int64_t configured_limit) {
+    return std::clamp<int64_t>(configured_limit, 0, kMaxPageHeaderSize);


Is it more reasonable to throw an exception when configured_limit is not within the range?

data_pagesize is an existing best-effort flush target rather than a hard user-visible limit. I kept the configured value accepted and only cap the internal BYTE_ARRAY budget at the PageHeader int32 boundary, adding a throw here would introduce behavior change.

luozenglin · 2026-07-20T12:20:25Z

+        &null_count);
+
+    // For PLAIN encoding this is the data-page value size. During dictionary
+    // encoding it is a conservative upper bound on dictionary growth: duplicate


When cardinality is low, using the plain-encoded size to estimate the deduplicated dictionary size can introduce an order-of-magnitude error. Is this reasonable?

The estimate is intentionally conservative and follows the same policy as arrow-rs #9972. Its merged implementation uses the remaining dictionary-page budget and estimates ordinary Utf8/Binary input from its PLAIN-encoded size. Arrow DictionaryArray input is handled separately because its values are already deduplicated (code and comment).
For low-cardinality input this may cause additional subchunk work, but fallback still checks the actual dict_encoded_size(), so the conservative estimate itself does not cause false fallback. repeatedByteArrayValuesDoNotForceDictionaryFallback covers this case.

luozenglin · 2026-07-20T12:27:03Z

+    // The offset range is exact for canonical Arrow binary arrays and a
+    // conservative upper bound if a null slot retains bytes.
+    const int64_t page_byte_limit = CurrentPageByteLimit();
+    const int64_t batch_encoded_bytes = PlainEncodedRangeSize(


When using DELTA_SYTE-ARRAY encoding, batch_encoded_bytes uses PLAIN size while CurrentPageBytes() uses actual DELTA size. This can greatly overestimate repeated or prefix-similar strings and create excessive pages。

Thanks. I separated the PLAIN/raw byte budget from the actual encoded page size. DELTA now uses the raw estimate only to bound each encoder Put(), while page flushing is based on EstimatedDataEncodedSize(). PLAIN and dictionary keep their remaining-budget behavior. I also fixed DELTA_LENGTH_BYTE_ARRAY Arrow payload accounting and added compressible/incompressible DELTA page-layout and round-trip tests.

luozenglin · 2026-07-20T12:34:42Z

+        batch_num_spaced_values,
+        batch_num_values,
+        page_byte_limit);
+    if (CappedPageBytes(


I guess there are many cases online where batch_decoded_bytes are greater than 1M but less than INT32 MAX, which cannot hit this fast path. Please sample the online situation for performance testing and consider whether it is necessary to increase the default 1M page size

Thanks. I validated the fix with the affected online cases, but stable reproducible samples are limited because some failures are intermittent. I’ll continue monitoring and collecting samples.
I prefer to keep the default 1 MiB page target unchanged in this PR. Increasing it would raise per-page memory usage and only move the threshold rather than solve oversized batches. Any default-size tuning should be evaluated separately with representative workload data.

luozenglin

LGTM

fix(parquet): prevent batch-induced oversized BYTE_ARRAY pages

dea6984

Copilot AI review requested due to automatic review settings July 16, 2026 12:59

Copilot AI reviewed Jul 16, 2026

Weixin-Xu reviewed Jul 20, 2026

View reviewed changes

luozenglin reviewed Jul 20, 2026

View reviewed changes

fix(parquet): harden PageHeader size handling, add tests

f19f730

olemon111 force-pushed the fix_parquet_writer_oversized_bytes#612 branch from 0b2ebb0 to f19f730 Compare July 22, 2026 12:16

luozenglin approved these changes Jul 30, 2026

View reviewed changes

olemon111 added this pull request to the merge queue Jul 31, 2026

Merged via the queue into bytedance:main with commit 9ccfac8 Jul 31, 2026
7 checks passed

Uh oh!

Conversation

olemon111 commented Jul 16, 2026

What problem does this PR solve?

Type of Change

Description

Performance Impact

Release Note

Checklist (For Author)

Breaking Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olemon111 Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luozenglin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olemon111 Jul 22, 2026 •

edited

Loading