Skip to content

Backport "Parquet dynamic batch size" #13350#13393

Closed
achopragh wants to merge 3 commits intoprestodb:masterfrom
achopragh:backport12503
Closed

Backport "Parquet dynamic batch size" #13350#13393
achopragh wants to merge 3 commits intoprestodb:masterfrom
achopragh:backport12503

Conversation

@achopragh
Copy link
Copy Markdown
Contributor

@achopragh achopragh commented Sep 12, 2019

== RELEASE NOTES ==

General Changes

Hive Changes

  • ...
  • ...

@achopragh achopragh force-pushed the backport12503 branch 2 times, most recently from 18be1ee to ef8537b Compare September 25, 2019 20:15
@wenleix wenleix mentioned this pull request Sep 25, 2019
Closed
@wenleix wenleix changed the title Add dynamic batch sizing in Parquet reader Backport "Parquet dynamic batch size" #13350 Sep 25, 2019
wenleix
wenleix reviewed Sep 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

Some lines in the original PR seems missing.

presto-hive/src/test/java/com/facebook/presto/hive/parquet/ParquetTester.java Outdated
Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 26, 2019

Choose a reason for hiding this comment

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

@achopragh : Looks like this change is missing? https://github.com/prestosql/presto/pull/58/files#diff-af0230f49180fa0b1814f3f564aa6eb7R411

Copy link
Copy Markdown
Contributor Author

@achopragh achopragh Sep 26, 2019

Choose a reason for hiding this comment

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

Fixed

presto-hive/src/test/java/com/facebook/presto/hive/parquet/AbstractTestParquetReader.java Outdated
Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 26, 2019

Choose a reason for hiding this comment

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

@achopragh Looks like some tests are missing? https://github.com/prestosql/presto/pull/58/files#diff-5d5a5b526f7467bd9f12d418be29c1f5R1490

i.e.

  • testStructMaxReadBytes
  • testArrayMaxReadBytes
  • testMapMaxReadBytes

Copy link
Copy Markdown
Contributor Author

@achopragh achopragh Sep 26, 2019

Choose a reason for hiding this comment

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

Added

Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 27, 2019

Choose a reason for hiding this comment

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

@achopragh I didn't see these methods here?

presto-hive/src/test/java/com/facebook/presto/hive/parquet/ParquetTester.java Outdated
Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 26, 2019

Choose a reason for hiding this comment

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

nit: empty line

presto-hive/src/test/java/com/facebook/presto/hive/parquet/ParquetTester.java Outdated
Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 26, 2019

Choose a reason for hiding this comment

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

nit: empty line

presto-hive/src/test/java/com/facebook/presto/hive/parquet/ParquetTester.java Outdated
Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 26, 2019

Choose a reason for hiding this comment

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

nit: empty line afterwards

presto-parquet/src/main/java/com/facebook/presto/parquet/reader/ParquetReader.java Outdated
Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 26, 2019

Choose a reason for hiding this comment

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

nit: empty line before.

@luohao @achopragh
Add dynamic batch sizing in Parquet reader
a545276 
Cherry-pick of trinodb/trino#58

Co-authored-by: Amit Chopra <amitchopra@fb.com>
luohao and others added 2 commits September 27, 2019 07:41
@luohao @achopragh
Add dynamic batch sizing in Parquet reader
9bb959e 
Cherry-pick of trinodb/trino#58

Co-authored-by: Amit Chopra <amitchopra@fb.com>
@achopragh
Merge branch 'backport12503' of github.com:achopragh/presto into back…
f8c6a2c 
…port12503

Added a few missing tests
wenleix
wenleix approved these changes Sep 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

LGTM. I will squash & rebase before merge.

@wenleix wenleix requested review from highker and zhenxiao September 27, 2019 20:36
zhenxiao
zhenxiao approved these changes Sep 27, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

looks good to me. Just one comment about constants imports.

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;
import static org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector.Category.PRIMITIVE;
import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.BINARY;
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Sep 27, 2019

Choose a reason for hiding this comment

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

any reason remove these constants? were added for schema verification
might trigger compile errors

Copy link
Copy Markdown
Contributor

@wenleix wenleix Sep 27, 2019

Choose a reason for hiding this comment

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

@zhenxiao : Will make sure Travis is green in #13473 before merge :)

@wenleix wenleix mentioned this pull request Sep 27, 2019
Merged
@wenleix
Copy link
Copy Markdown
Contributor

wenleix commented Sep 28, 2019

Merged via #13473 . Thanks for the contribution !

@wenleix wenleix closed this Sep 28, 2019
@wenleix wenleix mentioned this pull request Sep 28, 2019
Closed
@yingsu00 yingsu00 mentioned this pull request Oct 2, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhenxiao zhenxiao zhenxiao approved these changes

@highker highker Awaiting requested review from highker

+1 more reviewer

@wenleix wenleix wenleix approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@achopragh @wenleix @zhenxiao @facebook-github-bot @luohao