Add regression tests for Parquet large binary offset overflow #9361

vigneshsiva11 · 2026-02-05T14:41:42Z

Which issue does this PR close?

Refs Error when reading row group larger than 2GB (total string length per 8k row batch exceeds 2GB) #7973

Rationale for this change

This PR adds regression coverage for an offset overflow panic encountered when reading Parquet files with large binary columns using the Arrow Parquet reader.

The issue occurs when a single RecordBatch contains more than 2GB of binary data and is decoded into a 'BinaryArray' using 32-bit offsets. Similar failures are observed across multiple Parquet encodings. Adding regression tests helps document the failure mode and provides a foundation for validating a follow-up fix.

What changes are included in this PR?

This PR introduces a new test file under 'parquet/tests/arrow_reader' that adds regression tests covering large binary columns for the following Parquet encodings:

PLAIN
DELTA_LENGTH_BYTE_ARRAY
DELTA_BYTE_ARRAY
RLE_DICTIONARY

The tests construct Parquet files that exceed the 32-bit offset limit when decoded into a single Arrow 'BinaryArray' and assert successful RecordBatch decoding.

Are these changes tested?

Yes. This PR consists entirely of regression tests.

The tests are currently marked as ignored to avoid excessive memory usage in CI and to document the existing failure mode. They are intended to be enabled once the underlying reader logic is updated to safely handle large binary data without overflowing offsets.

Are there any user-facing changes?

No. This PR only adds tests and does not introduce any user-facing changes.

Copilot

Pull request overview

This PR adds regression tests for a Parquet reader issue where reading files with more than 2GB of binary data in a single RecordBatch causes an offset overflow panic. The tests document the failure mode across multiple Parquet encodings and are marked as ignored to avoid excessive memory usage in CI.

Changes:

Added test file large_string_overflow.rs with four regression tests covering different Parquet encodings (PLAIN, DELTA_LENGTH_BYTE_ARRAY, DELTA_BYTE_ARRAY, and RLE_DICTIONARY) for large binary data
Tests create Parquet files exceeding 2GB when decoded into a single BinaryArray
All tests are marked with #[ignore] to document the existing failure without causing CI issues

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

parquet/tests/arrow_reader/large_string_overflow.rs

Copilot · 2026-02-05T14:49:34Z

parquet/tests/arrow_reader/large_string_overflow.rs

+fn large_binary_rle_dictionary_encoding_overflow() {
+    let array = make_large_binary_array();
+    let file =
+        write_parquet_with_encoding(array, Encoding::RLE_DICTIONARY);


This test will panic when run. The WriterProperties builder at line 40 calls set_encoding(encoding) with Encoding::RLE_DICTIONARY, but according to the parquet properties implementation (parquet/src/file/properties.rs:1087-1089), calling set_encoding with RLE_DICTIONARY or PLAIN_DICTIONARY will panic with "Dictionary encoding can not be used as fallback encoding".

To enable dictionary encoding, you should only use set_dictionary_enabled(true) without calling set_encoding(Encoding::RLE_DICTIONARY). The dictionary encoding will be used automatically when dictionary is enabled.

Copilot · 2026-02-05T14:49:35Z

parquet/tests/arrow_reader/large_string_overflow.rs

+    let props = WriterProperties::builder()
+        .set_dictionary_enabled(true)
+        .set_encoding(encoding)
+        .build();
+


The write_parquet_with_encoding function sets both set_dictionary_enabled(true) and set_encoding(encoding). According to the WriterProperties documentation, when dictionary is enabled and a specific encoding is set, the specified encoding becomes the "fallback" encoding (see parquet/src/file/properties.rs:1079-1081). This means these tests may not actually be testing the specified encodings directly, but rather testing dictionary encoding with these as fallbacks.

For more accurate testing of specific encodings, consider setting set_dictionary_enabled(false) for non-dictionary encodings (PLAIN, DELTA_LENGTH_BYTE_ARRAY, DELTA_BYTE_ARRAY). The dictionary test should only use set_dictionary_enabled(true) without calling set_encoding.

Suggested change

let props = WriterProperties::builder()

.set_dictionary_enabled(true)

.set_encoding(encoding)

.build();

let mut builder = WriterProperties::builder();

// For non-dictionary encodings, disable dictionary and set the encoding directly.

// For dictionary encoding, enable dictionary and rely on the default dictionary behavior.

let builder = match encoding {

Encoding::RLE_DICTIONARY => builder.set_dictionary_enabled(true),

_ => builder

.set_dictionary_enabled(false)

.set_encoding(encoding),

};

let props = builder.build();

I think this is an important comment to resolve

alamb

Thank you for this PR @vigneshsiva11 -- it is a good start. I left some comments -- let me know what you think

alamb · 2026-02-08T11:54:40Z

parquet/tests/arrow_reader/large_string_overflow.rs

+    let props = WriterProperties::builder()
+        .set_dictionary_enabled(true)
+        .set_encoding(encoding)
+        .build();
+


I think this is an important comment to resolve

alamb · 2026-02-08T11:59:29Z

parquet/tests/arrow_reader/large_string_overflow.rs

@@ -0,0 +1,116 @@
+// Licensed to the Apache Software Foundation (ASF) under one


In order for this test to actually be run, I think you need to add it to mod.rs

As it is the file is ignored and the tests are not run

index ffc36655b3..33fc59d494 100644 --- a/parquet/tests/arrow_reader/mod.rs +++ b/parquet/tests/arrow_reader/mod.rs @@ -48,6 +48,7 @@ mod io; mod predicate_cache; mod row_filter; mod statistics; +mod large_string_overflow; // returns a struct array with columns "int32_col", "float32_col" and "float64_col" with the specified values fn struct_array(input: Vec<(Option<i32>, Option<f32>, Option<f64>)>) -> ArrayRef {

When I add that this file doesn't compile

I had to change the building ot

fn make_large_binary_array() -> ArrayRef { let value = vec![b'a'; VALUE_SIZE]; let mut builder = BinaryBuilder::new(); for _ in 0..ROWS { builder.append_value(&value); } Arc::new(builder.finish()) }

And then it seems to work well

alamb · 2026-02-08T12:01:28Z

parquet/tests/arrow_reader/large_string_overflow.rs

+}
+
+#[test]
+#[ignore = "regression test for >2GB binary offset overflow"]


Rather than ignoring the tests, can you please

add an assertion that it should panic?

Add a comment with a link to the ticket?

That way we are sure the test properly covers what you want

For example

#[test] // Panics until https://github.com/apache/arrow-rs/issues/7973 is fixed #[should_panic(expected = "byte array offset overflow")] fn large_binary_plain_encoding_overflow() {

vigneshsiva11 · 2026-02-08T14:37:18Z

Hi @alamb, Thanks for the feedback.

I’ve updated the PR to address your comments:

Replaced the ignored test with #[should_panic] to explicitly assert the current failure.
Added a comment linking to Error when reading row group larger than 2GB (total string length per 8k row batch exceeds 2GB) #7973 explaining why the panic is expected.
Kept the other encoding cases as ignored regression tests for the follow-up fix.
Reformatted the code and confirmed CI passes.

alamb

Thanks @vigneshsiva11 -- getting close

alamb · 2026-02-09T21:35:38Z

parquet/tests/arrow_reader/large_string_overflow.rs

+}
+
+#[test]
+// Panics until https://github.com/apache/arrow-rs/issues/7973 is fixed


Can you please apply this same pattern to the other tests in this file (so they all use should_panic and hav a comment)?

Hi @alamb, thanks for the review!

I’ve updated the PR to apply the same pattern to all tests in this file:

Converted the remaining cases to use #[should_panic(expected = "byte array offset overflow")]

Added a comment on each test linking to #7973 to document why the panic is currently expected

Removed the ignored tests so all cases are explicitly asserted

I’ve also reformatted the code and confirmed CI passes. Please let me know if this looks good or if you’d like any further adjustments.

Thanks again!

vigneshsiva11 · 2026-02-10T15:52:12Z

This failure looks like a transient CI issue on Windows while downloading protoc ('protoc-21.4-win64.zip' unzip error).

There are no changes in this PR related to CI or protoc. Could someone please re-run the failing job?

alamb · 2026-02-10T16:44:13Z

Thanks @vigneshsiva11 -- I restarted the windows job

alamb

