PARQUET-1482: [C++] Add branch to TypedRecordReader::ReadNewPage for … #3312

rdmello · 2019-01-05T00:54:09Z

…PageType::DATA_PAGE_V2 to address incompatibility with parquetjs.

Tests

This commit doesn't include tests right now; I am working on adding tests and was hoping for some initial feedback on the code changes. I may need to use an actual file generated by parquetjs to test this issue, so I wonder if adding feeds1kMicros.parquet from the JIRA task to the parquet-testing repository is an option for this.

Description

parquetjs seems to be writing Parquet V2 files with DataPageV2 pages, while parquet-cpp writes Parquet V2 files with DataPage pages.

Since TypedRecordReader::ReadNewPage() only had a branch for PageType::DATA_PAGE, the reader would return without reading any data for records that have DATA_PAGE_V2 pages. This explains the behavior observed in PARQUET-1482.

This commit adds a new if-else branch for the DataPageV2 case in TypedRecordReader::ReadNewPage(). Since the DataPageV2 branch needed to reuse the code from the DataPage case, I refactored the repetition/definition level decoder initialization and the data decoder initialization to two new methods in the TypedRecordReader class. These new methods are now called by the DataPage and DataPageV2 initialization branches in TypedRecordReader::ReadNewPage().

There is an alternate implementation possible (with a smaller diff) by sharing the same else-if branch between DataPage and DataPageV2 using a pointer-to-derived shared_ptr<Page>. However, since the Page superclass doesn't have the necessary encoding() or num_values() methods, I would need to add a common superclass to both DataPage and DataPageV2 that defined these methods. I didn't do this because I was hesitant to modify the Page class hierarchy for this commit.

wesm · 2019-01-05T01:05:25Z

The parquetjs project should probably be warned about cross-compatibility -- V2 data pages are likely unreadable in a number of places.

rdmello · 2019-01-07T22:42:40Z

Hi Wes, thanks for responding on this! I just opened an issue with the parquetjs project to make them aware of this too: ironSource/parquetjs#78 .

I've noticed the same issue (from PARQUET-1482) occurring with files written using the ParquetFileWriter class in parquet-mr too (through the MATLAB interface for Apache Parquet).

I can't reproduce this using the newest release of parquet-mr (1.11.0), but I think a previous version of it (1.9.0) would also default to writing Parquet V2 files with DataPageV2 pages. Therefore this may be more widespread than only files written using the parquetjs library.

I see that there are Travis CI build failures, but these seem to be unrelated to the changes in this commit. I was thinking of pulling in changes from apache:master after the build is passing, rebasing the parquet_1482 branch, and then re-running the CI, but let me know if there's a better way to do this.

xhochy

I'm in general happy with the changes but we definitely should have a test for this code. You could add probably in the most simple form to https://github.com/apache/arrow/blob/master/cpp/src/parquet/file-deserialize-test.cc

wesm · 2019-01-29T03:04:25Z

I agree that a unit test would be a good idea. When we were building this library initially I tried to make the file deserialization internals a bit more accessible to unit testing, otherwise triggering and testing various code paths would be a lot more difficult

rdmello · 2019-01-29T22:03:44Z

Hi Uwe and Wes, thank you for the recommendations. I added a test in file-deserialize-test.cc that exercises the DataPageV2 serialization and deserialization code path in the latest version of the pull request. Please let me know if you have any feedback on this.

My understanding is that the new test will not directly test the changes to record_reader.cc that I've made. To do a round-trip test of DataPageV2 workflows, the Arrow interface to Parquet will probably need a way to write Parquet files with DataPageV2 page headers. I couldn't find a way to do this directly using the Arrow-based interface for Parquet, but let me know if I'm missing something here.

I was thinking of creating a new JIRA story for writing DataPageV2 headers using the Arrow interface for Parquet, but I found PARQUET-458 that seems to be similar. Is it alright if I assign this JIRA story to myself?

xhochy · 2019-01-30T14:37:41Z

@rdmello Feel free to assign PARQUET-458 to yourself.

To enable writing V2 pages in the Arrow, you should add an option https://github.com/apache/arrow/blob/master/cpp/src/parquet/properties.h#L142-L433 to switch between V1 and V2 pages. This option needs then be passed through the layers where the page is actually created.

cpp/src/parquet/arrow/record_reader.cc

hatemhelal · 2019-02-21T13:47:09Z

I've gone through these changes and they look good to me.

Would it be ok to accept this PR and follow up with a new PR to respond to the feedback? @rdmello could use PARQUET-458 to complete the write implementation and add the suggested tests.

@xhochy and @wesm, thoughts?

wesm · 2019-02-28T01:37:05Z

Sorry for the delay, will review this so it gets into 0.13.0

