ARROW-11149: [Rust] DF Support List/LargeList/FixedSizeList in create_batch_empty by ovr · Pull Request #9114 · apache/arrow

ovr · 2021-01-06T17:33:58Z

Previously build_empty_list_array was declared inside Parquet (array_reader), but I will use this function inside DataFushion's create_batch_empty (it's used inside hash_aggregate to make an empty batch from the provided schema that contains type for columns). I moved it to Arrow (because it's common and useful) and made build_empty_large_list_array (for large lists) on top of macros with different implementation than build_empty_list_array.

ovr · 2021-01-06T17:35:52Z

What ArrowError should I throw on unimplemented? todo!? Or let's declare this one?
Thanks

github-actions · 2021-01-06T20:28:15Z

https://issues.apache.org/jira/browse/ARROW-11149

jorgecarleitao · 2021-01-07T07:08:26Z

I believe that we can make this with generics, now that we have GenericListBuilder::<Offset>(). It would make the code simpler to read, and a consistent function signature.

ovr · 2021-01-07T15:38:23Z

@jorgecarleitao You are right, but I was thinking about modifying make_empty_list_fn macros to support FixedSizeListBuilder, which is not GenericListBuilder.

Something like:

make_empty_list_fn!(build_empty_fixed_size_list_array, FixedSizeListBuilder);

But I am having a problem with constructing it, FixedSizeListBuilder requires 2 arguments in new method instead of one like GenericListBuilder.

pub fn new(values_builder: T, length: i32) -> Self {

Do you have any solutions how is it possible to do by Marcos? And is it a good idea?

Thanks!

jorgecarleitao · 2021-01-07T16:10:04Z

Last time I tried something similar, I had to implement the FixedSizeList differently. A fixed sized will always need to receive an extra parameter.

nevi-me · 2021-01-07T16:11:11Z

Yes, better to implement it differently

ovr · 2021-01-07T17:14:26Z

@jorgecarleitao and @nevi-me 👍

I've moved build_empty_list_array to function build_empty_list_array<OffsetSize: OffsetSizeTrait>, and introduce build_empty_fixed_size_list_array by different implementation as suggested above.

Thanks

In apache#9114, I've prepared support for List/LargeList/FixedSizeList, but will be great to support more types

ovr · 2021-01-11T22:47:58Z

@andygrove Can you take a look? Thanks

jorgecarleitao

I went through this, and it looks good and really useful.

I would just prefer that we have at least 1 test for this, for verification. I left some other minor comments, all optional.

Great work, @ovr

jorgecarleitao · 2021-01-15T05:22:34Z

Note that for an empty list, we know that the offset buffer will be a single entry, 0, and the values buffer will be an empty buffer (len = 0). Therefore, this code could be simplified by just passing the buffers directly instead of using builders.

Arrow API is a Complex, I still dont know how to do it pretty simple as you suggested with Buffer. I think it's not a big performance impact to use builder in this place.

jorgecarleitao · 2021-01-15T05:23:12Z

Could you add a test just to verify?

Thanks, added.

jorgecarleitao · 2021-01-15T05:23:49Z

What do you think of Unimplemented instead of Unsupported? Just to be consistent with the unimplemented! macro that rust already offers.

I found a better name from reading parquet sources, It's called NYI (Not yet implemented), I think it's better to use similar names accross packages.
Renamed.
Thanks

codecov-io · 2021-01-16T14:18:18Z

Codecov Report

Merging #9114 (849d8d3) into master (1393188) will decrease coverage by 0.01%.
The diff coverage is 39.79%.

@@ Coverage Diff @@## master #9114 +/- ##
==========================================
- Coverage 81.61% 81.59% -0.02% 
==========================================
Files 215 215 Lines 51867 51928 +61 ==========================================
+ Hits 42329 42370 +41 - Misses 9538 9558 +20

Impacted Files	Coverage Δ
rust/datafusion/src/physical_plan/common.rs	`74.28% <0.00%> (-4.51%)`	⬇️
rust/parquet/src/arrow/array_reader.rs	`73.65% <0.00%> (+2.25%)`	⬆️
rust/arrow/src/array/array_list.rs	`83.33% <43.67%> (-9.78%)`	⬇️
rust/arrow/src/error.rs	`9.52% <50.00%> (-0.48%)`	⬇️
rust/parquet/src/encodings/encoding.rs	`94.86% <0.00%> (-0.20%)`	⬇️
rust/arrow/src/array/builder.rs	`86.30% <0.00%> (+0.40%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaa7b7a...c898107. Read the comment docs.

jorgecarleitao · 2021-01-19T17:49:42Z

I think that the error name should be explicit, like all others. I can't understand what NYI means without having to go to the docs, and in an error message, the person often has no access to the docs (at least not in a 1 click thing).
NotYetImplemented?

I agree that NotYetImplemented is a better name

alamb

Thanks @ovr -- I think this PR is looking good to go.

Ideally we would could get the rename of NYI to NotYetImplemented but I also think we could merge this PR and rename the enum in a follow on PR

However, since this this PR needs a rebase, sadly, perhaps we can do the rename as part of the rebase.

alamb · 2021-01-20T12:50:34Z

I agree that NotYetImplemented is a better name

…_batch_empty

ovr · 2021-01-20T14:55:18Z

@alamb Rebased + renamed NYI -> NotYetImplemented in Arrow. Thanks

alamb · 2021-01-20T18:15:40Z

Awesome -- thanks @ovr !

alamb · 2021-01-20T18:17:02Z

The travis CI run is backed up -- https://github.com/apache/arrow/pull/9114/checks?check_run_id=1735587739 hasn't finished -- and this PR has no non-rust changes. I think it is good to go

…_batch_empty Previously `build_empty_list_array` was declared inside Parquet (`array_reader`), but I will use this function inside DataFushion's `create_batch_empty` (it's used inside hash_aggregate to make an empty batch from the provided schema that contains type for columns). I moved it to Arrow (because it's common and useful) and made `build_empty_large_list_array` (for large lists) on top of macros with different implementation than build_empty_list_array. Closes#9114 from ovr/issue-11149 Authored-by: Dmitry Patsura <zaets28rus@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

ovr force-pushed the issue-11149 branch from 0e3c6d1 to 12929f8Compare January 6, 2021 17:34

ovr commented Jan 6, 2021
View reviewed changes

github-actionsBot added Component: Rust - DataFusion Component: Rust Component: Parquet labels Jan 6, 2021

ovr force-pushed the issue-11149 branch from 12929f8 to da9aa33Compare January 6, 2021 22:06

ovr force-pushed the issue-11149 branch from 22d28fa to d32c89aCompare January 7, 2021 16:56

ovr changed the title ~~ARROW-11149: [Rust] DF Support List/LargeList in create_batch_empty~~ARROW-11149: [Rust] DF Support List/LargeList/FixedSizeList in create_batch_emptyJan 7, 2021

ovr added a commit to ovr/arrow that referenced this pull request Jan 10, 2021
ARROW-11201: [Rust][DataFusion] create_batch_empty - support more types
bb9c4b8
In apache#9114, I've prepared support for List/LargeList/FixedSizeList, but will be great to support more types

ovr mentioned this pull request Jan 10, 2021
ARROW-11201: [Rust][DataFusion] create_batch_empty - support more types #9156
Closed

ovr force-pushed the issue-11149 branch 2 times, most recently from 3da157f to f0251d7Compare January 13, 2021 21:52

jorgecarleitao reviewed Jan 15, 2021
View reviewed changes

ovr force-pushed the issue-11149 branch 2 times, most recently from 849d8d3 to c898107Compare January 16, 2021 14:03

jorgecarleitao reviewed Jan 19, 2021
View reviewed changes

alamb approved these changes Jan 20, 2021
View reviewed changes

alamb mentioned this pull request Jan 20, 2021
ARROW-11327: [Rust][DataFusion] Add DictionarySupport to create_batch_empty #9276
Closed

alamb added the needs-rebase A PR that needs to be rebased by the author label Jan 20, 2021

ovr force-pushed the issue-11149 branch from c898107 to 46e95bcCompare January 20, 2021 14:51

ARROW-11149: [Rust] DF Support List/LargeList/FixedSizeList in create…
2ab5653
…_batch_empty

ovr force-pushed the issue-11149 branch from 46e95bc to 2ab5653Compare January 20, 2021 14:53

alamb closed this in 23550c2Jan 20, 2021

ovr deleted the issue-11149 branch January 20, 2021 18:26

asfimport mentioned this pull request Jan 20, 2021
[Rust] create_batch_empty - support List, LargeList #27059
Closed

Uh oh!

Conversation

ovr commented Jan 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ovrJan 6, 2021• 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!

github-actionsBot commented Jan 6, 2021

Uh oh!

jorgecarleitao commented Jan 7, 2021

Uh oh!

ovr commented Jan 7, 2021

Uh oh!

jorgecarleitao commented Jan 7, 2021

Uh oh!

nevi-me commented Jan 7, 2021

Uh oh!

ovr commented Jan 7, 2021

Uh oh!

ovr commented Jan 11, 2021

Uh oh!

jorgecarleitao left a comment

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!

ovrJan 16, 2021• 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!

codecov-io commented Jan 16, 2021

Codecov Report

Uh oh!

Choose a reason for hiding this comment

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!

ovr commented Jan 20, 2021

Uh oh!

alamb commented Jan 20, 2021

Uh oh!

alamb commented Jan 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ovr commented Jan 6, 2021•
edited
Loading

ovrJan 6, 2021•
edited
Loading

ovrJan 16, 2021•
edited
Loading

alamb commented Jan 20, 2021•
edited
Loading