Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add initial support for sorted writer #6142

Closed
wants to merge 1 commit into from
Closed

Add initial support for sorted writer #6142

wants to merge 1 commit into from

Conversation

kewang1024
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented Aug 17, 2023 •
edited

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit bb645c4
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/64ff95a5d5f5070008db9f79

@facebook-github-bot 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 Aug 17, 2023
@kewang1024 kewang1024 force-pushed the sorted-writer branch 2 times, most recently from 249d6b8 to 038db1a Compare August 17, 2023 20:34
@kewang1024 kewang1024 force-pushed the sorted-writer branch 2 times, most recently from 7f1a8e3 to 0a52154 Compare September 1, 2023 06:08
xiaoxmeng
xiaoxmeng reviewed Sep 7, 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.

@kewang1024 overall looks good. Thanks!

velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
velox/dwio/common/SortingWriter.h Show resolved Hide resolved
velox/common/config/SpillConfig.h Outdated Show resolved Hide resolved
velox/common/config/SpillConfig.h Show resolved Hide resolved
velox/common/config/SpillConfig.h Show resolved Hide resolved
velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
velox/dwio/common/Writer.h Outdated Show resolved Hide resolved
velox/exec/Driver.h Show resolved Hide resolved
velox/exec/SortBuffer.cpp Outdated Show resolved Hide resolved
velox/exec/tests/TableWriteTest.cpp Show resolved Hide resolved
@facebookincubator facebookincubator deleted a comment from xiaoxmeng Sep 7, 2023
@kewang1024 kewang1024 force-pushed the sorted-writer branch 3 times, most recently from 5fe8d2a to b3ce287 Compare September 8, 2023 01:50
@kewang1024 kewang1024 marked this pull request as ready for review September 8, 2023 01:52
xiaoxmeng
xiaoxmeng approved these changes Sep 8, 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.

@kewang1024 LGTM % minors.

velox/connectors/Connector.h Outdated Show resolved Hide resolved
velox/exec/TableWriter.cpp Outdated Show resolved Hide resolved
velox/exec/tests/TableWriteTest.cpp Outdated Show resolved Hide resolved
velox/exec/tests/TableWriteTest.cpp Outdated Show resolved Hide resolved
velox/exec/tests/TableWriteTest.cpp Outdated Show resolved Hide resolved
xiaoxmeng
xiaoxmeng reviewed Sep 8, 2023
View reviewed changes
velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@kewang1024 kewang1024 force-pushed the sorted-writer branch 3 times, most recently from 21b0a52 to 352c539 Compare September 8, 2023 23:18
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

xiaoxmeng
xiaoxmeng reviewed Sep 9, 2023
View reviewed changes
velox/connectors/hive/HiveDataSink.h Outdated Show resolved Hide resolved
velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
velox/dwio/common/SortingWriter.h Outdated

virtual void abort() override;

std::unique_ptr<exec::SortBuffer> sortBuffer_;
Copy link
Contributor

Choose a reason for hiding this comment

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

const std::unique_ptr<exec::SortBuffer> sortBuffer_;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to call reset on sortBuffer_ in abort function, so can't set it to const

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Then move outputWriter_ ahead of sortBuffer_ as the former is a const member. Thanks!

velox/vector/BaseVector.h Outdated Show resolved Hide resolved
@kewang1024 kewang1024 force-pushed the sorted-writer branch 4 times, most recently from 23d2910 to 0934e14 Compare September 9, 2023 04:49
xiaoxmeng
xiaoxmeng approved these changes Sep 9, 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.

@kewang1024 Thanks for iterating!

velox/connectors/hive/HiveDataSink.h Outdated Show resolved Hide resolved
velox/connectors/hive/HiveDataSink.cpp Outdated Show resolved Hide resolved
velox/dwio/common/SortingWriter.h Outdated

virtual void abort() override;

std::unique_ptr<exec::SortBuffer> sortBuffer_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Then move outputWriter_ ahead of sortBuffer_ as the former is a const member. Thanks!

xiaoxmeng
xiaoxmeng reviewed Sep 10, 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.

@kewang1024 can you squash the commits into one? Thanks!

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

kewang1024 added a commit to kewang1024/velox that referenced this pull request Sep 11, 2023
@kewang1024 @facebook-github-bot
Add initial support for sorted writer (facebookincubator#6142)
58d7355 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

@facebook-github-bot
Copy link
Contributor

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

kewang1024 added a commit to kewang1024/velox that referenced this pull request Sep 11, 2023
@kewang1024 @facebook-github-bot
Add initial support for sorted writer (facebookincubator#6142)
1e546d3 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

kewang1024 added a commit to kewang1024/velox that referenced this pull request Sep 11, 2023
@kewang1024 @facebook-github-bot
Add initial support for sorted writer (facebookincubator#6142)
caff52a 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

kewang1024 added a commit to kewang1024/velox that referenced this pull request Sep 11, 2023
@kewang1024 @facebook-github-bot
Add initial support for sorted writer (facebookincubator#6142)
f866a8d 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

kewang1024 added a commit to kewang1024/velox that referenced this pull request Sep 11, 2023
@kewang1024 @facebook-github-bot
Add initial support for sorted writer (facebookincubator#6142)
cd5580e 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

@kewang1024 @facebook-github-bot
Add initial support for sorted writer (facebookincubator#6142)
bb645c4 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49103233

@facebook-github-bot
Copy link
Contributor

@kewang1024 merged this pull request in f6895df.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit f6895df4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
@kewang1024 @codyschierbeck
Add initial support for sorted writer (facebookincubator#6142)
dd49c33 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024

fbshipit-source-id: d10f3cc45d531315855a870c31db1cb3b90d20f6
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
@kewang1024 @codyschierbeck
Add initial support for sorted writer (facebookincubator#6142)
c8d210c 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024

fbshipit-source-id: d10f3cc45d531315855a870c31db1cb3b90d20f6
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
@kewang1024 @codyschierbeck
Add initial support for sorted writer (facebookincubator#6142)
772744c 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024

fbshipit-source-id: d10f3cc45d531315855a870c31db1cb3b90d20f6
ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
@kewang1024 @ericyuliu
Add initial support for sorted writer (facebookincubator#6142)
9fda724 
Summary: Pull Request resolved: facebookincubator#6142

Reviewed By: xiaoxmeng

Differential Revision: D49103233

Pulled By: kewang1024

fbshipit-source-id: d10f3cc45d531315855a870c31db1cb3b90d20f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kewang1024 @facebook-github-bot @xiaoxmeng