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(flightsql): remove Any encoding of DoPutPreparedStatementResult #5817

Merged
merged 1 commit into from
May 31, 2024
Merged

fix(flightsql): remove Any encoding of DoPutPreparedStatementResult #5817

merged 1 commit into from
May 31, 2024

Conversation

erratic-pattern
Copy link
Contributor

Which issue does this PR close?

Closes #5731.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels May 29, 2024
@erratic-pattern erratic-pattern force-pushed the flghtsql-prepared-statement-params-no-any branch 2 times, most recently from 1a6067e to 8f54a4a Compare May 29, 2024 19:20
@erratic-pattern erratic-pattern force-pushed the flghtsql-prepared-statement-params-no-any branch from 8f54a4a to 319fde6 Compare May 29, 2024 19:22
This was referenced May 29, 2024
Closed
Open
@tustvold tustvold added the api-change Changes to the arrow API label May 30, 2024
alamb
alamb approved these changes May 30, 2024
View reviewed changes
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.

Looks good to me too -- @matthewmturner is there any chance you can give this a quick look as well?

@alamb alamb mentioned this pull request May 31, 2024
Closed
3 tasks
@matthewmturner
Copy link
Contributor

LGTM for the prepared statement result (i think the title just needs to be updated). Im also not sure if there are other payloads that rust wraps in any which other languages dont - I havent had the time to compare those.

@alamb alamb changed the title fix(flightsql): remove Any encoding of DoPutPreparedStatementUpdateResult fix(flightsql): remove Any encoding of DoPutPreparedStatementResult May 31, 2024
@alamb
Copy link
Contributor

alamb commented May 31, 2024

LGTM for the prepared statement result (i think the title just needs to be updated). Im also not sure if there are other payloads that rust wraps in any which other languages dont - I havent had the time to compare those.

Thanks @matthewmturner -- I updated the title of the PR

Given this PR is clearly more compatible than the current code, I think it is good to go.

If/when we evaluate the other methods and find additional issues, let's file additional tickets

@alamb alamb merged commit 95ef912 into apache:master May 31, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewmturner matthewmturner matthewmturner approved these changes

@tustvold tustvold tustvold approved these changes

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Milestone
No milestone
4 participants
@erratic-pattern @matthewmturner @alamb @tustvold