ARROW-4285: [Python] Use proper builder interface for serialization #3423

pcmoritz · 2019-01-18T06:49:14Z

The UnionBuilder API is still pretty rough, feedback on how it should look is appreciated. It also raises the question on what the best way to do type inference (i.e. which type will the UnionArray have?). Having this in the builder would be super useful though also for other applications where the schema is not known in advance.

codecov-io · 2019-01-18T07:24:25Z

Codecov Report

Merging #3423 into master will increase coverage by 0.81%.
The diff coverage is 90.54%.

@@            Coverage Diff             @@
##           master    #3423      +/-   ##
==========================================
+ Coverage   88.23%   89.04%   +0.81%     
==========================================
  Files         635      509     -126     
  Lines       80181    71122    -9059     
  Branches     1069        0    -1069     
==========================================
- Hits        70751    63334    -7417     
+ Misses       9317     7788    -1529     
+ Partials      113        0     -113

Impacted Files	Coverage Δ
cpp/src/arrow/python/serialize.h	`0% <ø> (ø)`	⬆️
cpp/src/arrow/array/builder_union.cc	`100% <100%> (ø)`
cpp/src/arrow/array/builder_union.h	`100% <100%> (ø)`
cpp/src/arrow/array/builder_nested.cc	`97.4% <100%> (+0.34%)`	⬆️
cpp/src/arrow/python/serialize.cc	`84.09% <81.91%> (-3.68%)`	⬇️
cpp/src/arrow/python/deserialize.cc	`90.43% <96.55%> (+3.76%)`	⬆️
cpp/src/plasma/thirdparty/dlmalloc.c	`38.29% <0%> (-7%)`	⬇️
python/pyarrow/plasma.py	`57.14% <0%> (-1.2%)`	⬇️
cpp/src/plasma/thirdparty/ae/ae.c	`71.09% <0%> (-0.95%)`	⬇️
cpp/src/parquet/column_reader.cc	`90.3% <0%> (-0.16%)`	⬇️
... and 143 more

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 de84293...0c6ceda. Read the comment docs.

cpp/src/arrow/array/builder_nested.cc

wesm · 2019-01-19T16:41:21Z

rebased. I had to squash because the prior merge commit made rebasing on master impossible

pitrou

This looks like a nice refactor. Please find a couple remarks below.

cpp/src/arrow/array/builder_nested.cc

cpp/src/arrow/array/builder_union.h

pitrou · 2019-01-22T17:56:47Z

cpp/src/arrow/array/builder_nested.cc

When does it happen? The same initialization already happens in ListBuilder::ListBuilder above.

Yes, this is for the case in which types are inferred at runtime.

Especially for nested builders and union builders, that's a very useful feature (potentially also for parsing CSV files and other operations where we don't know the types beforehand; sometimes it can be precomputed but espeically if there is a deeply nested data structure, that might be unnatural like for this PR). The way I have implemented it for this PR is by specifying a "nullptr" instead of a type, in which case the type will be computed upon Finish (and it will have the correct type including all the nested types). This is what this line is doing.

The way it is currently done in the PR shouldn't be merged, but we should have a good way to do this. There are two possibilities that come to my mind (they are all not super great, I hope you can come up with a better one):

Introduce and special type (Type::DYNAMIC or TYPE::BUILDING) which only be a type of a Builder, not an Array. If the Builder has this type, the real type will be inferred upon Finish(). This is compatible with the current API, but introduces this awkward type (and we have to be careful people don't use it for normal Arrays).

Change the builder API and remove the type() method. This might be too intrusive.

Do it similar to the way I'm doing it right now, and have a check in the type() method that the type is not nullptr.

Let me know about your thoughts @pitrou @wesm @xhochy

Another possibility I had considered is to have a boolean flag for the constructor of the builder that makes the type inference dynamic (in which case the type() method will check fail if it is called before the builder is finished).

I realize you may have misunderstood my question above. To rephrase it: the type is already inferred in the ListBuilder constructor, why is it needed to infer it again here?

The point of this is to not infer the type in the constructors, but in the Finish method. For more complex array builders like UnionBuilder, that's the first time we can reasonably know what the type is.

pitrou · 2019-01-22T17:58:38Z

cpp/src/arrow/array/builder_nested.cc

This would preferably be done at constructor time, so that builder->type() returns something valid.

Also, I think you should add a StructBuilder constructor without a type argument, to reflect the fact that the type is now optional.

Yeah, this needs some more discussion I think, see my above comment.

I still think it would be better to do this inference in the StructBuilder constructor, just like we do for ListBuilder. Is there a reason not to do it?

cpp/src/arrow/python/serialize.cc

pitrou

Thanks for the updates! I think this is mostly good to go. Just a couple small issues remaining.

pitrou · 2019-01-28T15:16:32Z

cpp/src/arrow/array/builder_union.h

DenseUnionBuilder

pitrou · 2019-01-28T15:28:36Z

cpp/src/arrow/python/deserialize.cc

AFAIK, the recommended way to pass the templated callables is to use "forwarding references" a.k.a. "universal references", in other words:

..., CreateSequenceFn&& create_sequence, SetItemFn&& set_item, ...

pitrou · 2019-01-28T15:29:33Z

cpp/src/arrow/python/deserialize.cc

Need to check for errors after this (especially, creating a large set or tuple might fail with a MemoryError).

pitrou · 2019-01-28T15:33:20Z

cpp/src/arrow/python/deserialize.cc

Other set_item lambdas steal a reference to item, but PySet_Add doesn't, so you need to Py_DECREF(item) by yourself here.

Thanks, good catch! Should be fixed now.

pitrou · 2019-01-28T15:34:48Z

cpp/src/arrow/python/serialize.h

Not sure the name MAX is a good idea, it might be a preprocessor-defined macro on some systems...

renamed it, good catch

cpp/src/arrow/python/serialize.cc

pcmoritz · 2019-01-29T07:38:17Z

So I resolved the comments and also made the type inference for the builders more explicit. My preferred solution here in the longer term would be to make type inference the default (at least for non-primitive builders). What do you think?

pitrou · 2019-01-29T09:23:42Z

My preferred solution here in the longer term would be to make type inference the default (at least for non-primitive builders).

In which context?

pcmoritz · 2019-01-29T09:33:42Z

relating to this comment: #3423 (comment)

cpp/src/arrow/array/builder_nested.h

pitrou · 2019-01-29T12:05:20Z

Btw, the Travis-CI failure is unrelated, you can rebase from master to get it fixed.

pcmoritz · 2019-01-29T21:57:44Z

rebased

pcmoritz · 2019-01-30T05:28:42Z

This is now ready to merge

pitrou

Still two questions I don't remember getting an answer to :-)

pitrou · 2019-01-30T14:58:54Z

cpp/src/arrow/array/builder_nested.cc

  }

+  // If the type has not been specified in the constructor, infer it
+  if (!arrow::internal::checked_cast<ListType&>(*type_).value_type()) {


I'll ask again: why are you doing this, since it is already done in the constructor? (see above)
Is there a situation where the ListBuilder constructor isn't called?

Ah, this is for the case where type_.value_type() has been initialized to nullptr in the constructor (i.e. the type wasn't known when the ListBuilder was constructed so it was nullptr there), so it will be computed in the Finish method. This is used if the type is inferred automatically during Finish. This requires btw that .Finish of the value builder is called first (which it is). Does that make sense?

Ahh.... Do you mean when value_builder_ itself is a type-inferred union builder? If so, can you add a comment? :-) Thanks.

added the comment :)

pitrou · 2019-01-30T14:59:24Z

cpp/src/arrow/array/builder_nested.cc

+    RETURN_NOT_OK(children_[i]->FinishInternal(&child_data[i]));
  }

+  // If the type has not been specified in the constructor, infer it


Again, I wonder why this couldn't be made in the StructBuilder constructor, where the fields are already known.

Similar to above, if the types associated to the StructBuilder were nullptr at construction time, they will be computed when .Finish is called.

pcmoritz · 2019-01-30T23:55:54Z

Maybe I can make this point a little bit clearer:

This PR makes it so that for any of the complex builders (Struct, List, Union), you can specify "nullptr" as the type during construction of the builder, then just build the nested arrays as you go along, and at the end (in the Finish method), the builder will recursively compute the right types for the resulting array.

This is distinct to the current mode of operation, where all types need to be known upfront and can be very useful especially for constructing nested arrays where computing the types in advance is hard.

Again I'm not married to the particular API, in fact I think using nullptr as the type is a bit dangerous. I proposed some other ways to do it in #3423 (comment) and the followup post (and implemented one of them in 39982cd).

If this is too controversial to discuss in this PR, I can open a mailing list thread.

pitrou · 2019-01-31T12:12:37Z

Thanks @pcmoritz !

pcmoritz · 2019-01-31T19:53:42Z

Thanks @pitrou !

