Parquet AsyncReader: Don't panic when empty offset_index is Some([]) by jroddev · Pull Request #6582 · apache/arrow-rs

jroddev · 2024-10-18T07:35:30Z

Which issue does this PR close?

Closes #.

Rationale for this change

If offset_index is loaded as Some([]) then AsyncReader may panic in a couple of places. These panics do not occur when offset_index is None. Empty offset_index can end up as either depending on how its loaded.

thread 'arrow::async_reader::tests::my_test' panicked at parquet/src/arrow/async_reader/mod.rs:832:34:
index out of bounds: the len is 0 but the index is 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

An empty offset_index will switch from None to Some([]) after it is written to disk with ParquetMetaDataWriter and then read back into memory with ParquetMetaDataReader (as is done in the external_metadata example).

I believe I also saw this issue on one parquet file without the roundtrip above, but I don't currently have the file to verify.

The panic can be reproduced in the external_metadata example by changing it to load alltypes_plain.parquet instead of using the one in generated in code. You will also need to remove the call to prepare_metadata as the filesize assertion does not hold with the new data.

    - let parquet_path = create_parquet_file(&tempdir);
    + let testdata = arrow::util::test_util::parquet_test_data();
    + let parquet_path = format!("{testdata}/alltypes_plain.parquet");
    ...
    - let metadata = prepare_metadata(metadata);

Possible related to #6447

arrow-rs/parquet/src/file/metadata/reader.rs

Line 261 in 9485897

    
           // FIXME: there are differing implementations in the case where page indexes are missing

Sounds similar to
#4761

Came across the issue at the same time as this one #6581

With this change page_locations will now equal None in both empty offset_index scenarios.
When offset_index is provided then page_locations is filled as expected:

page_locations: Some(
    [
        PageLocation {
            offset: 4,
            compressed_page_size: 109,
            first_row_index: 0,
        },
        PageLocation {
            offset: 113,
            compressed_page_size: 109,
            first_row_index: 21,
        },
       ...
   ])

jroddev · 2024-10-18T07:36:39Z

parquet/src/arrow/async_reader/mod.rs

        let offset_index = self
            .metadata
            .offset_index()
+            .filter(|index| !index.is_empty())


This panic happens when manually setting metadata.set_offset_index(Some(vec![])); as in the test empty_ofset_index_doesnt_panic_in_read_row_group

It seems the synchronous file reader does a similar filtering step, so this is probably a good addition. It may not have been an issue in the past since MetadataReader will return None rather than Some([]) for missing indexes.

jroddev · 2024-10-18T07:37:38Z

parquet/src/arrow/async_reader/mod.rs

            Some(data) => {
                let page_locations = self
                    .offset_index
+                    .filter(|index| !index.is_empty())


This panic happens when metadata is saved to a file and reloaded
such as in the test empty_offset_index_doesnt_panic_in_column_chunks or in the example external_metadata

jroddev · 2024-10-18T07:38:01Z

parquet/src/arrow/async_reader/mod.rs

    }
