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

[native] Refactor TaskManagerTest to not create common::WriterOptions instance #23480

Merged
merged 1 commit into from
Aug 22, 2024
Merged

[native] Refactor TaskManagerTest to not create common::WriterOptions instance #23480

merged 1 commit into from
Aug 22, 2024

Conversation

kewang1024
Copy link
Collaborator

@kewang1024 kewang1024 commented Aug 20, 2024 •
edited
Loading

We want to make common::WriterOptions::processConfigs function pure virtual
to make sure every data format will implement its own.
As a result, we should not create dwio::common::WriterOptions instance anymore

@kewang1024 kewang1024 requested a review from a team as a code owner August 20, 2024 22:44
xiaoxmeng
xiaoxmeng previously approved these changes Aug 21, 2024
View reviewed changes
yingsu00
yingsu00 requested changes Aug 21, 2024
View reviewed changes
presto-native-execution/presto_cpp/main/tests/TaskManagerTest.cpp
@@ -29,6 +29,7 @@
#include "velox/dwio/common/FileSink.h"
#include "velox/dwio/common/WriterFactory.h"
#include "velox/dwio/common/tests/utils/BatchMaker.h"
#include "velox/dwio/dwrf/writer/Writer.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

@kewang1024 Can you please explain why making this change? The TaskManagerTest is file-format orthogonal and should NOT depend on DWRF. We've been trying to remove DWRF dependencies from HiveConnectorTest as well, e.g. facebookincubator/velox#10150

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Essentially we want to make common::WriterOptions::processConfigs function pure virtual to make sure every data format will implement its own.
As a result, common::WriterOptions can not be initiated.

cc: @xiaoxmeng

Copy link
Contributor

Choose a reason for hiding this comment

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

@yingsu00 good point. We might need this change as we disable creation of dwio common writer options as it is an abstract class now. If we want to cover parquet file format here, then we can make this a parameterized test with different file formats. But I guess it is not required for TaskManagerTest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break Meta internal test as Velox side change on Writer options is landed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this header still?

@yingsu00 yingsu00 self-assigned this Aug 21, 2024
majetideepak
majetideepak previously requested changes Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

Please use [native] prefix for the commit.

yingsu00
yingsu00 reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@yingsu00 yingsu00 left a comment

Choose a reason for hiding this comment

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

I'm removing the "request change" to unblock Meta internal build break for now. We will revisit this issue soon. Since "dwio::common" used to work, I think there may be a better solution than making presto_cpp/main tests depend on DWRF.

@yingsu00 yingsu00 self-requested a review August 21, 2024 06:01
@kewang1024 kewang1024 changed the title Use dwrf::WriterOptions instead of common::WriterOptions [native] Use dwrf::WriterOptions instead of common::WriterOptions Aug 21, 2024
@kewang1024 kewang1024 changed the title [native] Use dwrf::WriterOptions instead of common::WriterOptions [native] Use dwrf::WriterOptions instead of common::WriterOptions in TaskManagerTest Aug 21, 2024
yingsu00
yingsu00 previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@yingsu00 yingsu00 left a comment

Choose a reason for hiding this comment

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

Approving it for now to unblock Meta workloads

yingsu00
yingsu00 reviewed Aug 22, 2024
View reviewed changes
presto-native-execution/presto_cpp/main/tests/TaskManagerTest.cpp Outdated
@@ -299,7 +300,8 @@ class TaskManagerTest : public testing::Test {
void writeToFile(
const std::string& filePath,
const std::vector<RowVectorPtr>& vectors) {
auto options = std::make_shared<dwio::common::WriterOptions>();
std::shared_ptr<dwio::common::WriterOptions> options =
Copy link
Contributor

@yingsu00 yingsu00 Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kewang1024 @xiaoxmeng Does the following not work?

const std::shared_ptr<dwio::common::WriterFactory> writerFactory_;

In constructor, initialize the writerFactory_ as:

writerFactory_(dwio::common::getWriterFactory(dwio::common::FileFormat::DWRF))

Then in writeToFile:

auto options = writerFactory_->createWriterOptions();

This way we don't need to include "velox/dwio/dwrf/writer/Writer.h" but just the dwio::common ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it works as in the production code. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @yingsu00 for your good suggestion!

@yingsu00 yingsu00 self-requested a review August 22, 2024 01:38
@kewang1024 kewang1024 dismissed stale reviews from yingsu00 and xiaoxmeng via 493840c August 22, 2024 06:51
@kewang1024 kewang1024 force-pushed the refactor-WriterOption branch 2 times, most recently from 493840c to 1513ddc Compare August 22, 2024 06:52
@kewang1024 kewang1024 changed the title [native] Use dwrf::WriterOptions instead of common::WriterOptions in TaskManagerTest [native] Refactor TaskManagerTest to not create common::WriterOptions instance Aug 22, 2024
@kewang1024
[native] Refactor TaskManagerTest to not create common::WriterOptions…
509597e 
… instance

After velox refactoring, we should not create dwio::common::WriterOptions instance anymore.

Update TaskManagerTest.cpp
xiaoxmeng
xiaoxmeng approved these changes Aug 22, 2024
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!

@kewang1024 kewang1024 merged commit 797019c into prestodb:master Aug 22, 2024
59 checks passed
@tdcmeehan tdcmeehan mentioned this pull request Aug 23, 2024
Merged
34 tasks
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

@majetideepak majetideepak Awaiting requested review from majetideepak

@yingsu00 yingsu00 Awaiting requested review from yingsu00

Assignees

@yingsu00 yingsu00

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kewang1024 @majetideepak @yingsu00 @xiaoxmeng