Thank you @vigneshsiva11 🙏

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T16:48:53Z

parquet/tests/arrow_reader/large_string_overflow.rs

+#[test]
+// Panics until https://github.com/apache/arrow-rs/issues/7973 is fixed
+#[should_panic(expected = "byte array offset overflow")]
+fn large_binary_plain_encoding_overflow() {


These tests allocate/write ~3GB of binary data and currently lack #[ignore], which will likely cause OOM/timeouts in CI. This also contradicts the PR description stating the tests are ignored. Consider marking these tests #[ignore] (or gating behind an env flag), and make the assertions reflect the intended post-fix behavior (i.e., successful RecordBatch decoding) rather than using #[should_panic].

Copilot · 2026-02-10T16:48:54Z

parquet/tests/arrow_reader/large_string_overflow.rs

+// Panics until https://github.com/apache/arrow-rs/issues/7973 is fixed
+#[should_panic(expected = "byte array offset overflow")]
+fn large_binary_plain_encoding_overflow() {


#[should_panic(expected = "byte array offset overflow")] doesn't appear to match the actual overflow error text produced by the Parquet arrow reader (e.g. "index overflow decoding byte array" from parquet/src/arrow/buffer/offset_buffer.rs). As written, these tests may fail even when the overflow occurs. If you keep a panic-based test, align the expected substring with the real message, or (preferably) assert on the returned Err explicitly instead of relying on unwrap() panics.

Copilot · 2026-02-10T16:48:54Z

parquet/tests/arrow_reader/mod.rs

 mod checksum;
 mod int96_stats_roundtrip;
 mod io;
+mod large_string_overflow;


The test module/file name (large_string_overflow) is misleading: this code is exercising DataType::Binary / BinaryBuilder, not strings. Renaming to something like large_binary_overflow (and updating the mod declaration) would make the purpose clearer and align with the PR title.

Suggested change

mod large_string_overflow;

mod large_binary_overflow;

Add regression tests for Parquet large binary offset overflow

c322da4

Copilot AI review requested due to automatic review settings February 5, 2026 14:41

github-actions bot added the parquet Changes to the parquet crate label Feb 5, 2026

Copilot started reviewing on behalf of vigneshsiva11 February 5, 2026 14:44 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

vigneshsiva11 added 4 commits February 5, 2026 15:01

Add Apache license header and format regression tests

b3e1146

Format Parquet large string overflow tests

d64f8be

Trigger CI rerun

42fb85b

Format Parquet large string overflow tests to satisfy CI

188f3b9

alamb reviewed Feb 8, 2026

View reviewed changes

parquet: fix large binary overflow regression tests

88c7f4c

alamb reviewed Feb 9, 2026

View reviewed changes

parquet: make large binary overflow tests assert panic

f16930a

alamb requested a review from Copilot February 10, 2026 16:44

Copilot started reviewing on behalf of alamb February 10, 2026 16:45 View session

alamb approved these changes Feb 10, 2026

View reviewed changes

Copilot AI reviewed Feb 10, 2026

View reviewed changes

-    let props = WriterProperties::builder()
-        .set_dictionary_enabled(true)
-        .set_encoding(encoding)
-        .build();
+    let mut builder = WriterProperties::builder();
+    // For non-dictionary encodings, disable dictionary and set the encoding directly.
+    // For dictionary encoding, enable dictionary and rely on the default dictionary behavior.
+    let builder = match encoding {
+        Encoding::RLE_DICTIONARY => builder.set_dictionary_enabled(true),
+        _ => builder
+            .set_dictionary_enabled(false)
+            .set_encoding(encoding),
+    };
+    let props = builder.build();

		@@ -0,0 +1,116 @@
		// Licensed to the Apache Software Foundation (ASF) under one

Add regression tests for Parquet large binary offset overflow #9361

Are you sure you want to change the base?

Add regression tests for Parquet large binary offset overflow #9361

Conversation

vigneshsiva11 commented Feb 5, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

alamb left a comment • 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!

alamb Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

vigneshsiva11 commented Feb 8, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

vigneshsiva11 Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

vigneshsiva11 commented Feb 10, 2026

Uh oh!

alamb commented Feb 10, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alamb left a comment •

edited

Loading