Documentation Improvements #4381

tustvold · 2023-06-07T17:33:31Z

Which issue does this PR close?

Part of #4071

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-06-07T17:34:28Z

arrow/src/lib.rs

@@ -317,6 +285,36 @@
 //! assert_eq!(string.value(1), "foo");
 //! ```
 //!
+//! # Crate Topology


This is moved lower-down so that the first thing people see is how to use an array

I think this is a good change

tustvold · 2023-06-07T17:34:52Z

arrow/src/lib.rs

@@ -109,7 +77,7 @@
 //! assert_eq!(min(&StringArray::from(vec!["b", "a", "c"])), Some("a"));
 //! ```
 //!
-//! For more examples, and details consult the [arrow_array] docs.
+//! **For more examples, and details consult the [arrow_array] docs.**


This is very easy to miss, as it is sandwiched between a code block and heading. Making it bold helps avoid this

arrow-array/src/array/binary_array.rs

tustvold · 2023-06-08T11:21:35Z

arrow-array/src/array/list_array.rs

@@ -57,6 +57,8 @@ impl OffsetSizeTrait for i64 {
 /// An array of [variable length arrays](https://arrow.apache.org/docs/format/Columnar.html#variable-size-list-layout)
 ///
 /// See [`ListArray`] and [`LargeListArray`]`
+///
+/// See [`GenericListBuilder`](crate::builder::GenericListBuilder) for how to construct a [`GenericListArray`]


For the more complex types I opted to just point people to the builders, whilst the arrays can be constructed in other ways these necessarily require knowledge of how these types are structured, or are limited to a specific subset. I think it is better to just encourage people to use the builders as a first port of call

I think this makes sense 👍

alamb

Thanks @tustvold -- the examples you updated are a great improvement, but removing the per-array type examples I feel pretty strongly is a step backwards for users (though it would certainly be easier to maintain)

arrow-array/src/array/binary_array.rs

alamb · 2023-06-08T12:37:50Z

arrow-array/src/array/boolean_array.rs

+/// # use arrow_array::{Array, BooleanArray};
+/// let arr: BooleanArray = vec![true, true, false].into();
+/// let values: Vec<_> = arr.iter().collect();
+/// assert_eq!(&values, &[Some(true), Some(true), Some(false)])


I don't think this assert adds anything to the doc example: I think it is already clear from the docs what the array contains.

alamb · 2023-06-08T12:39:00Z

arrow-array/src/array/boolean_array.rs

+/// let arr: BooleanArray = vec![true, true, false].into();
+/// let values: Vec<_> = arr.iter().collect();


This example seems unnecessarily complicated to me - why would we encourage this pattern instead of what was there

let arr = BooleanArray::from(vec![true, true, false])

arrow-array/src/array/boolean_array.rs

arrow-array/src/array/byte_array.rs

arrow-array/src/array/dictionary_array.rs

alamb · 2023-06-08T12:41:52Z

arrow-array/src/array/list_array.rs

@@ -57,6 +57,8 @@ impl OffsetSizeTrait for i64 {
 /// An array of [variable length arrays](https://arrow.apache.org/docs/format/Columnar.html#variable-size-list-layout)
 ///
 /// See [`ListArray`] and [`LargeListArray`]`
+///
+/// See [`GenericListBuilder`](crate::builder::GenericListBuilder) for how to construct a [`GenericListArray`]


I think this makes sense 👍

arrow-array/src/array/primitive_array.rs

alamb · 2023-06-08T12:43:04Z

arrow/src/lib.rs

@@ -317,6 +285,36 @@
 //! assert_eq!(string.value(1), "foo");
 //! ```
 //!
+//! # Crate Topology


I think this is a good change

arrow-array/src/array/byte_array.rs

arrow-array/src/array/primitive_array.rs

alamb

👌 very nice -- thank you @tustvold

alamb · 2023-06-08T18:21:49Z

arrow-array/src/array/dictionary_array.rs

 /// assert_eq!(&array, &expected);
 /// ```
+///


alamb · 2023-06-08T18:22:43Z

arrow-array/src/array/list_array.rs

-/// assert_eq!(false, list2.as_any().downcast_ref::<Int32Array>().unwrap().is_valid(1));
-/// assert_eq!(&[6, 7], list3.as_any().downcast_ref::<Int32Array>().unwrap().values());
-/// ```
+/// A [`GenericListArray`] of variable size lists, storing offsets as `i32`.


Can we also add a link here to the builder too?

Suggested change

/// A [`GenericListArray`] of variable size lists, storing offsets as `i32`.

/// A [`GenericListArray`] of variable size lists, storing offsets as `i32`.

///

/// See [`GenericListBuilder`](crate::builder::GenericListBuilder) for how to construct a [`GenericListArray`]

alamb · 2023-06-08T18:22:57Z

arrow-array/src/array/list_array.rs

-/// assert_eq!(false, list2.as_any().downcast_ref::<Int32Array>().unwrap().is_valid(1));
-/// assert_eq!(&[6, 7], list3.as_any().downcast_ref::<Int32Array>().unwrap().values());
-/// ```
+/// A [`GenericListArray`] of variable size lists, storing offsets as `i64`.


Suggested change

/// A [`GenericListArray`] of variable size lists, storing offsets as `i64`.

/// A [`GenericListArray`] of variable size lists, storing offsets as `i64`.

///

/// See [`GenericListBuilder`](crate::builder::GenericListBuilder) for how to construct a [`GenericListArray`]

alamb · 2023-06-08T18:23:48Z

arrow-array/src/array/primitive_array.rs

 pub type Decimal256Array = PrimitiveArray<Decimal256Type>;

 pub use crate::types::ArrowPrimitiveType;

 /// An array of [primitive values](https://arrow.apache.org/docs/format/Columnar.html#fixed-size-primitive-layout)
 ///
+/// # Example: From a Vec


I think these are nice examples 👍

alamb · 2023-06-08T18:24:19Z

arrow-array/src/array/primitive_array.rs

+/// let arr: PrimitiveArray<Int32Type> = vec![Some(1), None, Some(3), None].into();
+/// assert_eq!(4, arr.len());
+/// assert_eq!(2, arr.null_count());
+/// assert_eq!(arr.values(), &[1, 0, 3, 0])


is it worth pointing out that the 0 (for the null locations) are arbitrary values?

tustvold added 2 commits June 7, 2023 18:17

Reorder crate documentation

dc81b87

Add backlinks

46ca2be

github-actions bot added the arrow Changes to the arrow crate label Jun 7, 2023

tustvold commented Jun 7, 2023

View reviewed changes

tustvold added 3 commits June 8, 2023 11:25

Merge remote-tracking branch 'upstream/master' into reorder-docs

2fe7f2a

More examples

4fefb47

Merge remote-tracking branch 'upstream/master' into reorder-docs

e817527

tustvold force-pushed the reorder-docs branch from b7bf5aa to 75547ff Compare June 8, 2023 11:15

tustvold commented Jun 8, 2023

View reviewed changes

arrow-array/src/array/binary_array.rs Show resolved Hide resolved

tustvold requested a review from alamb June 8, 2023 11:19

Link from typedefs to generic types

5f801d4

tustvold force-pushed the reorder-docs branch from 75547ff to 5f801d4 Compare June 8, 2023 11:19

tustvold commented Jun 8, 2023

View reviewed changes

alamb reviewed Jun 8, 2023

View reviewed changes

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

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

tustvold force-pushed the reorder-docs branch from 161c22f to 5f801d4 Compare June 8, 2023 16:21

tustvold added 2 commits June 8, 2023 17:29

Restore examples

3631db0

Example cleanup

83d62df

alamb approved these changes Jun 8, 2023

View reviewed changes

tustvold added 2 commits June 9, 2023 15:34

Review feedback

d1ca801

Merge remote-tracking branch 'upstream/master' into reorder-docs

75d027b

tustvold merged commit 9b2b4ca into apache:master Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation Improvements #4381

Documentation Improvements #4381

tustvold commented Jun 7, 2023

tustvold Jun 7, 2023

alamb Jun 8, 2023

tustvold Jun 7, 2023

tustvold Jun 8, 2023

alamb Jun 8, 2023

alamb left a comment

alamb Jun 8, 2023

alamb Jun 8, 2023

alamb Jun 8, 2023

alamb Jun 8, 2023

alamb left a comment

alamb Jun 8, 2023

alamb Jun 8, 2023

alamb Jun 8, 2023

alamb Jun 8, 2023

alamb Jun 8, 2023

		/// let arr: BooleanArray = vec![true, true, false].into();
		/// let values: Vec<_> = arr.iter().collect();

Documentation Improvements #4381

Documentation Improvements #4381

Conversation

tustvold commented Jun 7, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment