Skip to content

Conversation

@etseidl
Copy link
Contributor

@etseidl etseidl commented Oct 3, 2024

Which issue does this PR close?

Addresses part of #6464.

Rationale for this change

If reading metadata from an external cache that lacks the page indexes, but the footer metadata contains page index offsets valid for the original source, attempting to read the page indexes can cause a variety of errors.

What changes are included in this PR?

Modifies ParqetMetaDataReader to keep track of the size of the footer metadata, and when reading the page indexes detects if the ranges for the two structures overlap. Returns an EOF error if they do.

Are there any user-facing changes?

No, changes are to internal APIs only.

@github-actions github-actions bot added the parquet Changes to the parquet crate label Oct 3, 2024
Some((parquet_idx, field))
}

#[cfg(test)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is copied from #6463

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @etseidl -- this seems like an improvement to me

.err()
.unwrap();
assert_eq!(
err.to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this certainly is better. Let's see if anyone hits this / is confused

range
}

// one-shot parse of footer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we can update the docs to mention it updates self.metadata_size 🤔

@alamb alamb merged commit c039762 into apache:master Oct 3, 2024
16 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 3, 2024

Thanks @etseidl

@etseidl etseidl deleted the detect_missing_indexes branch October 3, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants