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

Temporarily fix compatibility for SchemaResult #26

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

skejserjensen
Copy link
Contributor

This PR adds a temporary workaround for issue #2445 in arrow-rs by extending FlightService.get_schema() to always prepend both the 32-bit continuation indicator and the 32-bit little-endian length to SchemaResult.schema so it can be deserialized by the other Apache Arrow Flight implementations. To preserve compatibility with the Rust client (mdb), execute_command() have been extended to strip the first eight bytes of SchemaResult.schema. I have manually tested the changes with Go (using the code in Telegraf-Output-Apache-Arrow-Flight), Java, Python, and the Rust client (mdb) using multiple different schemas. Finally, while slightly outside the scope of the PR, I have moved DoExchangeStream before DoActionStream so the order of the types should match the order of the methods that use them in FlightService.

@skejserjensen skejserjensen self-assigned this Aug 22, 2022
@skejserjensen skejserjensen requested review from agneborn98 and removed request for agneborn98 August 23, 2022 06:58
@skejserjensen
Copy link
Contributor Author

Based on the discussion in the two issues I have updated the PR to serialize and deserialize the Schema to IpcMessage but use SchemaResult as the transport format as required by the return type of get_schema(). The associated functions syntax is used for try_from() in the client to disambiguate between different methods.

Copy link
Contributor

@agneborn98 agneborn98 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 a lot better and is definitely less "hacky". I have tested it on my computer as well and ran into no issues, so this solution works fine.

@skejserjensen skejserjensen merged commit d3d4729 into master Aug 24, 2022
@skejserjensen skejserjensen deleted the bug/serialize-schema branch August 24, 2022 06:38
@skejserjensen skejserjensen mentioned this pull request Sep 18, 2022
@skejserjensen skejserjensen mentioned this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants