Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: add examples for ListBuilder and GenericListBuilder #3891

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Mar 20, 2023

Which issue does this PR close?

N/A

Rationale for this change

I was initially confused about this API and @appletreeisyellow also asked about it on https://github.com/influxdata/influxdb_iox/pull/7198#discussion_r1142581652.

The docs don't help much https://docs.rs/arrow/35.0.0/arrow/array/struct.GenericListBuilder.html

Thus I conclude more documentation is in order

What changes are included in this PR?

Add doc comments

Are there any user-facing changes?

Docs / examples

@github-actions github-actions bot added the arrow Changes to the arrow crate label Mar 20, 2023
/// //
/// // column
/// // ---------
/// // {one},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps [ might be more obvious than {, which would suggest an object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good idea. I chose the { notation because it is what is used in the pretty print output for ListArray

For example:

https://github.com/influxdata/influxdb_iox/blob/efae5246cf1f2cc13222593f669dc4af3039d0df/influxdb_iox/tests/end_to_end_cases/flightsql.rs#L184-L192

SHould I propose changing that too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that isn't some IOx specific thing, the ArrayFormatter definitely should be using [

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed

Although I do realise our test coverage of this is fairly limited...

That looks like the output of a UnionArray tbh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update to match pretty output

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn test_pretty_format_fixed_size_list() {
// define a schema.
let field_type = DataType::FixedSizeList(
Box::new(Field::new("item", DataType::Int32, true)),
3,
);
let schema = Arc::new(Schema::new(vec![Field::new("d1", field_type, true)]));
let keys_builder = Int32Array::builder(3);
let mut builder = FixedSizeListBuilder::new(keys_builder, 3);
builder.values().append_slice(&[1, 2, 3]);
builder.append(true);
builder.values().append_slice(&[4, 5, 6]);
builder.append(false);
builder.values().append_slice(&[7, 8, 9]);
builder.append(true);
let array = Arc::new(builder.finish());
let batch = RecordBatch::try_new(schema, vec![array]).unwrap();
let table = pretty_format_batches(&[batch]).unwrap().to_string();
let expected = vec![
"+-----------+",
"| d1 |",
"+-----------+",
"| [1, 2, 3] |",
"| |",
"| [7, 8, 9] |",
"+-----------+",
];
shows the pretty format for a fixed size list.

I didn't find any test for a non fixed size list, so I will add one as well as part of a follow on PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2023-03-22 at 2 56 25 PM
Looks good now -- thank you for the suggestion

@tustvold tustvold merged commit 526c57a into apache:master Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants