Docs: Clarify that Array::value does not check for nulls#8065

alamb · 2025-08-06T12:21:17Z

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 [Variant] Add variant_get and Shredded VariantArray #8021

Rationale for this change

As part of the review in #8021, @scovich and I were discussing how VariantArray::value should behave in the presence of nulls: #8021 (comment)

Suggest to make this return Option 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

scovich · 2025-08-06T13:08:57Z

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?

arrow-array/src/array/byte_array.rs

alamb · 2025-08-06T15:02:22Z

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

Co-authored-by: Kyle Barron <kylebarron2@gmail.com>

…b/doc_value

viirya · 2025-08-18T17:54:37Z

arrow-array/src/array/boolean_array.rs

    /// 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.


As this calls value_unchecked actually, do we also want to add the note to value_unchecked?

Yes, this is an excellent call. I have done so

viirya · 2025-08-18T17:55:49Z

arrow-array/src/array/boolean_array.rs


    /// Returns the boolean value at index `i`.
+    ///
+    /// Note: This method does not check for nulls and the value is arbitrary


And actually I think all value methods on all arrays behaves like this.

Yes, I agree, that is why i was trying to add the documentation.

alamb

Thank you @viirya

alamb · 2025-08-18T18:21:20Z

arrow-array/src/array/boolean_array.rs

    /// 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.


Yes, this is an excellent call. I have done so

alamb · 2025-08-18T19:54:59Z

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

findepi · 2025-09-03T09:50:31Z

arrow-array/src/array/boolean_array.rs

+    /// Note: This method does not check for nulls and the value is arbitrary
+    /// if [`is_null`](Self::is_null) returns true for the index.


Here and in other places, is it guaranteed not to panic?

yes, it is guaranteed not to panic.

findepi · 2025-09-03T09:52:13Z

arrow-array/src/array/byte_array.rs

    /// 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.


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)

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.

findepi · 2025-09-03T09:53:17Z

arrow-array/src/array/fixed_size_list_array.rs

    /// 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.


but still well-defined

should this be construed as meaning the returned value is of size L, if self is a FixedSizeList(L)?

Docs: Clarify that Array::value does not check for nulls

6ee4c63

alamb added the documentation Improvements or additions to documentation label Aug 6, 2025

github-actions bot added the arrow Changes to the arrow crate label Aug 6, 2025

kylebarron reviewed Aug 6, 2025

View reviewed changes

arrow-array/src/array/byte_array.rs Outdated Show resolved Hide resolved

alamb and others added 4 commits August 6, 2025 11:07

Update arrow-array/src/array/byte_array.rs

140cd5b

Co-authored-by: Kyle Barron <kylebarron2@gmail.com>

clarify defined

618a255

Merge branch 'alamb/doc_value' of github.com:alamb/arrow-rs into alam…

f89c8e0

…b/doc_value

Merge remote-tracking branch 'apache/main' into alamb/doc_value

18fce22

github-actions bot added the parquet-variant parquet-variant* crates label Aug 18, 2025

viirya reviewed Aug 18, 2025

View reviewed changes

viirya approved these changes Aug 18, 2025

View reviewed changes

viirya reviewed Aug 18, 2025

View reviewed changes

Add note to value_unchecked

1c463b0

alamb commented Aug 18, 2025

View reviewed changes

alamb merged commit 78212bf into apache:main Aug 18, 2025
29 checks passed

alamb deleted the alamb/doc_value branch August 18, 2025 19:55

findepi reviewed Sep 3, 2025

View reviewed changes

		/// Note: This method does not check for nulls and the value is arbitrary
		/// if [`is_null`](Self::is_null) returns true for the index.

Conversation

alamb commented Aug 6, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

scovich commented Aug 6, 2025

Uh oh!

Uh oh!

alamb commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Aug 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alamb Aug 18, 2025 •

edited

Loading