…PageType::DATA_PAGE_V2 to address incompatibility with parquetjs. Tests This commit doesn't include tests; I am working on them now. I may need to use an actual file generated by parquetjs to test this issue, so I wonder if adding feeds1kMicros.parquet from the JIRA task to the parquet-testing repository is an option. Description parquetjs seems to be writing Parquet V2 files with DataPageV2 pages, while parquet-cpp writes Parquet V2 files with DataPage pages. Since TypedRecordReader::ReadNewPage() only had a branch for PageType::DATA_PAGE, the reader would return without reading any data for records that have DATA_PAGE_V2 pages. This explains the behavior observed in PARQUET-1482. This commit adds a new if-else branch for the DataPageV2 case in TypedRecordReader::ReadNewPage(). Since the DataPageV2 branch needed to reuse the code from the DataPage case, I refactored the repetition/definition level decoder initialization and the data decoder initialization to two new methods in the TypedRecordReader class. These new methods are now called by the DataPage and DataPageV2 initialization branches in TypedRecordReader::ReadNewPage(). There is an alternate implementation possible (with a smaller diff) by sharing the same else-if branch between DataPage and DataPageV2 using a pointer-to-derived shared_ptr<Page>. However, since the Page superclass doesn't have the necessary encoding() or num_values() methods, I would need to add a common superclass to both DataPage and DataPageV2 that defined these methods. I didn't do this because I was hesitant to modify the Page class hierarchy for this commit.

…on and deserialization.

wesm · 2019-03-06T16:49:32Z

Rebased. Have you reported an issue into upstream parquetjs?

wesm

This looks fine. The fact that DataPage and DataPageV2 do not have a common base seems like an eyesore to me. I'm going to quickly fix that

wesm · 2019-03-06T16:50:02Z

cpp/src/parquet/arrow/record_reader.cc

+  template <typename PageType>
+  int64_t InitializeLevelDecoders(const std::shared_ptr<PageType> page,
+                                  const Encoding::type repetition_level_encoding,
+                                  const Encoding::type definition_level_encoding);


const is not needed with Encoding::type

wesm · 2019-03-06T16:50:11Z

cpp/src/parquet/arrow/record_reader.cc

+
+  template <typename PageType>
+  void InitializeDataDecoder(const std::shared_ptr<PageType> page,
+                             const int64_t levels_bytes);


const not needed with int64

wesm · 2019-03-06T16:52:07Z

cpp/src/parquet/arrow/record_reader.cc

+  // Have not decoded any values from the data page yet
+  num_decoded_values_ = 0;
+
+  const uint8_t* buffer = page->data();


Only data() and num_values() are used here. We should have a common base class for DataPageV1 and DataPageV2 instead of having this template

wesm · 2019-03-06T16:52:30Z

cpp/src/parquet/arrow/record_reader.cc

