You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Abhisheklearn12 (via GitHub)" <gi...@apache.org> on 2026/08/01 16:24:52 UTC
[PR] fix(arrow-cast): preserve null dictionary values when casting to a view [arrow-rs]
Abhisheklearn12 opened a new pull request, #10510:
URL: https://github.com/apache/arrow-rs/pull/10510
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
-->
Split out of #10436 at review request, which is the PR linked to #8985.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
`view_from_dict_values` appends a view for every non-null key without checking whether the value it points at is null, so a null dictionary value surfaces as the empty slice its offsets span:
```rust
// values ["aa", NULL, "cc"], keys [0, 1, 2]
cast(&dict, &DataType::Utf8View) // before: ["aa", "", "cc"]
// after: ["aa", null, "cc"]
cast(&dict, &DataType::Utf8) // unchanged: ["aa", null, "cc"]
```
unpack_dictionary goes through take, and impl From<&GenericByteArray> for GenericByteViewArray branches on is_null, so both already produce null here. Only the dictionary to view path disagreed.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Check value validity before appending a view. Also bounds-check the key before indexing the offsets, turning an out of range key from undefined behaviour into an InvalidArgumentError.
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
If this PR claims a performance improvement, please include evidence such as benchmark results.
-->
Yes. The new test fails on the unfixed code, giving `Some("")` where `None` is expected.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
`Dictionary<_, Utf8>` -> `Utf8View `and `Dictionary<_, Binary> -> BinaryView` now produce null for a null dictionary value instead of an empty string.
--
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: [PR] fix(arrow-cast): preserve null dictionary values when casting to a view [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10510:
URL: https://github.com/apache/arrow-rs/pull/10510
--
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: [PR] fix(arrow-cast): preserve null dictionary values when casting to a view [arrow-rs]
Posted by "Abhisheklearn12 (via GitHub)" <gi...@apache.org>.
Abhisheklearn12 commented on code in PR #10510:
URL: https://github.com/apache/arrow-rs/pull/10510#discussion_r3698156636
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -141,9 +143,24 @@ fn view_from_dict_values<K: ArrowDictionaryKeyType, V: ByteArrayType, T: ByteVie
ArrowError::ComputeError("Invalid dictionary index".to_string())
})?;
+ // A dictionary built without validation can carry a key past the end of the
+ // values. Reject it rather than reading out of bounds below.
+ if idx >= values.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Dictionary key {idx} out of bounds for dictionary values of length {}",
+ values.len()
+ )));
+ }
+
Review Comment:
ohh, i checked, you're right. `try_new` and `ArrayDataBuilder::build()` both validate the key range, so i'd need `new_unchecked` to get there, removing it
--
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: [PR] fix(arrow-cast): preserve null dictionary values when casting to a view [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10510:
URL: https://github.com/apache/arrow-rs/pull/10510#discussion_r3697696950
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -7634,6 +7634,53 @@ mod tests {
assert_eq!(casted_binary_array.as_ref(), &binary_view_array);
}
+ #[test]
+ fn test_dict_to_view_null_dictionary_value_is_null() {
+ // A null *value* in the dictionary must produce a null row, not the empty slice that
+ // its offsets happen to span. `unpack_dictionary`, which serves every other target
+ // type, and `impl From<&GenericByteArray> for GenericByteViewArray` both produce null
+ // here, so the view output previously disagreed with every other path.
Review Comment:
```suggestion
// Ensure we preserve nulls in the values
```
can keep this short and simple
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -141,9 +143,24 @@ fn view_from_dict_values<K: ArrowDictionaryKeyType, V: ByteArrayType, T: ByteVie
ArrowError::ComputeError("Invalid dictionary index".to_string())
})?;
+ // A dictionary built without validation can carry a key past the end of the
+ // values. Reject it rather than reading out of bounds below.
+ if idx >= values.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Dictionary key {idx} out of bounds for dictionary values of length {}",
+ values.len()
+ )));
+ }
+
Review Comment:
if a dictionary was built without validation and was incorrect i dont think we need to account for this; it means it already didnt uphold its safety invariant
however if this can be reached without unsafe that would be something we need to 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow-cast): preserve null dictionary values when casting to a view [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10510:
URL: https://github.com/apache/arrow-rs/pull/10510#issuecomment-5172883108
thanks @Abhisheklearn12
--
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