Handle compressed empty DataPage v2 #7389

EnricoMi · 2025-04-07T10:09:44Z

Which issue does this PR close?

Closes Reading empty DataPageV2 fails with snappy: corrupt input (empty) #7388.

Rationale for this change

An empty bytes buffer cannot be decompressed. Spark's Parquet writer stores a DataPage v2 with only null values as an empty byte buffer, rather than compressed bytes that decompress to zero bytes.

The code currently tries to decompress a 0 bytes buffer, which is not allowed. This causes an error:

snappy: corrupt input (empty)

The issue is identical to this Apache Arrow issue: apache/arrow#22459
The fix is identical to Apache Arrow fix: apache/arrow#45252

What changes are included in this PR?

Do not attempt to decompress the empty bytes buffer.
This is tested in a unit test and a small example Parquet file.
Requires apache/parquet-testing#74.

Are there any user-facing changes?

No.

EnricoMi · 2025-04-07T10:22:10Z

parquet/src/file/serialized_reader.rs

-                &mut decompressed,
-                Some(uncompressed_size - offset),
-            )?;
+            if decompressed_size != 0 {


I'd prefer

Suggested change

if decompressed_size != 0 {

if decompressed_size > 0 {

here, but apache/arrow#45252 uses !=.

Should we add an extra check and return an error if offset > uncompressed_page_size?

Yes please! Otherwise we may be susceptible to some sort of DOS / parquet bomb with malformed input

I'd prefer

FWIW since decompressed_size is an unsigned integer I think they are equivalent you should do whatever you prefer / makes the most sense to you

parquet/src/file/serialized_reader.rs

EnricoMi · 2025-04-08T08:35:47Z

@mapleFU @pitrou this ports your bugfix apache/arrow#45252 from Arrow C++ to Rust.

alamb

Thank you @EnricoMi and @adamreeve

This is looking good.

alamb · 2025-04-08T15:01:51Z

parquet/src/file/serialized_reader.rs

-            let uncompressed_size = page_header.uncompressed_page_size as usize;
-            let mut decompressed = Vec::with_capacity(uncompressed_size);
-            let compressed = &buffer.as_ref()[offset..];
+            let uncompressed_page_size = page_header.uncompressed_page_size as usize;


If we are going to be messing with this code anyways, maybe we can do a checked conversion from signed to unsigned as well:

Suggested change

let uncompressed_page_size = page_header.uncompressed_page_size as usize;

let uncompressed_page_size = usize::try_from(page_header.uncompressed_page_size)?;

I have replaced as usize with usize::try_from in all unchecked places. There are 8 spots in this file with as u64, but I am not bolds enough to change those as well.

alamb · 2025-04-08T15:02:12Z

parquet/src/file/serialized_reader.rs

-                &mut decompressed,
-                Some(uncompressed_size - offset),
-            )?;
+            if decompressed_size != 0 {


Yes please! Otherwise we may be susceptible to some sort of DOS / parquet bomb with malformed input

alamb · 2025-04-08T15:12:15Z

parquet/src/file/serialized_reader.rs

-                &mut decompressed,
-                Some(uncompressed_size - offset),
-            )?;
+            if decompressed_size != 0 {


I'd prefer

FWIW since decompressed_size is an unsigned integer I think they are equivalent you should do whatever you prefer / makes the most sense to you

alamb · 2025-04-08T15:13:21Z

parquet/src/file/serialized_reader.rs

@@ -1321,6 +1320,107 @@ mod tests {
        assert_eq!(page_count, 2);
    }

+    #[test]
+    fn test_file_reader_empty_datapage_v2() {
+        let test_file = get_test_file("datapage_v2_empty_datapage.snappy.parquet");


While we are adding tests, can you also add a test that page_v2_empty_compressed.parquet (added in apache/parquet-testing#71) works too ? It doesn't seem to be used yet https://github.com/search?q=repo%3Aapache%2Farrow-rs%20page_v2_empty_compressed.parquet&type=code

I know you say the current reader works ok with that file, but it would be nice to add additional automated coverage

Added, this is a good test case.

alamb · 2025-04-08T15:19:10Z

parquet/src/file/serialized_reader.rs

+            assert!(is_expected_page);
+            page_count += 1;
+        }
+        assert_eq!(page_count, 1);


I double checked that the added test fails as follows without the code changes in this PR

assertion `left == right` failed left: 0 right: 1 Left: 0 Right: 1 <Click to see difference> thread 'file::serialized_reader::tests::test_file_reader_empty_datapage_v2' panicked at parquet/src/file/serialized_reader.rs:1422:9: assertion `left == right` failed left: 0 right: 1 stack backtrace:

Changed the while let Ok(Some(page)) = page_reader_0.get_next_page() { to while let Some(page) = page_reader_0.get_next_page().unwrap() { so it is obvious why expected pages are not seen (now the error surfaces).

alamb

Thank you @EnricoMi and @adamreeve

EnricoMi · 2025-04-16T12:57:11Z

Waiting for apache/parquet-testing#74 to be merged. Will then point parquet-testing sub-module back to main branch.

EnricoMi · 2025-04-21T11:06:09Z

apache/parquet-testing#74 has been merged, moved parquet-testing to latest master.

Dandandan · 2025-04-21T13:06:50Z

Thanks @EnricoMi

EnricoMi · 2025-04-21T16:04:02Z

@Dandandan thanks! Will this be released as 56.0.0 and 55.0.1?

alamb · 2025-04-28T18:14:38Z

@Dandandan thanks! Will this be released as 56.0.0 and 55.0.1?

It is scheduled to be released in 55.1.0 due in a few weeks

Release arrow-rs / parquet Minor version 55.1.0 (May 2025) #7393

EnricoMi · 2025-04-28T19:53:32Z

Thanks!

github-actions bot added the parquet Changes to the parquet crate label Apr 7, 2025

EnricoMi mentioned this pull request Apr 7, 2025

Add test file with empty DataPage v2 apache/parquet-testing#74

Merged

Work with empty datapage v2 (only null values)

adf4c2f

EnricoMi force-pushed the datapage-v2-empty branch from 1b7ce45 to adf4c2f Compare April 7, 2025 10:20

EnricoMi commented Apr 7, 2025

View reviewed changes

adamreeve reviewed Apr 7, 2025

View reviewed changes

parquet/src/file/serialized_reader.rs Outdated Show resolved Hide resolved

Remove obsolete TODO

ba81c9f

EnricoMi changed the title ~~Work with empty DataPage v2 (only null values)~~ Handle compressed empty DataPage v2 Apr 8, 2025

alamb reviewed Apr 8, 2025

View reviewed changes

EnricoMi added 5 commits April 9, 2025 20:07

Check datapage header values for plausibility

ac954a2

Replace as usize with usize::try_from(…)? where useful

e02fc5e

Assert reading page_v2_empty_compressed.parquet

b185bfe

Make get_next_page in test fail on errors

692d4f3

Split long error message

6ce79b1

adamreeve approved these changes Apr 14, 2025

View reviewed changes

alamb approved these changes Apr 14, 2025

View reviewed changes

Move parquet-testing to latest master

519bc5c

Dandandan merged commit 6a6c631 into apache:main Apr 21, 2025
16 checks passed

EnricoMi deleted the datapage-v2-empty branch April 21, 2025 16:04

This was referenced May 8, 2025

Release arrow-rs / parquet Minor version 55.1.0 (May 2025) #7393

Closed

Reading empty DataPageV2 fails with snappy: corrupt input (empty) #7388

Closed

	let uncompressed_page_size = page_header.uncompressed_page_size as usize;
	let uncompressed_page_size = usize::try_from(page_header.uncompressed_page_size)?;

Handle compressed empty DataPage v2 #7389

Handle compressed empty DataPage v2 #7389

Uh oh!

Conversation

EnricoMi commented Apr 7, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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!

Uh oh!

EnricoMi commented Apr 8, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EnricoMi Apr 9, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

EnricoMi commented Apr 16, 2025

Uh oh!

EnricoMi commented Apr 21, 2025

Uh oh!

Uh oh!

Dandandan commented Apr 21, 2025

Uh oh!

EnricoMi commented Apr 21, 2025

Uh oh!

alamb commented Apr 28, 2025

Uh oh!

EnricoMi commented Apr 28, 2025

Uh oh!

Uh oh!

EnricoMi commented Apr 7, 2025 •

edited by alamb

Loading

EnricoMi Apr 9, 2025 •

edited

Loading