You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "tobixdev (via GitHub)" <gi...@apache.org> on 2025/11/24 08:25:35 UTC
[PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
tobixdev opened a new pull request, #18903:
URL: https://github.com/apache/datafusion/pull/18903
## Which issue does this PR close?
- Closes https://github.com/apache/datafusion/issues/18870.
We should open a follow-up issue that tracks re-use `FixedSizeBinaryArray::new_null` after upgrading arrow-rs.
With this PR, we could backport it to #18843 if we wish.
Alternatively, we could also wait for a fix by arrow-rs.
## Rationale for this change
As we will have to wait for another arrow-rs update to get the fix for the issue https://github.com/apache/arrow-rs/pull/8901 , we have a temporaray fix that basically calls the same operations from DataFusion's code. Once the fix is in the arrow-rs codebase, we can re-use `FixedSizeBinaryArray::new_null`.
## What changes are included in this PR?
- A test. (`assert_eq!(result.as_fixed_size_binary().values().len(), 10);` returned 0 before)
- Directly allocate the values buffer in DataFusion.
## Are these changes tested?
Yes
## Are there any user-facing changes?
Yes, the buffer will now be allocated, zeroed, and set to the expected length. Same as in the arrow-rs fix.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3592359829
Thanks for letting me know, I didn't see it. I'll fix it today or tomorrow. 👍
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3580022677
> I don't see any way to create a new `FixedSizeBinaryArray` with zero length using `try_new`. Is this on-purpose? The comments says 0 should be fine. Is this a bug in arrow-rs?
This does seem like a bug; for reference, pyarrow allows it:
```python
>>> import pyarrow as pa
>>> pa.binary(0)
FixedSizeBinaryType(fixed_size_binary[0])
>>> pa.array([], type=pa.binary(0))
<pyarrow.lib.FixedSizeBinaryArray object at 0x102030b80>
[]
>>> pa.array([], type=pa.binary(0)).type
FixedSizeBinaryType(fixed_size_binary[0])
```
It would be nice to upstream a proper fix to arrow-rs and then can wait for it here; otherwise if we need a fix for DataFusion now we can do a workaround as you describe:
> We maybe could create a `FixedSizeBinaryArray` using the `From<ArrayData>` implementation.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3580319293
@Jefffrey Thanks for checking against pyarrow!
Here is a PR that fixes the issue in arrow-rs.
https://github.com/apache/arrow-rs/pull/8927
I also found out that we can create zero-sized item arrays using the builder API. I'll give that a try.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3591659700
Should be good to merge once the conflict is fixed fyi @tobixdev
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3609021302
@Jefffrey Thanks for the review! Should we track an issue for re-using `FixedSizeBinaryArray::new_null` after an arrow-rs upgrade? Maybe creating an "update arrow-rs" issue with a task to replace the TODO with the old code?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3592780784
@Jefffrey Should be good to go. 👍
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3609330586
> @Jefffrey Thanks for the review! Should we track an issue for re-using `FixedSizeBinaryArray::new_null` after an arrow-rs upgrade? Maybe creating an "update arrow-rs" issue with a task to replace the TODO with the old code?
Raising an issue to track this refactor sounds reasonable
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #18903:
URL: https://github.com/apache/datafusion/pull/18903
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3593548222
Thanks @tobixdev
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allocate a buffer of the correct length for ScalarValue::FixedSizeBinary in ScalarValue::to_array_of_size [datafusion]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #18903:
URL: https://github.com/apache/datafusion/pull/18903#issuecomment-3569698062
Hmm seems this change causes problems with `DataType::FixedSizeBinary(0)` as it divides by zero in `FixedSizeBinaryArray::try_new`.
```rust
/// Create a new [`FixedSizeBinaryArray`] from the provided parts, returning an error on failure
///
/// # Errors
///
/// * `size < 0`
/// * `values.len() / size != nulls.len()`
pub fn try_new(
size: i32,
values: Buffer,
nulls: Option<NullBuffer>,
) -> Result<Self, ArrowError> {
let data_type = DataType::FixedSizeBinary(size);
let s = size.to_usize().ok_or_else(|| {
ArrowError::InvalidArgumentError(format!("Size cannot be negative, got {size}"))
})?;
let len = values.len() / s; <--- crash
```
I don't see any way to create a new `FixedSizeBinaryArray` with zero length using `try_new`. Is this on-purpose? The comments says 0 should be fine. Is this a bug in arrow-rs?
We maybe could create a `FixedSizeBinaryArray` using the `From<ArrayData>` implementation.
Any preferences on how we should proceed? As I am unsure whether we even want to fix this issue on the DataFusion side (or wait for an arrow-rs update), I'll wait another opinion.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org