Skip to content

[native] Add support for Parquet Writer#19916

Merged
xiaoxmeng merged 1 commit intoprestodb:masterfrom
majetideepak:parquet-writer
Jun 22, 2023
Merged

[native] Add support for Parquet Writer#19916
xiaoxmeng merged 1 commit intoprestodb:masterfrom
majetideepak:parquet-writer

Conversation

@majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Jun 20, 2023 •
edited
Loading

Extend presto protocol to support HiveStorageFormat::toJson()

Test plan
Add a E2E test with Parquet as the table storage format.

== RELEASE NOTES ==

Hive Changes
* Add Prestissimo support to write Parquet table storage format.

@majetideepak majetideepak requested a review from a team as a code owner June 20, 2023 20:23
@aditi-pandit aditi-pandit changed the title [native] Add support and E2E tests for Parquet Writer [native] Add support for Parquet Writer Jun 20, 2023
aditi-pandit
aditi-pandit reviewed Jun 20, 2023
View reviewed changes
presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp Outdated
Copy link
Contributor

@aditi-pandit aditi-pandit Jun 20, 2023

Choose a reason for hiding this comment

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

It looks a bit strange for a method called toStorageFormat to return a FileFormat. Can this be called toFileFormat instead ?

Copy link
Collaborator Author

@majetideepak majetideepak Jun 21, 2023

Choose a reason for hiding this comment

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

Good point. Fixed!

aditi-pandit
aditi-pandit reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks Deepak for this change.

@majetideepak majetideepak force-pushed the parquet-writer branch 3 times, most recently from 8f60590 to 49f82fe Compare June 21, 2023 00:22
@majetideepak majetideepak requested a review from xiaoxmeng June 21, 2023 00:26
@majetideepak
Copy link
Collaborator Author

majetideepak commented Jun 21, 2023

@xiaoxmeng can you please take a look at this?

@facebook-github-bot
Copy link
Collaborator

facebook-github-bot commented Jun 21, 2023

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng
xiaoxmeng reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@majetideepak thanks for the change!

presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp Outdated
Copy link
Contributor

@xiaoxmeng xiaoxmeng Jun 21, 2023

Choose a reason for hiding this comment

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

Do we support all these formats now? @majetideepak @Yuhta Thanks!

Copy link
Contributor

@Yuhta Yuhta Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, but we will get an informative reader/writer factory error in velox anyway so it's ok to forward them all here

Copy link
Collaborator Author

@majetideepak majetideepak Jun 21, 2023

Choose a reason for hiding this comment

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

I removed them to keep it consistent with other similar conversions (eg. TableType TEMPORARY is not forwarded).
The error message is also cleaner in the protocol than a Velox factory error.

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java Outdated
Copy link
Contributor

@xiaoxmeng xiaoxmeng Jun 21, 2023

Choose a reason for hiding this comment

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

Can we add to test the format that we won't support? Thanks!

Copy link
Collaborator Author

@majetideepak majetideepak Jun 21, 2023

Choose a reason for hiding this comment

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

done

Copy link
Collaborator Author

@majetideepak majetideepak Jun 21, 2023

Choose a reason for hiding this comment

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

However, it needed extending the presto protocol.

xiaoxmeng
xiaoxmeng approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@majetideepak LGTM. Thanks!

@majetideepak
[native] Add support and E2E tests for Parquet Writer
90d569a 
Extend presto protocol to support HiveStorageFormat::toJson()
@majetideepak
Copy link
Collaborator Author

majetideepak commented Jun 21, 2023

@xiaoxmeng can you please help with the Meta Internal-Only Changes Check? Thanks.

@facebook-github-bot
Copy link
Collaborator

facebook-github-bot commented Jun 21, 2023

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@xiaoxmeng xiaoxmeng merged commit edabfe0 into prestodb:master Jun 22, 2023
@wanglinsong wanglinsong mentioned this pull request Jul 27, 2023
Merged
28 tasks
@majetideepak majetideepak deleted the parquet-writer branch February 2, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yuhta Yuhta Yuhta left review comments

@aditi-pandit aditi-pandit aditi-pandit left review comments

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

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

@majetideepak @facebook-github-bot @Yuhta @aditi-pandit @xiaoxmeng