Skip to content

PARQUET-374: Add api to read dictionary from each column chunk for predicate pushdown - #270

Open
zhenxiao wants to merge 1 commit into
apache:masterfrom
zhenxiao:dictionary
Open

PARQUET-374: Add api to read dictionary from each column chunk for predicate pushdown#270
zhenxiao wants to merge 1 commit into
apache:masterfrom
zhenxiao:dictionary

Conversation

@zhenxiao

Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please make sure indentation is consistent with existing code

@julienledem

Copy link
Copy Markdown
Member

Thanks for the contribution.
Comments are mostly about reorganizing code in the places that make more sense.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@julienledem I tested against both PAGE_V1 and PAGE_V2, this heuristics seems working OK, an ideal solution is to use PageEncodingStats, this is a heuristics workaround. I put todo here as a next step

@zhenxiao

Copy link
Copy Markdown
Author

thank you, @julienledem get comments addressed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Possibly you could check if you can read only the dictionary page instead of the entire column chunk.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@julienledem seems like could not decide the compressed page size until reading the page header, so at this time(before reading page header), do not know the actual size to read. I will leave todo here

@zhenxiao

Copy link
Copy Markdown
Author

thank you @julienledem get comments addressed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@zhenxiao Isn't this name a little misleading? I believe this is better as getCurrentRowGroupDictionaries(...) because it just returns the current row group and doesn't advance.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@danielcweeks my bad, forgot to advance the row group, just get it fixed. Following the readNextRowGroup, call this readNextRowGroupDictionaries

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@danielcweeks you are correct. read dictionaries should not advance the row group, only after reading the whole row group does. I get the method name updated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@julienledem Unfortunately, I don't think this API works very well for the uses cases we have right now. The issue @zhenxiao is seeing is that he wants to identify the row groups not filtered prior to processing them. This interface requires that you do both in lock step. The filter api has the same problem in that it basically needs to do up front row group filtering without loading any column data. We would have to be able to advance the dictionary separately from the row group, which would make this interface rather clunky.

Overall, I think the static interface was closer to what we currently need (though I can see how this approach can be used to filter while processing). Maybe we just need to separate out the reading of the dictionary as a utility that gets used by this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@julienledem @danielcweeks as @danielcweeks explained, the use case is,
read footer, for each row group read blockMetaData and Dictionary for each predicate column chunks, decide whether to read the row group or not based on stats and dictionary, keep the list of to-be-scanned row groups, and then start scanning the necessary row groups.
In this case advance dictionary is separate from advancing row group data.
I am thinking about the following options:

  1. advance blockId in both nextRowGroup() and nextRowGroupDictionaries(), while, they should not be called interleaving or together, only one of them could be called in the application
  2. have a user level advanceBlock() which will advance blockId, then user could decide when to call nextRowGroup(), when to advanceBlock(), and when to read dictionaries, this could be a little bit too flexible, and existing applications calling nextRowGroup() needs to be updated
  3. have a static method in ParquetFileReader, pass BlockMetaData, configuration, path as input parameters
    Agree with @danielcweeks option#3 is better. what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants