Add more docs and examples for ListArray and OffsetsBuffer by alamb · Pull Request #4607 · apache/arrow-rs

alamb · 2023-07-31T19:17:05Z

Which issue does this PR close?

N/A

Rationale for this change

I personally find ListArray confusing both for myself and to explain to others. Recently I made some pictures while reviewing and discussing with @izveigor in DataFusion apache/datafusion#6936 (comment) and want to add them to the docs so I can keep referring to them

I also think it would be nice to be able to use ListArray without having to refer to the Arrow spec

What changes are included in this PR?

Add more docs and examples for ListArray and OffsetsBuffer

With these changes I think someone has a plausible chance of understanding how to use ListArray without having to refer to the Arrow spec

Are there any user-facing changes?

Better docs

No API changes

arrow-array/src/array/list_array.rs

izveigor

LGTM! I think it is essential information for new arrow-rs users.

arrow-array/src/array/list_array.rs

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

arrow-array/src/array/list_array.rs

arrow-buffer/src/buffer/offset.rs

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

…o alamb/list_array_docs

alamb · 2023-08-01T12:29:10Z

I'll merge this later in the week if there are no more comments

cc @jayzhan211 @smiklos and @vincev who may also be interested

smiklos · 2023-08-01T13:14:55Z

Would it make sense to also document FixedSizeListArray ? It seems to store values differently than GenericListArray

jayzhan211 · 2023-08-01T13:44:30Z

Would it make sense to also document FixedSizeListArray ? It seems to store values differently than GenericListArray

FixedSizeListArray stores value the same as ListArray but each row has the same size of array.

tustvold · 2023-08-01T13:46:39Z

FixedSizeListArray stores value the same as ListArray but each row has the same size of array.

This isn't entirely accurate, there is no set of offsets which means that all slots have the same width. In particular this includes nulls, which can cause some interesting challenges.

jayzhan211 · 2023-08-01T13:47:49Z

arrow-array/src/array/list_array.rs

+/// ┌─────────────┐
+/// │   [A,B,C]   │
+/// ├─────────────┤
+/// │ [] (empty)  │


This is a little confusing to me.
(empty) is just the explanation to []? Showing that [] is empty array?

I think just leaving [] might be better. Maybe an explanation in the comment!

Good idea -- I will make that change.

Edit: Done in 8e6a56d

smiklos · 2023-08-01T13:50:43Z

FixedSizeListArray stores value the same as ListArray but each row has the same size of array.

This isn't entirely accurate, there is no set of offsets which means that all slots have the same width. In particular this includes nulls, which can cause some interesting challenges.

Based on the following array

[[1, 2], Null, [3,4]]

GenericListArray::values will look like this

[1, 2, 3]

But FixedSizeArray::values would look like this

[1, 2, Null, Null, 3, 4]

alamb · 2023-08-01T19:52:14Z

Would it make sense to also document FixedSizeListArray ? It seems to store values differently than GenericListArray

Yes it absolutely would make sense. If you have time that would be great. Otherwise I'll try and make a diagram tomorrow

smiklos · 2023-08-02T07:09:04Z

arrow-array/src/array/list_array.rs

+/// let mut builder = ListBuilder::new(values_builder);
+///
+/// // [A, B, C]
+/// builder.values().append_value("A");


I was thinking that perhaps the example doc of using the builder pattern should go to GenericListBuilder and here we could use the from or try_new_from_array_data to construct the array directly from values array and nulls.

FixedSizeListArray ist documented like this, so I'm in between adding builder based code there or keep the two separated. We could just link to the builder based approach from Generic/Fixed list array

I was torn on this, as keeping the example (that constructs the data in the diagram) I thought added value. However I think your idea of adding a link to the approach would be good.

I implemented this change in 151564d

smiklos · 2023-08-02T08:20:59Z

Would it make sense to also document FixedSizeListArray ? It seems to store values differently than GenericListArray

Yes it absolutely would make sense. If you have time that would be great. Otherwise I'll try and make a diagram tomorrow

I added this pr, largely based on the art in this pr.

arrow-array/src/array/list_array.rs

arrow-array/src/builder/generic_list_builder.rs

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

alamb added 2 commits July 31, 2023 15:11

Add more docs and examples for ListArray and OffsetsBuffer

7af4ee2

fix docs

ff72a5e

github-actions bot added the arrow Changes to the arrow crate label Jul 31, 2023

izveigor reviewed Jul 31, 2023

View reviewed changes

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

izveigor approved these changes Jul 31, 2023

View reviewed changes

tustvold reviewed Jul 31, 2023

View reviewed changes

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

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

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

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

alamb and others added 6 commits July 31, 2023 15:53

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

b593aaf

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

e051478

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

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

88efb57

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

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

c0b3411

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

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

05faa45

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

ascii repair

f03fd91

tustvold reviewed Jul 31, 2023

View reviewed changes

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

tustvold approved these changes Jul 31, 2023

View reviewed changes

viirya reviewed Jul 31, 2023

View reviewed changes

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

viirya reviewed Jul 31, 2023

View reviewed changes

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

viirya reviewed Jul 31, 2023

View reviewed changes

arrow-buffer/src/buffer/offset.rs Outdated Show resolved Hide resolved

viirya reviewed Jul 31, 2023

View reviewed changes

arrow-buffer/src/buffer/offset.rs Show resolved Hide resolved

viirya approved these changes Jul 31, 2023

View reviewed changes

alamb and others added 4 commits August 1, 2023 06:36

Merge remote-tracking branch 'apache/master' into alamb/list_array_docs

6f31380

Apply suggestions from code review

60a6e06

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

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

ecd5557

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

Merge branch 'alamb/list_array_docs' of github.com:alamb/arrow-rs int…

5d0101c

…o alamb/list_array_docs

jayzhan211 reviewed Aug 1, 2023

View reviewed changes

smiklos reviewed Aug 2, 2023

View reviewed changes

smiklos mentioned this pull request Aug 2, 2023

expand docs for FixedSizeListArray #4622

Merged

alamb added 3 commits August 2, 2023 05:57

Merge remote-tracking branch 'apache/master' into alamb/list_array_docs

7d423d8

Clarify empty list [] documenation and typos

8e6a56d

Move example to GenericLisBuilder

151564d

tustvold reviewed Aug 2, 2023

View reviewed changes

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

arrow-array/src/builder/generic_list_builder.rs Outdated Show resolved Hide resolved

alamb and others added 2 commits August 2, 2023 12:47

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

25d9d5a

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

Update arrow-array/src/builder/generic_list_builder.rs

4e9829b

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

tustvold merged commit 08e4692 into apache:master Aug 2, 2023

Conversation

alamb commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Uh oh!

izveigor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented Aug 1, 2023

Uh oh!

smiklos commented Aug 1, 2023

Uh oh!

jayzhan211 commented Aug 1, 2023

Uh oh!

tustvold commented Aug 1, 2023

Uh oh!

jayzhan211 Aug 1, 2023 • 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!

jayzhan211 Aug 1, 2023 • 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 Aug 2, 2023 • 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!

smiklos commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Aug 1, 2023

Uh oh!

smiklos Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

smiklos commented Aug 2, 2023

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alamb commented Jul 31, 2023 •

edited

Loading

jayzhan211 Aug 1, 2023 •

edited

Loading

jayzhan211 Aug 1, 2023 •

edited

Loading

alamb Aug 2, 2023 •

edited

Loading

smiklos commented Aug 1, 2023 •

edited

Loading