-
Notifications
You must be signed in to change notification settings - Fork 839
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
Conversation
arrow-array/src/builder/mod.rs
Outdated
/// // | ||
/// // column | ||
/// // --------- | ||
/// // {one}, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
SHould I propose changing that too?
There was a problem hiding this comment.
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 [
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed
- https://github.com/apache/arrow-rs/blob/master/arrow-cast/src/pretty.rs#L291
- https://github.com/apache/arrow-rs/blob/master/arrow-json/src/raw/mod.rs#L668
Although I do realise our test coverage of this is fairly limited...
That looks like the output of a UnionArray tbh?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arrow-rs/arrow-cast/src/pretty.rs
Lines 291 to 321 in 0e80ce6
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] |", | |
"+-----------+", | |
]; |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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