Skip to content

GH-35304: [C++][ORC] Support attributes conversion#35499

Merged
pitrou merged 3 commits intoapache:mainfrom
wgtmac:orc_attributes
May 11, 2023
Merged

GH-35304: [C++][ORC] Support attributes conversion#35499
pitrou merged 3 commits intoapache:mainfrom
wgtmac:orc_attributes

Conversation

@wgtmac
Copy link
Member

@wgtmac wgtmac commented May 9, 2023 •
edited by github-actions bot
Loading

Rationale for this change

Apache Orc has a per column attribute map and Apache Iceberg depends on this to encode its field metadata. However, the C++ Orc adapter does not know it which makes it difficult to support pyarrow and pyiceberg.

What changes are included in this PR?

Both reader and writer support Orc attributes conversion from/to arrow field metadata.

Are these changes tested?

Added two test cases to make sure the Orc adapter can preserve the attributes well.

Are there any user-facing changes?

No.

@github-actions
Copy link

github-actions bot commented May 9, 2023

@wgtmac
Copy link
Member Author

wgtmac commented May 9, 2023

@Fokko Could you please take a look? Thanks!

Fokko
Fokko approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

As far as my C++ skills go, this looks good. I think this PR is a good start, the next step would be to expose this in Python as well.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 9, 2023
@wgtmac
Copy link
Member Author

wgtmac commented May 9, 2023

Thanks @Fokko!

Actually I didn't add any new API. Therefore pyiceberg should be able to get the missing info from arrow schema field metadata via pyarrow. I am not familiar with pyarrow, so please correct me if I am wrong. @jorisvandenbossche

pitrou
pitrou requested changes May 9, 2023
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks @wgtmac ! Here are some comments and suggestions.

@wgtmac
Copy link
Member Author

wgtmac commented May 10, 2023

Thanks @pitrou! Welcome back!

@wgtmac wgtmac requested a review from pitrou May 10, 2023 14:55
pitrou
pitrou reviewed May 10, 2023
View reviewed changes
pitrou
pitrou approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented May 11, 2023

Actually I didn't add any new API. Therefore pyiceberg should be able to get the missing info from arrow schema field metadata via pyarrow. I am not familiar with pyarrow, so please correct me if I am wrong

Yes, that should be the case

@pitrou pitrou merged commit 4fd5c28 into apache:main May 11, 2023
@ursabot
Copy link

ursabot commented May 13, 2023

Benchmark runs are scheduled for baseline = cfcff49 and contender = 4fd5c28. 4fd5c28 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
[Finished ⬇️2.09% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.25% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.81% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 4fd5c28e ec2-t3-xlarge-us-east-2
[Finished] 4fd5c28e test-mac-arm
[Finished] 4fd5c28e ursa-i9-9960x
[Finished] 4fd5c28e ursa-thinkcentre-m75q
[Finished] cfcff49d ec2-t3-xlarge-us-east-2
[Finished] cfcff49d test-mac-arm
[Finished] cfcff49d ursa-i9-9960x
[Finished] cfcff49d 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

@Fokko
Copy link
Contributor

Fokko commented May 14, 2023

Great work @wgtmac and thanks @jorisvandenbossche for the clarification on the Python side, I'll give it a try!

@Fokko Fokko mentioned this pull request May 14, 2023
Closed
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@wgtmac @ArgusLi
apacheGH-35304: [C++][ORC] Support attributes conversion (apache#35499)
bf3f8c4 
### Rationale for this change

Apache Orc has a per column attribute map and Apache Iceberg depends on this to encode its field metadata. However, the C++ Orc adapter does not know it which makes it difficult to support pyarrow and pyiceberg.

### What changes are included in this PR?

Both reader and writer support Orc attributes conversion from/to arrow field metadata.

### Are these changes tested?

Added two test cases to make sure the Orc adapter can preserve the attributes well.

### Are there any user-facing changes?

No.
* Closes: apache#35304

Authored-by: Gang Wu <ustcwg@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
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

+1 more reviewer

@Fokko Fokko Fokko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Python] Read ORC metadata

5 participants

@wgtmac @jorisvandenbossche @ursabot @Fokko @pitrou