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

fix: Ensure Flight schema includes parent metadata #3811

Merged
merged 3 commits into from
Mar 8, 2023
Merged

fix: Ensure Flight schema includes parent metadata #3811

merged 3 commits into from
Mar 8, 2023

Conversation

stuartcarnie
Copy link
Contributor

Which issue does this PR close?

Closes #3779

Rationale for this change

Ensures that schema-level metadata is included when encoding the schema for a Flight response.

What changes are included in this PR?

The prepare_schema_for_flight function in the encode module of the arrow-flight crate was amended to include the schema from the parent.

Are there any user-facing changes?

No

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Mar 7, 2023
@stuartcarnie stuartcarnie changed the title fix: Ensure prepared schema includes parent metadata fix: Ensure Flight schema includes parent metadata Mar 7, 2023
@alamb alamb mentioned this pull request Mar 7, 2023
Merged
alamb
alamb approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @stuartcarnie. Looks good to me.

I think we should add a higher level test in encode_decode.rs (so if /when we remove this "prepare schema for flight" to support dictionary encoded arrays, we will still have coverage for metadata)

Here is a small PR (targeting this pR) to add one:
stuartcarnie#1

If you prefer we can also merge this PR and I can make a separate PR to arrow-rs

arrow-flight/src/encode.rs
@@ -502,6 +503,16 @@ mod tests {
);
}

#[test]
fn test_schema_metadata_encoded() {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alamb
Copy link
Contributor

alamb commented Mar 7, 2023

@stuartcarnie this PR is failing the 'cargo fmt' CI check: https://github.com/apache/arrow-rs/actions/runs/4351021747/jobs/7602273280

@stuartcarnie
Copy link
Contributor Author

@alamb thank you for the review and your PR, which I merged into this one.

@tustvold tustvold merged commit 81ed334 into apache:master Mar 8, 2023
@ursabot
Copy link

ursabot commented Mar 8, 2023

Benchmark runs are scheduled for baseline = 5d75729 and contender = 81ed334. 81ed334 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
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

@stuartcarnie stuartcarnie deleted the sgc/issue/schema_metadata_3779 branch March 8, 2023 22:12
@tustvold tustvold mentioned this pull request Mar 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schema-level metadata is not encoded in Flight responses
4 participants
@stuartcarnie @alamb @ursabot @tustvold