Parquet Cleanup by zhenxiao · Pull Request #10737 · prestodb/presto

zhenxiao · 2018-06-01T02:20:56Z

@dain @electrum @nezihyigitbasi now New Parquet Reader is production ready
this patch is to remove the old Parquet reader, and a cleanup of Parquet code

nezihyigitbasi

@zhenxiao thanks for cleaning this up. There are at least two changes in this single huge commit:

remove the old Parquet reader
cleanup/refactor

It would be great if you can split this commit up to reflect that (hope that would be easy).

nezihyigitbasi · 2018-06-15T21:09:40Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java

Any reason why you moved these methods from the PredicateUtils class to here?

it is mostly just called here. Try to not have cross module dependency

This module already depends on presto-parquet, also this class already imports PredicateUtils methods. I think we can keep these methods in their original class, the PredicateUtils.

oh, just found, HiveColumnHandle could not include in presto-parquet module, so I leave it here.

nezihyigitbasi · 2018-06-15T21:13:58Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveFileFormats.java

What's the reason for removing these tests?

these tests are for old Parquet reader only

nezihyigitbasi · 2018-06-15T21:17:22Z

presto-parquet/src/main/java/com/facebook/presto/parquet/ParquetTypeUtils.java

Is this copied from the hive module? If so, please add a comment that this is copied.

yes, will do

zhenxiao

thank you, @nezihyigitbasi I will take a try

zhenxiao · 2018-06-15T22:04:15Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveFileFormats.java

these tests are for old Parquet reader only

zhenxiao · 2018-06-15T22:04:36Z

presto-parquet/src/main/java/com/facebook/presto/parquet/ParquetTypeUtils.java

yes, will do

zhenxiao · 2018-06-15T22:06:52Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java

it is mostly just called here. Try to not have cross module dependency

zhenxiao · 2018-06-15T22:48:30Z

@nezihyigitbasi separate into 2 commits: one for remove old parquet reader, one for new Parquet reader code refactor

nezihyigitbasi

Remove old Parquet Reader LGTM. Please update the commit title as Remove the old Parquet reader

zhenxiao · 2018-06-19T00:32:04Z

thank you @nezihyigitbasi
get it updated

zhenxiao · 2018-06-27T21:20:34Z

kindly ping @nezihyigitbasi

nezihyigitbasi · 2018-07-11T22:38:58Z

@zhenxiao can you please rebase? I plan to continue reviewing this one in the next few days.

zhenxiao · 2018-07-12T14:44:49Z

get it rebased. @nezihyigitbasi

nezihyigitbasi

Remove the old Parquet Reader looks good.

Please update the commit message as ... reader.

nezihyigitbasi

Refactor Parquet code

We can update the commit title as Refactor the Parquet reader, and we can put more details in the commit message body, e.g., we can say This change creates the presto-parquet module and updates presto-hive to use it.
Now that we have removed the old reader, we can remove the parquet_optimized_reader_enabled/hive.parquet-optimized-reader.enabled property.

nezihyigitbasi · 2018-07-12T16:20:01Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java

This module already depends on presto-parquet, also this class already imports PredicateUtils methods. I think we can keep these methods in their original class, the PredicateUtils.

nezihyigitbasi · 2018-07-12T16:23:15Z

presto-parquet/pom.xml

In the top level project pom we use 1.1.1.7 version, why do we override that here?

zhenxiao · 2018-07-13T01:49:08Z

thank you, @nezihyigitbasi
comments addressed

zhenxiao · 2018-07-20T00:52:26Z

kindly ping

nezihyigitbasi · 2018-07-23T17:24:43Z

LGTM. But, before we can merge this I want to resolve all the open issues with the new Parquet reader as once we remove the old reader users won't have a fallback option:

zhenxiao · 2018-07-23T21:24:57Z

get it. Just added one more commit to remove predicate pushdown property. It is default as true for some releases.

zhenxiao · 2018-08-27T23:34:56Z

kindly ping

zhenxiao · 2018-09-05T17:57:36Z

kindly ping

zhenxiao · 2018-09-11T22:28:27Z

@nezihyigitbasi kindly ping :)

haozhun · 2018-09-11T23:42:42Z

@nezihyigitbasi is out for an extended period of time.

zhenxiao · 2018-09-11T23:46:01Z

oh, get it. thank you @haozhun @dain are you free to review?

dain

Very minor typo in comments, but otherwise looks good.

Also, can you verify that #10844 is no longer an issue?

dain · 2018-09-24T18:32:45Z

pom.xml

Can you retitle this commit "Move Parquet reader to new presto-parquet module"?

dain · 2018-09-24T18:39:10Z

presto-parquet/src/main/java/com/facebook/presto/parquet/predicate/Predicate.java

The javadocs here have two spaces where Parquet was removed.

zhenxiao · 2018-09-25T03:28:40Z

