You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/08/06 12:21:20 UTC
[PR] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
alamb opened a new pull request, #8065:
URL: https://github.com/apache/arrow-rs/pull/8065
# 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.
- Related to https://github.com/apache/arrow-rs/pull/8021
# Rationale for this change
As part of the review in https://github.com/apache/arrow-rs/pull/8021, @scovich and I were discussing how `VariantArray::value` should behave in the presence of nulls: https://github.com/apache/arrow-rs/pull/8021#discussion_r2256914173
> Suggest to make this return Option<Variant> so callers don't have to check for null themselves.
I realized it might not be 100% clear that the existing convention in this crate was that `value()` methods did not check for nulls / return `Option`. I think we should document it better
# What changes are included in this PR?
Explicitly document that `value` methods do not check for nulls and explain what happens when they are used on null values
# Are these changes tested?
Yes, by CI
# Are there any user-facing changes?
Additional documentation. No behavior changes
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2257429928
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -304,6 +304,10 @@ impl<T: ByteArrayType> GenericByteArray<T> {
}
/// Returns the element at index `i`
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// (but still valid) if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
Since the opposite of `is_null` is `is_valid`, I'd call this well-defined, not `valid`
```suggestion
/// (but still well-defined) if [`is_null`](Self::is_null) returns true for the index.
```
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2257507472
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -304,6 +304,10 @@ impl<T: ByteArrayType> GenericByteArray<T> {
}
/// Returns the element at index `i`
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// (but still valid) if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
will fix the others
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2283121460
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -185,6 +185,10 @@ impl BooleanArray {
}
/// Returns the boolean value at index `i`.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
Review Comment:
Yes, I agree
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2283082497
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -185,6 +185,10 @@ impl BooleanArray {
}
/// Returns the boolean value at index `i`.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
Review Comment:
And actually I think all `value` methods on all arrays behaves like 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: [PR] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#issuecomment-3160114209
> As part of the review in #8021, @scovich and I were discussing how `VariantArray::value` should behave in the presence of nulls:
I wasn't suggesting to change the signature of that public function... just of the internal `typed_value_to_variant` helper?
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2283080392
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -185,6 +185,10 @@ impl BooleanArray {
}
/// Returns the boolean value at index `i`.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
As this calls `value_unchecked` actually, do we also want to add the note to `value_unchecked`?
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#issuecomment-3160543973
> I wasn't suggesting to change the signature of that public function... just of the internal `typed_value_to_variant` helper?
Right, sorry -- the caller of `typed_value_to_variant` (value()) doesn't return an option, so I figured I would at least explain why that was not returning `Option`
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2283121460
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -185,6 +185,10 @@ impl BooleanArray {
}
/// Returns the boolean value at index `i`.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
Review Comment:
Yes, I agree, that is why i was trying to add the documentation.
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2283129369
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -185,6 +185,10 @@ impl BooleanArray {
}
/// Returns the boolean value at index `i`.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
Yes, this is an excellent call. I have done so
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#issuecomment-3198218204
Thanks again @viirya @scovich and @kylebarron
I merged this PR in as I am trying to get stuff merged before the next release. I am happy to make additional improvements in follow on PRs. Just let me know
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2318426348
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -178,13 +178,20 @@ impl BooleanArray {
/// Returns the boolean value at index `i`.
///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
Here and in other places, is it guaranteed not to panic?
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -276,6 +276,10 @@ impl<T: ByteArrayType> GenericByteArray<T> {
}
/// Returns the element at index `i`
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
Here and in other places, is it guaranteed not to panic and return a valid `T::Native` value corresponding to type `T` (in (theoretical?) case `T::Native` could represent some values not valid for `T`)
##########
arrow-array/src/array/fixed_size_list_array.rs:
##########
@@ -243,6 +243,12 @@ impl FixedSizeListArray {
}
/// Returns ith value of this list array.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// (but still well-defined) if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
> but still well-defined
should this be construed as meaning the returned value is of size L, if self is a FixedSizeList(L)?
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2326714437
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -276,6 +276,10 @@ impl<T: ByteArrayType> GenericByteArray<T> {
}
/// Returns the element at index `i`
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
> Here and in other places, is it guaranteed not to panic and return a valid T::Native value corresponding to type T
Yes, to the best of my knowledge
> (in (theoretical?) case T::Native could represent some values not valid for T)
Yes I think this is possible -- the only one I can think of now is potentially date/time types where some valid integer is not a valid date/time/timestamp instance.
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2326710717
##########
arrow-array/src/array/fixed_size_list_array.rs:
##########
@@ -243,6 +243,12 @@ impl FixedSizeListArray {
}
/// Returns ith value of this list array.
+ ///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// (but still well-defined) if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
yes
--
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] Docs: Clarify that Array::value does not check for nulls [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8065:
URL: https://github.com/apache/arrow-rs/pull/8065#discussion_r2326711440
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -178,13 +178,20 @@ impl BooleanArray {
/// Returns the boolean value at index `i`.
///
+ /// Note: This method does not check for nulls and the value is arbitrary
+ /// if [`is_null`](Self::is_null) returns true for the index.
Review Comment:
yes, it is guaranteed not to panic.
--
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