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.

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

feat(flight): support int32_to_int32_list_map in sql infos #4300

Merged
merged 5 commits into from
May 30, 2023

Conversation

roeap
Copy link
Contributor

@roeap roeap commented May 28, 2023

Draft as it depends on #4293 by @alamb, which contains tests useful to validate this implementation.

Which issue does this PR close?

Closes #.

Rationale for this change

Thus far the SqlInfo helpers in arrow-flight did not yet support complex types defined in the specs in the form of Map<i32, List<i32>>. This PR adds support for these types.

What changes are included in this PR?

  • support int32_to_int32_list_map type in SqlInfo dense union

Are there any user-facing changes?

Users can now specify SqlInfos with the Map of i32 Lists type.

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels May 28, 2023
@roeap roeap changed the title Sql infos list map feat(flight): support int32_to_int32_list_map in sql infos May 28, 2023
@alamb
Copy link
Contributor

alamb commented May 28, 2023

Thank you @roeap -- this is amazing!

@roeap roeap force-pushed the sql-infos-list-map branch from 6260088 to daf3634 Compare May 29, 2023 11:39
@roeap roeap marked this pull request as ready for review May 29, 2023 11:48
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This looks great -- thank you @roeap

I had one question about a doc example, otherwise I think this PR is ready to go. Thanks again

@@ -280,6 +348,22 @@ impl SqlInfoUnionBuilder {
/// let batch = info_list.encode().unwrap();
/// ```
///
/// # Example
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be identical to the example above. Is that intended? -- I wonder if it got added accidentally during merge 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did a rebase, and this is likely an artefact of that! Sorry for missing that, will fix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

albeit repetition sometimes being helpful to learn things, went back to a single instance of the example 🤣.

Copy link
Contributor

Choose a reason for hiding this comment

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

😆 👏

@@ -386,6 +474,15 @@ mod tests {

#[test]
fn test_sql_infos() {
let mut convert: HashMap<i32, Vec<i32>> = HashMap::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alamb alamb merged commit d5ba15a into apache:master May 30, 2023
@roeap roeap deleted the sql-infos-list-map branch May 30, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants