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

GH-29781: [C++][Parquet] Switch to use compliant nested types by default #35146

Merged
merged 4 commits into from
May 12, 2023
Merged

GH-29781: [C++][Parquet] Switch to use compliant nested types by default #35146

merged 4 commits into from
May 12, 2023

Conversation

wjones127
Copy link
Member

@wjones127 wjones127 commented Apr 14, 2023 •
edited by github-actions bot
Loading

Rationale for this change

This has been a long-standing TODO.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

This PR includes breaking changes to public APIs.

@github-actions
Copy link

@wjones127 wjones127 mentioned this pull request Apr 14, 2023
Closed
mapleFU
mapleFU reviewed Apr 16, 2023
View reviewed changes
cpp/src/parquet/properties.h
@@ -819,8 +819,7 @@ class PARQUET_EXPORT ArrowWriterProperties {
coerce_timestamps_unit_(::arrow::TimeUnit::SECOND),
truncated_timestamps_allowed_(false),
store_schema_(false),
// TODO: At some point we should flip this.
Copy link
Member

Choose a reason for hiding this comment

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

/// This is disabled by default, but will be enabled by default in future.

Should we change this in Loc 880?

Copy link
Member

Choose a reason for hiding this comment

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

The line 320 in the parquet.rst doc should also be changed.

   std::shared_ptr<ArrowWriterProperties> arrow_props = ArrowWriterProperties::Builder()
      .enable_deprecated_int96_timestamps() // default False
      ->store_schema() // default False
      ->enable_compliant_nested_types() // default False
      ->build();

@wjones127 wjones127 force-pushed the GH-29781-parquet-compliant-nested-types branch from 4d0b91d to 72b4c67 Compare April 17, 2023 22:06
@wjones127 wjones127 force-pushed the GH-29781-parquet-compliant-nested-types branch from 72b4c67 to 564000c Compare April 17, 2023 22:07
@wjones127 wjones127 marked this pull request as ready for review April 18, 2023 20:37
@wjones127 wjones127 requested a review from AlenkaF as a code owner April 18, 2023 20:37
mapleFU
mapleFU approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

public abstract class ConversionPatterns {

  static final String MAP_REPEATED_NAME = "key_value";
  private static final String ELEMENT_NAME = "element";

Go through the Java code, seems that it also uses "element". LGTM

@mapleFU
Copy link
Member

mapleFU commented Apr 19, 2023 •
edited
Loading

In C++, code shows force. But when I go through code in arrow-rs, it doesn't say is force. And though parquet-format use "element" in description, it doesn't says it's forced. Maybe we should ask the parquet maillist later?

@wjones127
Copy link
Member Author

In C++, code shows force. But when I go through code in arrow-rs, it doesn't say is force. And though parquet-format use "element" in description, it doesn't says it's forced. Maybe we should ask the parquet maillist later?

The rules were laid out earlier here:

https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#backward-compatibility-rules

Before this PR I did work to make sure that the Arrow C++ implementation (1) didn't care about the field names in equality comparison (PR) and (2) could cheaply cast between types that differed only in field names (PR).

@mapleFU
Copy link
Member

mapleFU commented Apr 20, 2023

Thanks! So, this flag could make converting and reading a bit cheaper?

@wgtmac
Copy link
Member

wgtmac commented May 4, 2023

Is it time to revive this PR and check it in? @wjones127

@wjones127 wjones127 force-pushed the GH-29781-parquet-compliant-nested-types branch from 8f28f2a to 0ea421d Compare May 10, 2023 21:11
wgtmac
wgtmac approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

LGTM! cc @pitrou

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 11, 2023
@pitrou pitrou merged commit e324f9a into apache:main May 12, 2023
@wjones127 wjones127 deleted the GH-29781-parquet-compliant-nested-types branch May 12, 2023 16:10
@ursabot
Copy link

ursabot commented May 14, 2023

Benchmark runs are scheduled for baseline = 1624d5a and contender = e324f9a. e324f9a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.66% ⬆️0.06%] test-mac-arm
[Finished ⬇️1.52% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] e324f9ab ec2-t3-xlarge-us-east-2
[Failed] e324f9ab test-mac-arm
[Finished] e324f9ab ursa-i9-9960x
[Finished] e324f9ab ursa-thinkcentre-m75q
[Finished] 1624d5aa ec2-t3-xlarge-us-east-2
[Finished] 1624d5aa test-mac-arm
[Finished] 1624d5aa ursa-i9-9960x
[Finished] 1624d5aa ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented May 14, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@wjones127 @pitrou @ArgusLi
apacheGH-29781: [C++][Parquet] Switch to use compliant nested types b…
9938421 
…y default (apache#35146)

### Rationale for this change

This has been a long-standing TODO.

### What changes are included in this PR?

### Are these changes tested?

### Are there any user-facing changes?

**This PR includes breaking changes to public APIs.**
* Closes: apache#29781

Lead-authored-by: Will Jones <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
@wjones127 @pitrou @rtpsw
apacheGH-29781: [C++][Parquet] Switch to use compliant nested types b…
3ae3a39 
…y default (apache#35146)

### Rationale for this change

This has been a long-standing TODO.

### What changes are included in this PR?

### Are these changes tested?

### Are there any user-facing changes?

**This PR includes breaking changes to public APIs.**
* Closes: apache#29781

Lead-authored-by: Will Jones <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@mapleFU mapleFU mentioned this pull request Oct 26, 2023
Merged
@kevinzwang kevinzwang mentioned this pull request Jun 21, 2024
Merged
kevinzwang added a commit to Eventual-Inc/Daft that referenced this pull request Jun 22, 2024
@kevinzwang
[BUG] Raise error when Ray Data tensor cannot be pickled and disable …
b7e6e41 
…compliant nested types (#2428)

This solves some of our flaky unit tests that try to read a Ray dataset
tensor type under a Pyarrow version that does not properly serialize it.
Adds a new exception when we detect this issue, and also updates our
pyarrow dependencies to 13.0.0.

I also found a separate bug upon upgrading to pyarrow, where versions >=
13.0.0 will use compliant Parquet nested type names by default (see
apache/arrow#35146), which would then cause a
discrepancy between the Parquet and Arrow schemas for extension types.
This would lead to incorrect conversion from Parquet to Arrow when we
would read a file. This PR disables that explicitly.

Finally I also added a `coerce_temporal_nanoseconds` parameter to
`to_pandas` to revert it to its pre pyarrow>=13.0.0 behavior, which
makes the sql integration tests pass again

Confirmed to raise proper error in local testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou approved these changes

@wgtmac wgtmac wgtmac approved these changes

@mapleFU mapleFU mapleFU approved these changes

@AlenkaF AlenkaF Awaiting requested review from AlenkaF

Assignees
No one assigned
Labels
awaiting changes Awaiting changes Component: C++ Component: Documentation Component: Parquet Component: Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++][Parquet] Default to compliant nested types in Parquet writer
5 participants
@wjones127 @mapleFU @wgtmac @ursabot @pitrou