Skip reading Parquet pages using Column Indexes feature. by shangxinli · Pull Request #17284 · prestodb/presto

shangxinli · 2022-02-11T02:53:39Z

Port some code from parquet-mr repo https://github.com/apache/parquet-mr
Co-authored-by: Gabor Szadovszky gabor.szadovszky@cloudera.com

More details about Parquet Column Indexes feature:

Column Indexes also named as page level indexes that have min/max values for each page in a given column chunk. When reading pages, a reader doesn't need to process the page header to determine whether the page could be skipped based on the statistics. More information about this feature can be found https://github.com/apache/parquet-format/blob/master/PageIndex.md

Test plan - (Please fill in how you tested your changes)

This feature was tested in the Uber staging environment and then rolled out to production for 5+ months.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

General Changes
* Add support to use Parquet page-level statistics to skip pages  

Hive Changes
* Introduce a flag "hive.parquet-column-index-filter-enabled" to turn on/off this feature. By default it is off.

shangxinli · 2022-02-11T18:27:33Z

@rongrong @zhenxiao @beinan I just created this new PR for the Parquet column index to rebase. If you can merge it, it would be great! The old PRs can be found #17216 and #16963

beinan

Thank you @shangxinli for you great work and rebase! Just two minor var naming issue, I guess you might miss these two places during the rebase

beinan · 2022-02-15T00:59:47Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

setColumnIndexFilterEnabled

If I change to 'setColumnIndexFilterEnabled' it always causes some odd test failures that seem unrelated. I changed it to setColumnIndexFilter

It seems even we change to setColumnIndexFilterEnabled still doesn't work ether. Just keep the original name for now.

zhenxiao

@shangxinli nice work
A few more comments
we will speedup to merge this PR

zhenxiao · 2022-02-14T07:41:36Z

...parquet/src/main/java/com/facebook/presto/parquet/predicate/TupleDomainParquetPredicate.java

s/canDropCanWithRangeStats/canDropWithRangeStatistics/g

zhenxiao · 2022-02-14T07:41:53Z

...parquet/src/main/java/com/facebook/presto/parquet/predicate/TupleDomainParquetPredicate.java

return columnDomain.intersect(domain).isNone();

zhenxiao · 2022-02-14T07:42:09Z

...parquet/src/main/java/com/facebook/presto/parquet/predicate/TupleDomainParquetPredicate.java

s/ciConversions/conversions/g

zhenxiao · 2022-02-14T07:55:49Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/AbstractColumnReader.java

currentRow = currentRow + 1;

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/AbstractColumnReader.java

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/PageReader.java

zhenxiao · 2022-02-15T04:01:51Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/PageReader.java

could we move pageIndex = pageIndex + 1 out of the function call?

zhenxiao · 2022-02-15T04:10:56Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java

the comment is not useful. let's remove it

zhenxiao · 2022-02-15T04:17:14Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java

should we consider merge PageRanges with OffsetRange into one class? They are quite similar

It seems the same but we have to convert between int/long.

yes, shall we use merge the two classes?

The variable 'length' is int in PageRanges but long in OffsetRanges. If we merge, we need to cast, which we should generally avoid casting as much as we can.

I am inclined to merge PageRanges into OffsetRange. We are casting OffsetRange length to int in code below. Doing the merge could remove duplicate code, and save the cast, too.

zhenxiao · 2022-02-15T04:18:00Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java

s/rangeStartPos/startPosition/g

zhenxiao

@shangxinli mostly good
a few remaining minor things

zhenxiao · 2022-02-16T08:24:42Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ColumnIndexFilterUtils.java

@shangxinli could we fix here?

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/AbstractColumnReader.java

zhenxiao · 2022-02-16T08:30:59Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ColumnIndexFilterUtils.java

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/PageReader.java

zhenxiao · 2022-02-16T08:32:12Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java

yes, shall we use merge the two classes?

shangxinli · 2022-02-17T15:35:41Z

@zhenxiao @beinan @rongrong Just addressed the comments. Please have a look if you have more.

zhenxiao

hi @shangxinli mostly look
only 2 remaining minor issues. others looks good to me

zhenxiao · 2022-02-18T04:33:08Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java

we are casting OffsetRange length to int, shall we use OffsetRange directly?

Sounds good

zhenxiao · 2022-02-18T04:34:23Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java

I am inclined to merge PageRanges into OffsetRange. We are casting OffsetRange length to int in code below. Doing the merge could remove duplicate code, and save the cast, too.

zhenxiao · 2022-02-18T04:34:40Z

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ColumnIndexFilterUtils.java

kindly ping

shangxinli · 2022-02-18T22:14:51Z

Addressed the last two comments from @zhenxiao.

zhenxiao · 2022-02-19T07:54:13Z

looks good to me
hi @shangxinli could you please keep the last 2 comments addressed, and squash all commits into one?
Then @beinan and I could merge it

shangxinli · 2022-02-19T16:30:48Z

Yeah, I did. But the test keep failing with the error "didn't finish within the time-out 60000". I revert it but still failed. It shouldn't be related to the change itself. I will add back the last commit and squash.

Port some code from parquet-mr repo https://github.com/apache/parquet-mr Co-authored-by: Gabor Szadovszky <gabor.szadovszky@cloudera.com> More details about Parquet Column Indexes feature: Column Indexes also named as page level indexes that have min/max values for each page in a given column chunk. When reading pages, a reader doesn't need to process the page header to determine whether the page could be skipped based on the statistics. More information about this feature can be found https://github.com/apache/parquet-format/blob/master/PageIndex.md

aweisberg · 2022-02-22T19:15:23Z

Does this break the build?

[ERROR] Failed to execute goal on project presto-parquet: Could not resolve dependencies for project com.facebook.presto:presto-parquet:jar:0.272-SNAPSHOT: Failed to collect dependencies at junit:junit:jar:4.13.1: Failed to read artifact descriptor for junit:junit:jar:4.13.1: Could not transfer artifact junit:junit:pom:4.13.1 from/to nexus (https://maven.thefacebook.com/nexus/content/groups/public): Transfer failed for https://maven.thefacebook.com/nexus/content/groups/public/junit/junit/4.13.1/junit-4.13.1.pom: No route to host (Host unreachable) -> [Help 1]

Looks like a real version of junit, maybe something wrong @ Central?

beinan · 2022-02-22T21:42:56Z

Does this break the build?

[ERROR] Failed to execute goal on project presto-parquet: Could not resolve dependencies for project com.facebook.presto:presto-parquet:jar:0.272-SNAPSHOT: Failed to collect dependencies at junit:junit:jar:4.13.1: Failed to read artifact descriptor for junit:junit:jar:4.13.1: Could not transfer artifact junit:junit:pom:4.13.1 from/to nexus (https://maven.thefacebook.com/nexus/content/groups/public): Transfer failed for https://maven.thefacebook.com/nexus/content/groups/public/junit/junit/4.13.1/junit-4.13.1.pom: No route to host (Host unreachable) -> [Help 1]

Looks like a real version of junit, maybe something wrong @ Central?

hmmm, very likely, let me remove the real junit version. Just posted a PR to fix it #17334 @aweisberg

shangxinli mentioned this pull request Feb 11, 2022

Skip reading Parquet pages using Column Indexes feature of Parquet 1.11 #17216

Closed

shangxinli force-pushed the column_index_final_one_commit branch 2 times, most recently from c87451b to 78295ee Compare February 11, 2022 17:08

beinan reviewed Feb 15, 2022

View reviewed changes

zhenxiao requested changes Feb 15, 2022

View reviewed changes

zhenxiao reviewed Feb 16, 2022

View reviewed changes

shangxinli force-pushed the column_index_final_one_commit branch 4 times, most recently from d3e1180 to f8d63e2 Compare February 17, 2022 06:16

zhenxiao approved these changes Feb 18, 2022

View reviewed changes

shangxinli force-pushed the column_index_final_one_commit branch from b5164a7 to cde2032 Compare February 18, 2022 23:13

beinan approved these changes Feb 18, 2022

View reviewed changes

shangxinli force-pushed the column_index_final_one_commit branch from ce3b0e4 to 0a98e6d Compare February 19, 2022 16:34

shangxinli force-pushed the column_index_final_one_commit branch from 0a98e6d to 58db92d Compare February 20, 2022 17:48

beinan merged commit 3cb1ee2 into prestodb:master Feb 21, 2022

varungajjala mentioned this pull request Mar 22, 2022

Add release notes for 0.272 #17499

Closed

9 tasks

asjadsyed mentioned this pull request Mar 23, 2022

Add release notes for 0.272 #17510

Closed

9 tasks

asjadsyed mentioned this pull request Apr 1, 2022

Add release notes for 0.272 #17564

Merged

8 tasks

Conversation

shangxinli commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shangxinli commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beinan left a comment

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!

zhenxiao left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangxinli commented Feb 17, 2022

Uh oh!

zhenxiao left a comment

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

shangxinli commented Feb 11, 2022 •

edited

Loading

shangxinli commented Feb 11, 2022 •

edited

Loading

aweisberg commented Feb 22, 2022 •

edited

Loading