-
Notifications
You must be signed in to change notification settings - Fork 867
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
Conversation
Thank you @roeap -- this is amazing! |
There was a problem hiding this 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
arrow-flight/src/sql/sql_info.rs
Outdated
@@ -280,6 +348,22 @@ impl SqlInfoUnionBuilder { | |||
/// let batch = info_list.encode().unwrap(); | |||
/// ``` | |||
/// | |||
/// # Example |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 🤣.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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?
int32_to_int32_list_map
type in SqlInfo dense unionAre there any user-facing changes?
Users can now specify SqlInfos with the Map of i32 Lists type.