GH-33936: [Go] C Data Interface: export dummy buffer for nil buffers #33951
Conversation
go/arrow/cdata/cdata_exports.go
Outdated
| if i > 0 || !internal.HasValidityBitmap(arr.DataType().ID(), flatbuf.MetadataVersionV5) { | ||
| // apache/arrow#33936: export a dummy buffer to be friendly to | ||
| // implementations that don't import NULL properly | ||
| buffers[i] = (*C.void)(unsafe.Pointer(&C.kGoCdataZeroRegion)) | ||
| } else { | ||
| buffers[i] = nil | ||
| } |
There was a problem hiding this comment.
should this be behind some sort of flag so that we can easily switch back to using nil when the other implementations catch up? Or is that not worth it?
There was a problem hiding this comment.
If/when we decide to remove it later, I think we should just remove it from the code entirely, instead of trying to flag it.
There was a problem hiding this comment.
I think for Arrow 13 we could circle back and remove it?
There was a problem hiding this comment.
The C++ side fix isn't even shipping until Arrow 12
|
Benchmark runs are scheduled for baseline = 3affada and contender = 5765aa2. 5765aa2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…ffers (apache#33951) ### Rationale for this change While apache#14805 clarified that NULL buffers are valid in the C Data Interface, not all implementations handle this correctly (or were fixed, but earlier versions still exist). ### What changes are included in this PR? Export a non-NULL dummy buffer for nil/empty buffers to ensure compatibility. ### Are these changes tested? A regression test is included. * Closes: apache#33936 Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…ffers (apache#33951) ### Rationale for this change While apache#14805 clarified that NULL buffers are valid in the C Data Interface, not all implementations handle this correctly (or were fixed, but earlier versions still exist). ### What changes are included in this PR? Export a non-NULL dummy buffer for nil/empty buffers to ensure compatibility. ### Are these changes tested? A regression test is included. * Closes: apache#33936 Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…ffers (apache#33951) ### Rationale for this change While apache#14805 clarified that NULL buffers are valid in the C Data Interface, not all implementations handle this correctly (or were fixed, but earlier versions still exist). ### What changes are included in this PR? Export a non-NULL dummy buffer for nil/empty buffers to ensure compatibility. ### Are these changes tested? A regression test is included. * Closes: apache#33936 Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
While #14805 clarified that NULL buffers are valid in the C Data Interface, not all implementations handle this correctly (or were fixed, but earlier versions still exist).
What changes are included in this PR?
Export a non-NULL dummy buffer for nil/empty buffers to ensure compatibility.
Are these changes tested?
A regression test is included.