[Velox] Table writer 2: Add write protocol interface by gggrace14 · Pull Request #2845 · facebookincubator/velox

gggrace14 · 2022-10-14T09:00:03Z

Add WriteProtocol interface to allow systems to implement different
write and commit behaviors, including write & target directories
and file names, commit actions and output, etc. Support registering
WriteProtocols by CommitStrategy.

Add two base implementations of WriteProtocols for Hive connector.
getWriterParameters() is where table writer can get required parameters
including write & target directories and file names.
commit() can be extended to perform commit actions.

netlify · 2022-10-14T09:00:08Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`80a4a97`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/635a084e9049d70008a70201

facebook-github-bot · 2022-10-14T09:00:24Z

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

mbasmanova

@gggrace14 Some questions and comments.

mbasmanova · 2022-10-19T17:54:45Z

velox/connectors/CMakeLists.txt

Any particular reason not to add WriteProtocol.cpp to velox_connector?

mbasmanova · 2022-10-19T17:56:40Z

velox/connectors/WriteProtocol.h

naming: getCommitStrategy -> commitStrategy

mbasmanova · 2022-10-19T17:59:39Z

velox/connectors/WriteProtocol.h

Any particular reason this API doesn't take CommitStrategy? It would be more natural to specify both CommitStrategy and WriteProtocol when registering as opposed to fetching CommitStrategy from the protocol.

Okay, used to pass CommitStrategy to this func together wi/ the WriteProtocol. Changing it back to the more natural way.

mbasmanova · 2022-10-19T18:00:15Z

velox/connectors/WriteProtocol.h

No need to define this again. The base implementation is the same.

I still see this method here. I assume it can be removed, no?

mbasmanova · 2022-10-19T18:00:50Z

velox/connectors/WriteProtocol.h

Would you document that it is valid to return nullptr?

mbasmanova · 2022-10-19T18:02:00Z

velox/connectors/hive/CMakeLists.txt

Any particular reason not to include HiveWriteProtocol.cpp into velox_hive_connector ? Why "OBJECT"?

Revising. I'd like to include it into velox_hive_connector, which is easier. Just saw HivePartitionFunction.cpp was in a separate item

mbasmanova · 2022-10-19T18:02:32Z

velox/connectors/CMakeLists.txt

Any particular reason not to add WriteProtocol.cpp to velox_connector?

Revising. Adding it to velox_connector makes it easier.

mbasmanova · 2022-10-19T18:03:36Z

velox/connectors/hive/HiveWriteProtocol.h

Would you document this constructor and explain when the caller should specify writeFileName and writeDirectory? Should there be some checks based on updateMode?

It would really be nice to document this constructor. Otherwise, it will be hard for future readers / users of the API to understand how to use it properly.

Sorry, missed this comment previously. Moving the documentation from data members to this constructor. Relations between write & target and updateMode might not be straightforward, which is determined by a concrete implementation of WriteProtocol::getWriterParameters(). So might not want to have checks on the updateMode.

mbasmanova · 2022-10-19T18:04:24Z

velox/exec/TableWriter.h

Why do we need to expose these?

mbasmanova

@gggrace14 Looks good to me % some questions.

mbasmanova · 2022-10-21T15:49:36Z

velox/connectors/hive/HiveConnector.h

Nice. Would you add a comment explaining this config?

mbasmanova · 2022-10-21T15:49:58Z

velox/connectors/hive/HiveWriteProtocol.cpp

typo: used -> be used

mbasmanova · 2022-10-21T15:50:43Z

velox/connectors/hive/HiveWriteProtocol.h

It would really be nice to document this constructor. Otherwise, it will be hard for future readers / users of the API to understand how to use it properly.

Pass LocationHandle to HiveInsertTableHandle, and pass HiveInsertTableHandle instead of an actual write file name to HiveDataSink. This allows Velox callers to pass more info to HiveDataSink that is used by TableWriter. Thus this gets TableWriter ready for more flexible write and commit strategies. Allow multiple writers in HiveDataSink, to make HiveDataSink and TableWriter ready for partitioned table writing. For now we only keep one writer. Also make HiveDataSink generate a random file name interally, rather than relying Velox callers to pass in a file name. Users could extend this behavior with WriteProtocols, whose support will come in next.

Add WriteProtocol interface to allow systems to implement different write and commit behaviors, including write & target directories and file names, commit actions and output, etc. Support registering WriteProtocols by CommitStrategy. Add two base implementations of WriteProtocols for Hive connector. getWriterParameters() is where table writer can get required parameters including write & target directories and file names. commit() can be extended to perform commit actions.

Without OBJECT label for velox_hive_conector, build of presto_server sees error like undefined reference to HiveTableHandle::HiveTableHandle(). facebookincubator#2897 added OBJECT, but the next PR facebookincubator#2845 removed it according to change history. Actually PR facebookincubator#2845 did not touch OBJECT according to the PR page. It is likely due to file merge of CMakeList.txt.

Summary: Without OBJECT label for velox_hive_conector, build of presto_server sees error like undefined reference to HiveTableHandle::HiveTableHandle(). #2897 added OBJECT, but the next PR #2845 removed it according to change history. Actually PR #2845 did not touch OBJECT according to the PR page. It is likely due to file merge of CMakeList.txt. Pull Request resolved: #3094 Reviewed By: kgpai, mbasmanova Differential Revision: D41031123 Pulled By: gggrace14 fbshipit-source-id: f50b53b01e5ab2296cd7cb8ee826cd41cf5f4916

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 14, 2022

gggrace14 requested a review from mbasmanova October 14, 2022 09:21

gggrace14 marked this pull request as ready for review October 14, 2022 09:21

gggrace14 requested a review from spershin October 14, 2022 09:21

gggrace14 force-pushed the write_protocol branch 2 times, most recently from 91537bb to 284a9fd Compare October 14, 2022 19:41

gggrace14 changed the title ~~[Velox] Add write protocol interface~~ [Velox] Table writer 2: Add write protocol interface Oct 14, 2022

gggrace14 force-pushed the write_protocol branch 6 times, most recently from 3f78584 to 27f1fe7 Compare October 19, 2022 07:43

mbasmanova requested a review from majetideepak October 19, 2022 17:25

mbasmanova reviewed Oct 19, 2022

View reviewed changes

gggrace14 force-pushed the write_protocol branch 3 times, most recently from b827fee to 3fd5827 Compare October 21, 2022 03:39

mbasmanova approved these changes Oct 21, 2022

View reviewed changes

gggrace14 requested a review from aditi-pandit October 21, 2022 16:43

gggrace14 force-pushed the write_protocol branch 3 times, most recently from a3f72b8 to 697e52d Compare October 27, 2022 02:47

gggrace14 added 2 commits October 26, 2022 21:24

gggrace14 force-pushed the write_protocol branch from 697e52d to 80a4a97 Compare October 27, 2022 04:25

facebook-github-bot closed this in bdd209c Oct 27, 2022

gggrace14 deleted the write_protocol branch October 27, 2022 23:39

gggrace14 mentioned this pull request Nov 4, 2022

[Velox] Bring back OBJECT to velox_hive_connector #3094

Closed

Conversation

gggrace14 commented Oct 14, 2022

Uh oh!

netlify bot commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

facebook-github-bot commented Oct 14, 2022

Uh oh!

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

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify bot commented Oct 14, 2022 •

edited

Loading