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-36120: [C#] Support schema metadata through the C API #36122

Merged
merged 2 commits into from
Jun 16, 2023
Merged

GH-36120: [C#] Support schema metadata through the C API #36122

merged 2 commits into from
Jun 16, 2023

Conversation

CurtHagenlocher
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher commented Jun 16, 2023 •
edited
Loading

What changes are included in this PR?

Import and export of field- and schema-level metadata via the C API.

Are these changes tested?

Yes

@github-actions
Copy link

⚠️ GitHub issue #36120 has been automatically assigned in GitHub to PR creator.

@github-actions
Copy link

⚠️ GitHub issue #36120 has been automatically assigned in GitHub to PR creator.

1 similar comment
@github-actions
Copy link

⚠️ GitHub issue #36120 has been automatically assigned in GitHub to PR creator.

@CurtHagenlocher
Copy link
Contributor Author

@wjones127 has offered to review ;)

@wjones127 wjones127 self-requested a review June 16, 2023 18:38
wjones127
wjones127 requested changes Jun 16, 2023
View reviewed changes
Copy link
Member

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Would mainly like to see a test for multi-byte characters, and then have a few smaller nits you can consider.

csharp/test/Apache.Arrow.Tests/CDataInterfacePythonTests.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test that involves some multi-byte unicode characters?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 16, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 16, 2023
wjones127
wjones127 approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 16, 2023
@wjones127 wjones127 merged commit 14f2e4e into apache:main Jun 16, 2023
@CurtHagenlocher CurtHagenlocher deleted the SchemaMetadata branch June 16, 2023 19:47
@conbench-apache-arrow
Copy link

Conbench analyzed the 6 benchmark runs on commit 14f2e4e3.

There were 2 benchmark results indicating a performance regression:

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjones127 wjones127 wjones127 approved these changes

@westonpace westonpace Awaiting requested review from westonpace

Assignees
No one assigned
Labels
awaiting merge Awaiting merge Component: C#
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C#] Support schema and field metadata in the C Data Interface
2 participants
@CurtHagenlocher @wjones127