feat: Add TextWriter #11789
feat: Add TextWriter #11789
Conversation
✅ Deploy Preview for meta-velox canceled.
|
|
@kewang1024 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
b0db7b7 to
b4cc637
Compare
|
@kewang1024 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
CMakeLists.txt
Outdated
| add_library(gflags_gflags INTERFACE | ||
| velox/dwio/text/writer/BufferedWriter.cpp | ||
| velox/dwio/text/writer/BufferedWriter.h | ||
| velox/dwio/text/tests/writer/TextWriterTest.cpp |
| add_test( | ||
| NAME velox_text_writer_test | ||
| COMMAND velox_text_writer_test | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}) |
There was a problem hiding this comment.
could be simplified as add_test(velox_text_writer_test velox_text_writer_test)
There was a problem hiding this comment.
|
|
||
| BufferedWriter::BufferedWriter( | ||
| std::unique_ptr<dwio::common::FileSink> sink, | ||
| std::shared_ptr<memory::MemoryPool> pool) |
There was a problem hiding this comment.
add && and const &, respectively, to avoid object copies.
There was a problem hiding this comment.
It was written this way to be more consistent with our codebase
velox/velox/dwio/parquet/writer/Writer.cpp
Lines 221 to 222 in c046524
velox/velox/dwio/parquet/writer/Writer.cpp
Lines 41 to 42 in c046524
|
|
||
| #pragma once | ||
|
|
||
| namespace facebook::velox::text { |
There was a problem hiding this comment.
nit: how about facebook::velox::dwio::text?
There was a problem hiding this comment.
But for orc/parquet, their namespace doesn't have dwio
velox/velox/dwio/orc/reader/OrcReader.h
Line 22 in c046524
There was a problem hiding this comment.
It might be better to have dwio namespace there. We probably need to fix that cc @majetideepak
|
|
||
| #pragma once | ||
|
|
||
| namespace facebook::velox::text { |
b4cc637 to
d219184
Compare
|
|
||
| #pragma once | ||
|
|
||
| namespace facebook::velox::text { |
There was a problem hiding this comment.
It might be better to have dwio namespace there. We probably need to fix that cc @majetideepak
| std::vector<std::vector<std::string>> result = readFile(filePath); | ||
| EXPECT_EQ(result.size(), 3); | ||
| EXPECT_EQ(result[0].size(), 2); | ||
| EXPECT_EQ(result[0][0], "1"); |
There was a problem hiding this comment.
Consider to have a fuzzer test by using (in fbcode/velox/vector/fuzzer/VectorFuzzer.h)
TypePtr randType(
FuzzerGenerator& rng,
const std::vector<TypePtr>& scalarTypes,
int maxDepth = 5);
to generate random type data with variable size vector and verify the result.
There was a problem hiding this comment.
I've also thought about that, but as of now we're not able to read the data back
we need to do it in a followup after we move text reader into velox
2df38a9 to
6862c7b
Compare
There was a problem hiding this comment.
@kewang1024 LGTM. Thanks and please merge after address comments.
eac8374 to
c17a2ce
Compare
c17a2ce to
ded0c59
Compare
|
@kewang1024 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@kewang1024 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kewang1024 merged this pull request in 0605101. |
| velox_dwio_arrow_parquet_writer_lib | ||
| velox_dwio_arrow_parquet_writer_util_lib |
There was a problem hiding this comment.
Curious, why does text writer need to depend on arrow parquet writer lib ?
This creates problems in presto_server build because parquet arrow are not built natively on the mac.
(Another miss on velox part is that parquet does not build on the mac, and thus velox_dwio_arrow_parquet_writer_lib is not built , along with obviously velox_dwio_text_writer) .
Summary: Add TextWriter support for primitive data types. - [ ] Add support for complex types - [ ] Add TextReader support - [ ] Add fuzzer test after we have TextReader Pull Request resolved: facebookincubator#11789 Reviewed By: xiaoxmeng Differential Revision: D66943043 Pulled By: kewang1024 fbshipit-source-id: 82cd558c3602b8f1b30adbceec313296a22613bc
Add TextWriter support for primitive data types.