You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "andishgar (via GitHub)" <gi...@apache.org> on 2026/06/25 02:06:57 UTC
[I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
andishgar opened a new issue, #50249:
URL: https://github.com/apache/arrow/issues/50249
### Describe the enhancement requested
`arrow::BaseBinaryBuilder::GetView` returns an empty string for data that is added via `arrow::BaseBinaryBuilder::AppendNull`. This behavior has already been identified as problematic in [issue 39635](https://github.com/apache/arrow/issues/39635), where null data is incorrectly treated as an empty string, as seen [here](https://github.com/apache/arrow/blob/12b3eda46a69fbc2e7f6f9c52be345b678f4baae/cpp/src/arrow/array/dict_internal.h#L169-L170).
### Component(s)
C++
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4841123711
@pitrou, `Binary` works correctly because it uses [this logic](https://github.com/apache/arrow/blob/12b3eda46a69fbc2e7f6f9c52be345b678f4baae/cpp/src/arrow/util/hashing.h#L964) to access the validity bitmap directly. Note that both `BinaryView` and `BaseBinary` types are ultimately built from `BaseBinaryBuilder`; however, the way the final arrays are constructed differs between them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4840924346
Well, https://github.com/apache/arrow/issues/39635 is only for BinaryView, Binary works fine. So there must be another explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4850755748
@pitrou, several notes:
1. Regarding this:
> If we want to add one, we should do so for all builder classes, not just BinaryView.
I'm okay with that, and I'm aware that `SparseUnion`, `DenseUnion`, and `RunEndEncoded` builders have the bitmap buffer in different positions, so I should use a different strategy to check whether the data is null. I'll send a follow-up PR to address this by adding `IsNull` support for all builders.
2. I found that the following code doesn't appear to be used anywhere, and its logic also seems incorrect. Shouldn't we remove it?
https://github.com/apache/arrow/blob/bd8c6cc86749af8e70905ca3e20f5b81bfd8ab1a/cpp/src/arrow/array/builder_base.cc#L70-L86
3. Regarding this:
> It could make sense to have some kind of BinaryViewDataBuilder class that builds the data buffers (excluding the null bitmap).
I'll open a separate issue to discuss this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4797437004
@andishgar Just like with `BaseBinaryArray` and other concrete array types, the implicit assumption is that `GetValue` (or `GetView`) should only be called for non-null slots. Perhaps this should be clarified in the respective docstrings?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4851636855
> I'm okay with that, and I'm aware that `SparseUnion`, `DenseUnion`, and `RunEndEncoded` builders have the bitmap buffer in different positions, so I should use a different strategy to check whether the data is null. I'll send a follow-up PR to address this by adding `IsNull` support for all builders.
Actually, I think we should only do it for Builders that have a top-level physical null bitmap (therefore, not REE or Unions).
> 2. I found that the following code doesn't appear to be used anywhere, and its logic also seems incorrect. Shouldn't we remove it?
Good catch, feel free to open an issue and PR then :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4841468383
@pitrou, to wrap up my point: the current `BaseBinaryBuilder` API does not provide a way to determine whether the current value is null while iterating. I used the `compute::unique` implementation for `BinaryView` as an example: because `BaseBinaryBuilder` has no API to check whether a value is null, it ends up using incorrect logic for null values.
Given this, would it make sense to add an `IsNull(index)` API to `BaseBinaryBuilder`? Our discussion is not about the `unique` kernel itself—`unique` is just an example that demonstrates why the current `BaseBinaryBuilder` API is insufficient for iteration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4801136188
@pitrou, the problem is that `BaseBinaryBuilder` does not have any method in its public API to check whether data is null. Should we add such a method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4833027487
Well, it seems that no other build has such a method, why would it be necessary for `BaseBinaryBuilder` specifically?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4841485827
BTW, I’m going to tackle the issue regarding the `BinaryView` unique kernel in a separate PR. However, please consider it here only as an example of why the current API for iterating over `BaseBinaryBuilder` values is insufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4840902367
@pitrou, as I mentioned [here](https://github.com/apache/arrow/issues/50249#issue-4739960981), the current API is problematic because it does not provide a way to determine whether a value is null while iterating over a `BaseBinaryBuilder`. For example, `VisitValues` uses [this code](https://github.com/apache/arrow/blob/12b3eda46a69fbc2e7f6f9c52be345b678f4baae/cpp/src/arrow/util/hashing.h#L862-L867) to iterate over the builder's values, but with the current interface it has no way to distinguish null values from non-null ones. This makes it impossible for callers that need to preserve null semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4795230764
@pitrou, is it okay to change GetView's behavior for null data? I'm planning to apply the patch below – already ran all tests, all pass.
[1b3c02.zip](https://github.com/user-attachments/files/29318720/1b3c02.zip)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4845252004
I understand, but the none of the Builder classes currently have a IsNull or IsValid method. If we want to add one, we should do so for all builder classes, not just BinaryView.
Or we can just circumvent the issue by doing things differently. It could make sense to have some kind of `BinaryViewDataBuilder` class that builds the data buffers (excluding the null bitmap).
And actually, such a separation (full Builder vs. non-null DataBuilder) could probably make sense for other builders too. There are contexts where we already know the null bitmap (for example because it's the same as the input we are operating on, which is common in compute kernels) and just want to build the non-null buffers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Change BaseBinaryBuilder::GetView to return a view with a null data pointer for entries added via AppendNull [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #50249:
URL: https://github.com/apache/arrow/issues/50249#issuecomment-4841119286
@pitrou, binary works fine becuase it uses [this logic ](https://github.com/apache/arrow/blob/12b3eda46a69fbc2e7f6f9c52be345b678f4baae/cpp/src/arrow/util/hashing.h#L964)for accessing the bitmap buffer directly, Note that both BinaryView and BaseBinary Types creates final results from BaseBinaryBuilder, however the way final creating is happend is differnt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org