Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GenericColumnReader::skip_records Missing OffsetIndex Fallback #2433

Closed
tustvold opened this issue Aug 12, 2022 · 3 comments
Closed

Add GenericColumnReader::skip_records Missing OffsetIndex Fallback #2433

tustvold opened this issue Aug 12, 2022 · 3 comments
Assignees
Labels
enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate

Comments

@tustvold
Copy link
Contributor

tustvold commented Aug 12, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

GenericColumnReader::skip_records always calls to PageReader::peek_next_page. If the file doesn't have an OffsetIndex or this hasn't been loaded, this will result in an error.

Describe the solution you'd like

I would like some mechanism for GenericColumnReader to fallback to decoding the level data from the page and using this to skip decoding. This may require adding a PageReader::has_page_metadata() -> bool or something similar to detect this situation.

Not only would this avoid needing to know ahead of time if index information is available before pushing down RowFilter, etc... but would also allow these APIs to work even for files without a PageIndex - as just decoding the levels will still be significantly faster than also decoding the values

Describe alternatives you've considered

Additional context

FYI @Ted-Jiang

@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Aug 12, 2022
@Ted-Jiang
Copy link
Member

Make sense. @tustvold
After reading the code i found:
there are two kinds implements of skip_next_page

  1. sync_reader: only support skip according to page_index as you mention.
  2. async_reader: only support skip according to parsing all dataPage headers in column chunk.

in previous versions of the format, Statistics are stored for ColumnChunks in ColumnMetaData and for individual pages inside DataPageHeader structs. When reading pages, a reader had to process the page header to determine whether the page could be skipped based on the statistics. This means the reader had to access all pages in a column, thus likely reading most of the column data from disk. (copy from parquet doc)

I think the page_index intend to not read all pageHeader.
So i think we should support all the two methods in both async or sync (read all page header as fallback) 🤔

@Ted-Jiang
Copy link
Member

working on 😊

@Ted-Jiang
Copy link
Member

@tustvold we can close this issue.

@alamb alamb added the parquet Changes to the parquet crate label Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate
Projects
None yet
Development

No branches or pull requests

3 participants