Fix reading empty Parquet DataPage #10121
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
@yingsu00 can you take a look at this fix? Thanks! |
majetideepak
force-pushed
the
fix-snappy
branch
3 times, most recently
from
ea56126 to
fb84bca
Compare
|
@yingsu00 can you take a look at this PR? |
|
@yingsu00, @nmahadevuni Can you review this fix as well? |
| TEST_F(ParquetReaderTest, testEmptyDataPage) { | ||
| const std::string sample(getExampleFilePath("snappy.parquet")); | ||
|
|
||
| facebook::velox::dwio::common::ReaderOptions readerOptions{leafPool_.get()}; |
There was a problem hiding this comment.
nit: we should put the entire test suite under a facebook::velox::dwio::test namespace so you don't need to specify the full path to everything.
There was a problem hiding this comment.
I fixed this to dwio::common::ReaderOptions. There are a couple of using namespace ... on top. This is now consistent with the velox/dwio/dwrf/test/ReaderTest.cpp test. Both are not under any namespace.
There was a problem hiding this comment.
I cleaned up the remaining as well.
There was a problem hiding this comment.
@majetideepak Thanks for fixing the bug! It looks good, just one nit.
| @@ -1215,3 +1208,24 @@ TEST_F(ParquetReaderTest, testLzoDataPage) { | |||
| .str(), | |||
| "31232"); | |||
| } | |||
|
|
|||
| TEST_F(ParquetReaderTest, testEmptyDataPage) { | |||
| const std::string sample(getExampleFilePath("snappy.parquet")); | |||
There was a problem hiding this comment.
Maybe rename this file to empty_v2datapage.parquet?
majetideepak
added
the
ready-to-merge
|
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@bikramSingh91 merged this pull request in 06c8a0c. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Resolves the compression issue 7 here #9560