[native] Refactor TaskManagerTest to not create common::WriterOptions instance#23480
Conversation
| #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" |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I think this will break Meta internal test as Velox side change on Writer options is landed.
There was a problem hiding this comment.
Do we need this header still?
majetideepak
left a comment
majetideepak
left a comment
There was a problem hiding this comment.
Please use [native] prefix for the commit.
yingsu00
left a comment
yingsu00
left a comment
There was a problem hiding this comment.
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.
kewang1024
changed the title
kewang1024
force-pushed
the
refactor-WriterOption
branch
from
f8ce350 to
f611af5
Compare
kewang1024
changed the title
yingsu00
left a comment
yingsu00
left a comment
There was a problem hiding this comment.
Approving it for now to unblock Meta workloads
| const std::string& filePath, | ||
| const std::vector<RowVectorPtr>& vectors) { | ||
| auto options = std::make_shared<dwio::common::WriterOptions>(); | ||
| std::shared_ptr<dwio::common::WriterOptions> options = |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Yeah, I think it works as in the production code. Thanks!
There was a problem hiding this comment.
Thanks @yingsu00 for your good suggestion!
kewang1024
force-pushed
the
refactor-WriterOption
branch
2 times, most recently
from
493840c to
1513ddc
Compare
kewang1024
changed the title
… instance After velox refactoring, we should not create dwio::common::WriterOptions instance anymore. Update TaskManagerTest.cpp
kewang1024
force-pushed
the
refactor-WriterOption
branch
from
1513ddc to
509597e
Compare
4 participants
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