+void TypedRecordReader<DType>::InitializeDataDecoder(const std::shared_ptr<PageType> page,
+                                                     const int64_t levels_byte_size) {
+  const uint8_t* buffer = page->data() + levels_byte_size;
+  const int64_t data_size = page->size() - levels_byte_size;


rdmello · 2019-03-06T17:29:50Z

Hi Wes, thanks for rebasing the code and providing feedback. Yes, I did report this to parquetjs earlier: ironSource/parquetjs#78

I've looked through some of the parquetjs code base, and my understanding is that there is a way to provide an option to the ParquetWriter class in parquetjs that would write DataPageV1 pages. However the default behavior for parquetjs is still to write DataPageV2 pages.

wesm · 2019-03-06T19:12:55Z

+1. Will merge once the build looks good

wesm · 2019-03-06T21:46:08Z

Appveyor is about half passed. Merging

…PageType::DATA_PAGE_V2 to address incompatibility with parquetjs. **Tests** This commit doesn't include tests right now; I am working on adding tests and was hoping for some initial feedback on the code changes. I may need to use an actual file generated by `parquetjs` to test this issue, so I wonder if adding `feeds1kMicros.parquet` from the JIRA task to the parquet-testing repository is an option for this. **Description** `parquetjs` seems to be writing Parquet V2 files with [`DataPageV2`](https://github.com/apache/parquet-format/blob/e93dd628d90aa076745558998f0bf5d9c262bf22/src/main/thrift/parquet.thrift#L529) pages, while `parquet-cpp` writes Parquet V2 files with [`DataPage`](https://github.com/apache/parquet-format/blob/e93dd628d90aa076745558998f0bf5d9c262bf22/src/main/thrift/parquet.thrift#L492) pages. Since `TypedRecordReader::ReadNewPage()` only had a branch for `PageType::DATA_PAGE`, the reader would return without reading any data for records that have `DATA_PAGE_V2` pages. This explains the behavior observed in [PARQUET-1482](https://issues.apache.org/jira/projects/PARQUET/issues/PARQUET-1482?filter=allopenissues). This commit adds a new if-else branch for the `DataPageV2` case in `TypedRecordReader::ReadNewPage()`. Since the `DataPageV2` branch needed to reuse the code from the `DataPage` case, I refactored the repetition/definition level decoder initialization and the data decoder initialization to two new methods in the `TypedRecordReader` class. These new methods are now called by the `DataPage` and `DataPageV2` initialization branches in `TypedRecordReader::ReadNewPage()`. There is an alternate implementation possible (with a smaller diff) by sharing the same else-if branch between `DataPage` and `DataPageV2` using a pointer-to-derived `shared_ptr<Page>`. However, since the Page superclass doesn't have the necessary `encoding()` or `num_values()` methods, I would need to add a common superclass to both `DataPage` and `DataPageV2` that defined these methods. I didn't do this because I was hesitant to modify the `Page` class hierarchy for this commit. Author: Wes McKinney <[email protected]> Author: rdmello <[email protected]> Author: Rylan Dmello <[email protected]> Closes #3312 from rdmello/parquet_1482 and squashes the following commits: c5cb0f3 <Wes McKinney> Add DataPage base class for DataPageV1 and DataPageV2 8df8328 <rdmello> PARQUET-1482: Adding basic unit test for DataPageV2 serialization and deserialization. 9df3222 <Rylan Dmello> PARQUET-1482: Add branch to TypedRecordReader::ReadNewPage for PageType::DATA_PAGE_V2 to address incompatibility with parquetjs.

rdmello mentioned this pull request Jan 7, 2019

Parquet writer should default to writing pages with DataPageV1 for better compatibility with other Parquet readers ironSource/parquetjs#78

Open

rdmello force-pushed the parquet_1482 branch from d158118 to ec0ee52 Compare January 8, 2019 15:13

xhochy reviewed Jan 14, 2019

View reviewed changes

rdmello force-pushed the parquet_1482 branch from ec0ee52 to 1cfb11f Compare January 29, 2019 21:45

wesm force-pushed the master branch from 3088183 to 0c6b2d2 Compare February 18, 2019 19:34

hatemhelal reviewed Feb 21, 2019

View reviewed changes

cpp/src/parquet/arrow/record_reader.cc Outdated Show resolved Hide resolved

rdmello and others added 2 commits March 6, 2019 10:48

PARQUET-1482: [C++] Adding basic unit test for DataPageV2 serializati…

8df8328

…on and deserialization.

wesm force-pushed the parquet_1482 branch from 1cfb11f to 8df8328 Compare March 6, 2019 16:49

wesm reviewed Mar 6, 2019

View reviewed changes

Add DataPage base class for DataPageV1 and DataPageV2

c5cb0f3

wesm closed this in c16eccb Mar 6, 2019

kevingurney deleted the parquet_1482 branch February 7, 2022 19:31

asfimport mentioned this pull request Jun 23, 2024

[C++][Parquet] Unable to read data from parquet file generated with parquetjs #42868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-1482: [C++] Add branch to TypedRecordReader::ReadNewPage for … #3312

PARQUET-1482: [C++] Add branch to TypedRecordReader::ReadNewPage for … #3312

rdmello commented Jan 5, 2019 •

edited

Loading

wesm commented Jan 5, 2019

rdmello commented Jan 7, 2019 •

edited

Loading

xhochy left a comment

wesm commented Jan 29, 2019

rdmello commented Jan 29, 2019

xhochy commented Jan 30, 2019

hatemhelal commented Feb 21, 2019

wesm commented Feb 28, 2019

wesm commented Mar 6, 2019

wesm left a comment

wesm Mar 6, 2019

wesm Mar 6, 2019

wesm Mar 6, 2019

wesm Mar 6, 2019

rdmello commented Mar 6, 2019

wesm commented Mar 6, 2019

wesm commented Mar 6, 2019

PARQUET-1482: [C++] Add branch to TypedRecordReader::ReadNewPage for … #3312

PARQUET-1482: [C++] Add branch to TypedRecordReader::ReadNewPage for … #3312

Conversation

rdmello commented Jan 5, 2019 • edited Loading

wesm commented Jan 5, 2019

rdmello commented Jan 7, 2019 • edited Loading

xhochy left a comment

Choose a reason for hiding this comment

wesm commented Jan 29, 2019

rdmello commented Jan 29, 2019

xhochy commented Jan 30, 2019

hatemhelal commented Feb 21, 2019

wesm commented Feb 28, 2019

wesm commented Mar 6, 2019

wesm left a comment

Choose a reason for hiding this comment

wesm Mar 6, 2019

Choose a reason for hiding this comment

wesm Mar 6, 2019

Choose a reason for hiding this comment

wesm Mar 6, 2019

Choose a reason for hiding this comment

wesm Mar 6, 2019

Choose a reason for hiding this comment

rdmello commented Mar 6, 2019

wesm commented Mar 6, 2019

wesm commented Mar 6, 2019

rdmello commented Jan 5, 2019 •

edited

Loading

rdmello commented Jan 7, 2019 •

edited

Loading