+
+    #[tokio::test]
+    async fn empty_ofset_index_doesnt_panic_in_read_row_group() {


These tests may not be well named. Feel free to suggest better ones

jroddev · 2024-10-18T07:40:06Z

parquet/src/arrow/async_reader/mod.rs

+                .unwrap();
+
+        let result = reader.try_collect::<Vec<_>>().await.unwrap();
+        assert_eq!(result.len(), 8);


Not sure if there's a more concise+focused way to write these tests.
Ideally this assertion checks the page_locations are like this

page_locations: Some( [ PageLocation { offset: 4, compressed_page_size: 109, first_row_index: 0, }, PageLocation { offset: 113, compressed_page_size: 109, first_row_index: 21, }, ... ])

and the other 2 tests assert that page_locations is Emtpy.

jroddev · 2024-10-18T07:40:44Z

parquet/src/arrow/async_reader/mod.rs

+    }
+
+    #[tokio::test]
+    async fn empty_offset_index_doesnt_panic_in_column_chunks() {


This test aims to replicate the writing and reloading of metadata as is done in the external_metadata example.

tustvold · 2024-10-18T08:35:57Z

Empty offset_index can end up as either depending on how its loaded.

This feels like a bug, possibly introduced by the recent metadata changes. An empty offset index and no offset index are semantically different things.

@etseidl could you possibly take a look?

etseidl · 2024-10-18T14:48:08Z

Yes, as @jroddev found, at least part of this issue is tracked by #6447. The original reader for the offset index returns an empty vec if offset indexes are requested but are not actually present in the file.

arrow-rs/parquet/src/file/page_index/index_reader.rs

Line 135 in dd5a229

None => return Ok(vec![]),

There is a test somewhere that actually expects this behavior (I'll search later for that). The async MetadataLoader instead leaves the offset index as None in that case.

arrow-rs/parquet/src/arrow/async_reader/metadata.rs

Line 174 in dd5a229

None => return Ok(()),

@alamb and I felt we couldn't reconcile the two until 54.0.0.

As to ParquetMetadataWriter, I'm honestly not sure what happened in the past when the offset index was Some([]), so I'll do some digging there. It's possible there was a behavior change there.

I'll have more time this afternoon to dig into this and look over this PR.

Edit:
Found the relevant test:

arrow-rs/parquet/src/arrow/arrow_reader/mod.rs

Lines 3587 to 3589 in dd5a229

    
           // FIXME: this test will fail when metadata parsing returns `None` for missing page 
        
           // indexes. https://github.com/apache/arrow-rs/issues/6447 
        
           assert!(builder.metadata().offset_index().unwrap()[0].is_empty());

parquet/src/arrow/async_reader/mod.rs

etseidl · 2024-10-18T21:54:54Z

parquet/src/arrow/async_reader/mod.rs

+    }
+
+    #[tokio::test]
+    async fn non_empty_ofset_index_doesnt_panic_in_read_row_group() {


Suggested change

async fn non_empty_ofset_index_doesnt_panic_in_read_row_group() {

async fn non_empty_offset_index_doesnt_panic_in_read_row_group() {

etseidl · 2024-10-18T22:52:46Z

Hit submit too soon...the rub here is that we're using the synchronous metadata reader with the asynchronous file reader (which is a scenario we're trying to support). I think the workaround in this PR is correct, but will become unnecessary once the ParquetMetaDataReader properly returns None rather than Some([]) when page indexes are not present. But it's likely good to have the extra sanity checks this PR provides.

Co-authored-by: Ed Seidl <[email protected]>

alamb

Thank you @jroddev and @etseidl for the review.

This looks good to me

…x_panic

alamb · 2024-10-21T14:51:48Z

I took the liberty of pushing a commit to this PR to fix CI / merge it up to master

I also implemented @etseidl 's suggestion in #6582 (comment) while I had it checked out.

jroddev added 4 commits October 18, 2024 17:49

Don't panic when empty offset_index is Some([])

d520be1

Remove duplicate test

e796b2e

Add a test with offset_index where page_locations doesnt end up as None

a7522a3

remove unused imports

b35fa13

github-actions bot added the parquet Changes to the parquet crate label Oct 18, 2024

jroddev commented Oct 18, 2024

View reviewed changes

etseidl approved these changes Oct 18, 2024

View reviewed changes

Update parquet/src/arrow/async_reader/mod.rs

a761b71

Co-authored-by: Ed Seidl <[email protected]>

alamb approved these changes Oct 21, 2024

View reviewed changes

alamb added 3 commits October 21, 2024 10:46

Merge remote-tracking branch 'apache/master' into jroddev/offset_inde…

95f3d9b

…x_panic

Rename test, add explanatory comments

4b617ca

cargo fmt

3fd1584

etseidl mentioned this pull request Oct 29, 2024

Return None when Parquet page indexes are not present in file #6639

Merged

tustvold merged commit 56d4713 into apache:master Oct 30, 2024

jroddev deleted the jroddev/offset_index_panic branch November 4, 2024 09:59

	async fn non_empty_ofset_index_doesnt_panic_in_read_row_group() {
	async fn non_empty_offset_index_doesnt_panic_in_read_row_group() {

Comments

Conversation

jroddev commented Oct 18, 2024

Which issue does this PR close?

Rationale for this change

Uh oh!

jroddev Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

etseidl Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jroddev Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jroddev Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jroddev Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

jroddev Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

tustvold commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etseidl commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

etseidl Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

etseidl commented Oct 18, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tustvold commented Oct 18, 2024 •

edited

Loading

etseidl commented Oct 18, 2024 •

edited

Loading