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-41993 [Go] IPC writer shift voffsets when offsets array does not start from zero #43176

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

joellubi
Copy link
Member

@joellubi joellubi commented Jul 8, 2024

Rationale for this change

It should be valid to specify offset buffers that do not start from zero. This particularly important for when multiple arrays share a single value buffer.

What changes are included in this PR?

  • Add condition to shift offsets buffer when it does not start from zero
  • Test to reproduce failure and then validate fix

Are these changes tested?

Yes

Are there any user-facing changes?

Variable-length binary arrays that share a value buffer will not result in errors.

Copy link

github-actions bot commented Jul 8, 2024

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

@joellubi joellubi changed the title GH-41993 [Go][arrow/ipc/writer] WIP: voffsets are not updated when Binary-like arrays get truncated GH-41993 [Go] IPC writer shift voffsets when offsets array does not start from zero Jul 9, 2024
@joellubi joellubi marked this pull request as ready for review July 9, 2024 18:40
@joellubi joellubi requested a review from zeroshade as a code owner July 9, 2024 18:40
@joellubi joellubi marked this pull request as draft July 9, 2024 18:50
@joellubi
Copy link
Member Author

joellubi commented Jul 9, 2024

Sorry just noticed a bug, will fix and reopen shortly

@joellubi joellubi marked this pull request as ready for review July 9, 2024 19:29
@joellubi
Copy link
Member Author

joellubi commented Jul 9, 2024

Sorry just noticed a bug, will fix and reopen shortly

Fix in place, should be good now

@zeroshade
Copy link
Member

@notfilippo can you give this a try with your example and confirm that this fixes the issue?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 11, 2024
@notfilippo
Copy link

Thanks for the ping. I'll check my internal repro with this new version and let you know asap!

Copy link

@notfilippo notfilippo left a comment

Choose a reason for hiding this comment

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

Tested with an internal repro. It works! Thanks @joellubi :)

@joellubi joellubi merged commit 1fce293 into apache:main Jul 15, 2024
25 of 27 checks passed
@joellubi joellubi removed the awaiting merge Awaiting merge label Jul 15, 2024
Copy link

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

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.

3 participants