The UnionBuilder API is still pretty rough, feedback on how it should look is appreciated. It also raises the question on what the best way to do type inference (i.e. which type will the UnionArray have?). Having this in the builder would be super useful though also for other applications where the schema is not known in advance. Author: Philipp Moritz <[email protected]> Closes apache#3423 from pcmoritz/union-builder and squashes the following commits: 0c6ceda <Philipp Moritz> add comment 9d5fcc2 <Philipp Moritz> lint 39982cd <Philipp Moritz> remove infer_type 745ed58 <Philipp Moritz> more comments d5e2300 <Philipp Moritz> comments 9697e69 <Philipp Moritz> dynamic_cast -> checked_cast 0ae5b66 <Philipp Moritz> fixes a82720c <Philipp Moritz> add documentation 91d08e3 <Philipp Moritz> remove macro ff7e052 <Philipp Moritz> use VisitIterable 2b7e4b9 <Philipp Moritz> more cleanups 1db3c21 <Philipp Moritz> cleanups 7b41b05 <Philipp Moritz> starting to refactor UnionBuilder 7488834 <Philipp Moritz> use enum for types, type constructors 80c9838 <Philipp Moritz> clean up history for union builder branch

…bjects This is a regression from #3423, the recursion depth was incremented for arrays and dicts but not for lists, tuples and sets. I also added a regression test for this. Author: Philipp Moritz <[email protected]> Closes #3556 from pcmoritz/recursive-serialization and squashes the following commits: f83cff9 <Philipp Moritz> fix 903788e <Philipp Moritz> fix serialization of objects that contain themselves

xhochy reviewed Jan 18, 2019

View reviewed changes

cpp/src/arrow/array/builder_nested.cc Outdated Show resolved Hide resolved

wesm force-pushed the master branch from 501cde9 to 2e8d389 Compare January 19, 2019 16:33

wesm force-pushed the union-builder branch from 94a169d to b4d488b Compare January 19, 2019 16:40

wesm force-pushed the union-builder branch 2 times, most recently from f678628 to 52cabd7 Compare January 20, 2019 16:50

pitrou requested changes Jan 22, 2019

View reviewed changes

pcmoritz force-pushed the union-builder branch from 0f0fcaa to 8a4bdca Compare January 24, 2019 18:32

pitrou requested changes Jan 28, 2019

View reviewed changes

pitrou reviewed Jan 29, 2019

View reviewed changes

cpp/src/arrow/array/builder_nested.h Outdated Show resolved Hide resolved

pcmoritz force-pushed the union-builder branch from 7169f85 to d1493bc Compare January 29, 2019 09:58

pcmoritz added 14 commits January 29, 2019 12:36

clean up history for union builder branch

80c9838

use enum for types, type constructors

7488834

starting to refactor UnionBuilder

7b41b05

cleanups

1db3c21

more cleanups

2b7e4b9

use VisitIterable

ff7e052

remove macro

91d08e3

add documentation

a82720c

fixes

0ae5b66

dynamic_cast -> checked_cast

9697e69

comments

d5e2300

more comments

745ed58

remove infer_type

39982cd

lint

9d5fcc2

pcmoritz force-pushed the union-builder branch from f81c95a to 9d5fcc2 Compare January 29, 2019 20:36

pitrou reviewed Jan 30, 2019

View reviewed changes

add comment

0c6ceda

pitrou approved these changes Jan 31, 2019

View reviewed changes

pitrou closed this in aa43784 Jan 31, 2019

robertnishihara deleted the union-builder branch February 4, 2019 20:52

pcmoritz mentioned this pull request Feb 4, 2019

ARROW-4475: [Python] Fix recursive serialization of self-containing objects #3556

Closed

guoyuhong mentioned this pull request Mar 2, 2019

Update arrow version to fix plasma bugs ray-project/ray#4127

Merged

This was referenced Jan 31, 2019

[Python] Use proper builder interface for serialization #20860

Closed

[Python] Serializing objects that contain themselves #21032

Closed

[Python] Remove usage of std::to_string and std::stoi #21045

Closed

ARROW-4285: [Python] Use proper builder interface for serialization #3423

ARROW-4285: [Python] Use proper builder interface for serialization #3423

Uh oh!

Conversation

pcmoritz commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

wesm commented Jan 19, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou 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!

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!

Uh oh!

Uh oh!

pcmoritz commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Jan 29, 2019

Uh oh!

pcmoritz commented Jan 29, 2019

Uh oh!

Uh oh!

pitrou commented Jan 29, 2019

Uh oh!

pcmoritz commented Jan 29, 2019

Uh oh!

pcmoritz commented Jan 30, 2019

Uh oh!

pcmoritz commented Jan 18, 2019 •

edited

Loading

codecov-io commented Jan 18, 2019 •

edited

Loading

pcmoritz commented Jan 29, 2019 •

edited

Loading

pitrou Jan 31, 2019 •

edited

Loading