Docs: Clarify that Array::value does not check for nulls #8065
Conversation
Right, sorry -- the caller of |
| /// 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. |
There was a problem hiding this comment.
As this calls value_unchecked actually, do we also want to add the note to value_unchecked?
There was a problem hiding this comment.
Yes, this is an excellent call. I have done so
|
|
||
| /// Returns the boolean value at index `i`. | ||
| /// | ||
| /// Note: This method does not check for nulls and the value is arbitrary |
There was a problem hiding this comment.
And actually I think all value methods on all arrays behaves like this.
There was a problem hiding this comment.
Yes, I agree, that is why i was trying to add the documentation.
| /// 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. |
There was a problem hiding this comment.
Yes, this is an excellent call. I have done so
|
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 |
| /// Note: This method does not check for nulls and the value is arbitrary | ||
| /// if [`is_null`](Self::is_null) returns true for the index. |
There was a problem hiding this comment.
Here and in other places, is it guaranteed not to panic?
There was a problem hiding this comment.
yes, it is guaranteed not to panic.
| /// 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. |
There was a problem hiding this 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)
There was a problem hiding this 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.
| /// 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. |
There was a problem hiding this comment.
but still well-defined
should this be construed as meaning the returned value is of size L, if self is a FixedSizeList(L)?
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.
variant_getand ShreddedVariantArray#8021Rationale for this change
As part of the review in #8021, @scovich and I were discussing how
VariantArray::valueshould behave in the presence of nulls: #8021 (comment)I realized it might not be 100% clear that the existing convention in this crate was that
value()methods did not check for nulls / returnOption. I think we should document it betterWhat changes are included in this PR?
Explicitly document that
valuemethods do not check for nulls and explain what happens when they are used on null valuesAre these changes tested?
Yes, by CI
Are there any user-facing changes?
Additional documentation. No behavior changes