@dain thank you, I get comments addressed
#10844 should be fixed in 203+ version

zhenxiao · 2018-10-02T03:47:01Z

kindly ping @nezihyigitbasi @dain

zhenxiao · 2018-10-12T20:27:16Z

kindly ping @nezihyigitbasi @dain

dain · 2018-10-12T21:38:34Z

It looks good to me. @nezihyigitbasi do you have any additional concerns?

nezihyigitbasi

LGTM too.

nezihyigitbasi · 2018-10-15T16:44:01Z

Merged, thanks @zhenxiao.

facebook-github-bot added the CLA Signed label Jun 1, 2018

dain requested a review from nezihyigitbasi June 15, 2018 00:48

nezihyigitbasi reviewed Jun 15, 2018

View reviewed changes

nezihyigitbasi self-assigned this Jun 15, 2018

zhenxiao commented Jun 15, 2018

View reviewed changes

zhenxiao force-pushed the parquet-clean branch from 05ca258 to a678dbb Compare June 15, 2018 22:47

zhenxiao force-pushed the parquet-clean branch from a678dbb to f46f843 Compare June 18, 2018 19:29

nezihyigitbasi reviewed Jun 18, 2018

View reviewed changes

zhenxiao force-pushed the parquet-clean branch from f46f843 to 095eb4e Compare June 19, 2018 00:31

zhenxiao force-pushed the parquet-clean branch from 095eb4e to 744de4d Compare July 12, 2018 14:42

nezihyigitbasi reviewed Jul 12, 2018

View reviewed changes

zhenxiao force-pushed the parquet-clean branch from 744de4d to f5ebed7 Compare July 13, 2018 01:42

zhenxiao force-pushed the parquet-clean branch 3 times, most recently from a603af9 to fd2e9cd Compare July 19, 2018 22:49

zhenxiao force-pushed the parquet-clean branch from fd2e9cd to ea9c684 Compare July 23, 2018 21:23

zhenxiao force-pushed the parquet-clean branch from ea9c684 to ff4e63d Compare July 26, 2018 18:57

zhenxiao force-pushed the parquet-clean branch from ff4e63d to 8c129ce Compare September 5, 2018 17:57

zhenxiao force-pushed the parquet-clean branch from 8c129ce to 46fdae0 Compare September 11, 2018 22:27

zhenxiao force-pushed the parquet-clean branch from 46fdae0 to ae24937 Compare September 14, 2018 20:46

dain suggested changes Sep 24, 2018

View reviewed changes

zhenxiao force-pushed the parquet-clean branch from ae24937 to f62eafb Compare September 25, 2018 03:27

zhenxiao added 4 commits October 1, 2018 20:09

Remove the old Parquet reader

888228e

Move Parquet reader to new presto-parquet module

f461337

Remove hive.parquet-optimized-reader.enabled property

c571933

Remove hive.parquet-predicate-pushdown.enabled property

9dd7f20

zhenxiao force-pushed the parquet-clean branch from f62eafb to 9dd7f20 Compare October 2, 2018 03:46

dain self-assigned this Oct 2, 2018

dain approved these changes Oct 12, 2018

View reviewed changes

nezihyigitbasi approved these changes Oct 13, 2018

View reviewed changes

nezihyigitbasi merged commit 7a22609 into prestodb:master Oct 15, 2018

sajjoseph mentioned this pull request Nov 3, 2018

Support Nested Schema Evolution in Parquet for Presto #6675. Copy of … #10158

Closed

zhenxiao deleted the parquet-clean branch March 3, 2020 22:14

Conversation

zhenxiao commented Jun 1, 2018

Uh oh!

nezihyigitbasi 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!

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!

zhenxiao commented Jun 15, 2018

Uh oh!

nezihyigitbasi left a comment

Choose a reason for hiding this comment

Uh oh!

zhenxiao commented Jun 19, 2018

Uh oh!

zhenxiao commented Jun 27, 2018

Uh oh!

nezihyigitbasi commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhenxiao commented Jul 12, 2018

Uh oh!

nezihyigitbasi left a comment

Choose a reason for hiding this comment

Uh oh!

nezihyigitbasi 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!

zhenxiao commented Jul 13, 2018

Uh oh!

zhenxiao commented Jul 20, 2018

Uh oh!

nezihyigitbasi commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhenxiao commented Jul 23, 2018

Uh oh!

zhenxiao commented Aug 27, 2018

Uh oh!

zhenxiao commented Sep 5, 2018

Uh oh!

zhenxiao commented Sep 11, 2018

Uh oh!

haozhun commented Sep 11, 2018

Uh oh!

zhenxiao commented Sep 11, 2018

Uh oh!

dain 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

nezihyigitbasi commented Jul 11, 2018 •

edited

Loading

nezihyigitbasi commented Jul 23, 2018 •

edited

Loading