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

GH-37884: [Swift] Allow reading of unaligned FlatBuffers buffers #38635

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

abandy
Copy link
Contributor

@abandy abandy commented Nov 8, 2023

The PR enables the swift readers to read from unaligned buffers (fix for issue: 37884)

Enabling unaligned buffers incurs a performance penalty so the developer will need to consider this when enabling this feature.

It is not currently possible to recover from a buffer unaligned error as this error is a fatalError so trying aligned and then falling back to unaligned is not an option. Also, FlatBuffers has a verifier that should be able to catch this error but currently it seems to fail on both aligned and unaligned buffers (I tried verifying the example python server get return value but verification fails even though the buffers are able to be read successfully)

@abandy abandy requested a review from kou as a code owner November 8, 2023 14:08
Copy link

github-actions bot commented Nov 8, 2023

⚠️ GitHub issue #37884 has been automatically assigned in GitHub to PR creator.

@abandy abandy force-pushed the 37884 branch 2 times, most recently from ea6ee75 to 4114f5a Compare December 4, 2023 22:11
@abandy
Copy link
Contributor Author

abandy commented Dec 5, 2023

@kou Please review when you get a chance.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@@ -32,7 +32,7 @@ let package = Package(
targets: ["Arrow"]),
],
dependencies: [
.package(url: "https://github.com/google/flatbuffers.git", from: "23.3.3")
.package(url: "https://github.com/google/flatbuffers.git", branch: "master")
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment why we need to use master for now and when we can specify a tag again?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Dec 5, 2023
@abandy
Copy link
Contributor Author

abandy commented Dec 8, 2023

@kou Please merge when you get a chance. Thank you!

@kou kou changed the title GH-37884: [Swift] allow reading of unaligned FlatBuffers buffers GH-37884: [Swift][FlightRPC] allow reading of unaligned FlatBuffers buffers Dec 8, 2023
@kou kou changed the title GH-37884: [Swift][FlightRPC] allow reading of unaligned FlatBuffers buffers GH-37884: [Swift] Allow reading of unaligned FlatBuffers buffers Dec 8, 2023
@kou kou merged commit ad63158 into apache:main Dec 8, 2023
8 checks passed
@kou kou removed the awaiting merge Awaiting merge label Dec 8, 2023
@kou
Copy link
Member

kou commented Dec 8, 2023

Done!

@mgrazianoc
Copy link

Nice to hear about that!!!

Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit ad63158.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

mapleFU pushed a commit to mapleFU/arrow that referenced this pull request Dec 13, 2023
apache#38635)

The PR enables the swift readers to read from unaligned buffers (fix for issue: 37884)

Enabling unaligned buffers incurs a performance penalty so the developer will need to consider this when enabling this feature.  

It is not currently possible to recover from a buffer unaligned error as this error is a fatalError so trying aligned and then falling back to unaligned is not an option.  Also, FlatBuffers has a verifier that should be able to catch this error but currently it seems to fail on both aligned and unaligned buffers (I tried verifying the example python server get return value but verification fails even though the buffers are able to be read successfully)
* Closes: apache#37884

Authored-by: Alva Bandy <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
clayburn pushed a commit to clayburn/arrow that referenced this pull request Jan 23, 2024
apache#38635)

The PR enables the swift readers to read from unaligned buffers (fix for issue: 37884)

Enabling unaligned buffers incurs a performance penalty so the developer will need to consider this when enabling this feature.  

It is not currently possible to recover from a buffer unaligned error as this error is a fatalError so trying aligned and then falling back to unaligned is not an option.  Also, FlatBuffers has a verifier that should be able to catch this error but currently it seems to fail on both aligned and unaligned buffers (I tried verifying the example python server get return value but verification fails even though the buffers are able to be read successfully)
* Closes: apache#37884

Authored-by: Alva Bandy <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
apache#38635)

The PR enables the swift readers to read from unaligned buffers (fix for issue: 37884)

Enabling unaligned buffers incurs a performance penalty so the developer will need to consider this when enabling this feature.  

It is not currently possible to recover from a buffer unaligned error as this error is a fatalError so trying aligned and then falling back to unaligned is not an option.  Also, FlatBuffers has a verifier that should be able to catch this error but currently it seems to fail on both aligned and unaligned buffers (I tried verifying the example python server get return value but verification fails even though the buffers are able to be read successfully)
* Closes: apache#37884

Authored-by: Alva Bandy <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
@abandy abandy deleted the 37884 branch April 2, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Swift][FlightRPC] Processing valid utf-8 lead to misaligned error
3 participants