-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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-34385: [Go] Read IPC files with compression enabled but uncompressed buffers #34476
GH-34385: [Go] Read IPC files with compression enabled but uncompressed buffers #34476
Conversation
|
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.
Should we unskip the integration tests here?
go/arrow/ipc/ipc.go
Outdated
// Note that enabling this option may result in unreadable data for Arrow | ||
// C++ versions prior to 12.0.0. |
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.
Doesn't this also apply to go? (Or else, why did we have problems in the integration test if this only affected C++ readers?)
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.
gah, my fault for copying the comment, i'll fix it and update.
Hmm, looks like CGO is having issues across multiple PRs |
@lidavidm Yea, looks like the issue is that macOS is missing the openssl lib for some reason, I 'll open a PR for it soon |
Addressed the failing macos issue with #34488 |
Benchmark runs are scheduled for baseline = f7fbfca and contender = 864ba82. 864ba82 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Rationale for this change
Fixing a bug in compressing buffers by prepending -1 when a buffer is not compressed due to size.
Are these changes tested?
Unit tests added, and other tests will be enabled via integration tests in #15194