ARROW-4810: [Format][C++] Add "LargeList" type with 64-bit offsets #3848

pcmoritz · 2019-03-09T02:06:15Z

No description provided.

emkornfield · 2019-03-10T04:28:44Z

format/Schema.fbs

I think this might be backwards incompatible, because the type is encoded with an enum which is just an integer (https://google.github.io/flatbuffers/md__schemas.html), you probably need to put this at the end.

thanks, fixed!

emkornfield · 2019-03-10T04:29:54Z

format/Schema.fbs

Please document that this is type optional for implementations (unless we can get consensus on the mailing list if we want it to be globally supported)

good point, I fixed it!

pcmoritz · 2019-03-11T01:32:17Z

This is ready to review in case there is more feedback!

emkornfield

There was a comment on the jira asking for both c++ and Java implementations before adding a type to schema.fbs might want to discuss this on the mailing list (sorry if I missed the Java side)

pcmoritz · 2019-03-11T06:02:58Z

Thanks for pointing me to that. Yeah, there is currently no Java implementation for this I'm afraid (and I don't know the Java codebase well enough to implement it). If anybody has time to do it that would be great!

emkornfield · 2019-03-11T06:10:08Z

If someone else doesn't speak up I can put it on my to do list, behind getting my currently open PRs checked in (unless this is blocking something, in which case I can reprioritize)

pcmoritz · 2019-03-11T06:14:37Z

Thanks, that would be amazing! I'm not really blocked and will just squash this PR for now and keep working on top of it.

Let me know if I can help you with your currently open PRs!

emkornfield · 2019-03-11T06:58:07Z

Actually, if you want to trade #3644 is missing the C++ side of the implementation if you can pick that up, I can prioritize the work on the java side for this. For now I will just clone this PR and start working on top of it, but if there is a good way to collaborate (on a different PR a using branches was mentioned, I don't know the exact mechanics) let me know.

wesm · 2019-03-11T13:51:54Z

I have funding if you know any Java contractors who might be interested in doing work-for-hire on this project to help with work beyond what the handful of us volunteers are able to do

maartenbreddels · 2019-03-11T14:51:23Z

I started working together with an 'ex'-JVM contractor today actally, maybe we can help out here.

emkornfield · 2019-03-13T04:30:33Z

@maartenbreddels @pcmoritz I'm going to pick up the java side of things (unless you've already started). My aim is to have a PR out sometime towards the end of the week.

emkornfield · 2019-03-13T09:19:26Z

A proof of concept java version (see caveats in commit message): 03956ca @pcmoritz let me know if you want to try to write the integration tests or can I can try to do it later in the week. Also if someone can point me to java benchmarks (and any other tests that should be run), so I can try to assess regressions and upstream API breakages I would appreciate it.

maartenbreddels · 2019-03-13T09:50:47Z

My aim is to have a PR out sometime towards the end of the week.

👍

pcmoritz · 2019-03-14T02:19:32Z

@emkornfield Thanks, this is great! I got started on the C++ side of DurationInterval and plan to have something by the end of the week. Unfortunately I can't commit to more at the moment :(

emkornfield · 2019-03-14T02:49:25Z

Actually if you want to share what you have I wouldn't mind doing it myself for durationinterval I want to get more familiar with the code. I think we should discuss use cases for LargeList on the ML since I think it is limited by other parts of the arrow spec but I might be misunderstanding something

pcmoritz · 2019-03-14T02:56:07Z

I haven't gotten too far yet: master...pcmoritz:interval-cpp

Most of the code will be pretty similar to this PR (in terms of what needs to be added, etc) :)

Feel free to continue/make your own branch!

emkornfield · 2019-03-14T05:55:33Z

Thanks. Also I commented on the JIRA, but my confusion over utility was based on the memory layout document to indicates arrays should still only be 32-bit but Message.fbs was updated quite a while ago to indicate 64-bit offsets.

emkornfield · 2019-03-15T05:53:42Z

format/Schema.fbs


+// List with 64-bit offsets. This type is optional and currently
+// only supported by the C++ implementation.
+table LargeList {


@fsaintjacques had a suggestion which I liked and I think is worth discussing to parameterize List instead of creating a new type. I know @wesm suggested he prefers this as a separate type like you have here, but I'm not sure what the rationale is.

pitrou

On the principle this must be validated through the ML discussion. But here are some comments on the implementation.

pitrou · 2019-03-18T18:06:11Z

cpp/src/arrow/type.h

    LIST,

+    /// A list of some logical data type, using 64-bit offsets
+    LARGE_LIST,


Putting this before the end of the enum will break the ABI. I'm not sure we care. @xhochy Comments ?

We sadly break the ABI quite a lot, I've given up a bit on keeping it stable. I'll look into this in 3-6 months again.

pitrou · 2019-03-18T18:06:43Z

cpp/src/arrow/type.h

+
+  std::string ToString() const override;
+
+  std::string name() const override { return "large_list"; }


Or perhaps "list64".

pitrou · 2019-03-18T18:06:57Z

cpp/src/arrow/type.h

+///
+/// List data is nested data where each value is a variable number of
+/// child items.  Lists can be recursively nested, for example
+/// list(list(int32)).


"large_list(large_list(int32))".

pitrou · 2019-03-18T18:08:27Z

cpp/src/arrow/ipc/writer.cc

  Status Visit(const BinaryArray& array) override { return VisitBinary(array); }

-  Status Visit(const ListArray& array) override {
+  template <typename ListArrayType, typename OffsetType>


Perhaps the offset type should be available as ListType::offset_type and LargeListType::offset_type.

pitrou · 2019-03-18T18:11:15Z

cpp/src/arrow/pretty_print.cc

  template <typename T>
-  inline typename std::enable_if<std::is_base_of<ListArray, T>::value, Status>::type
+  inline typename std::enable_if<std::is_base_of<ListArray, T>::value ||
+                                     std::is_base_of<LargeListArray, T>::value,


Perhaps a BaseListArray base class would be useful (and perhaps also a BaseListType).
Also enable_if_list<T> in type_traits.h may deserve updating.

pitrou · 2019-03-18T18:12:14Z

cpp/src/arrow/array-list-test.cc

  ASSERT_EQ("counts", type.value_field()->name());
 }

+TEST_F(TestLargeListArray, TestBuilderPreserveFieleName) {


Do we really want to copy/paste all ListArray tests instead of using some kind of parametrization?

pitrou · 2019-03-18T18:14:26Z

cpp/src/arrow/array/builder_nested.h

+/// represent multiple different logical types.  If no logical type is provided
+/// at construction time, the class defaults to List<T> where t is taken from the
+/// value_builder/values that the object is constructed with.
+class ARROW_EXPORT LargeListBuilder : public ArrayBuilder {


Instead of copy/pasting the ListBuilder implementation, how about factoring it into a templated base class that both concrete implementations can inherit from?

emkornfield · 2019-06-12T06:07:36Z

@pcmoritz were you going to take this up again?

pitrou · 2019-07-30T15:49:39Z

Recommending to close this in favour of #4969

emkornfield · 2019-08-01T03:36:59Z

Closing in favor of #4969 which is merged

emkornfield requested changes Mar 10, 2019

View reviewed changes

emkornfield reviewed Mar 11, 2019

View reviewed changes

implement LargeListArray, LargeListBuilder and IPC methods

c885e90

pcmoritz force-pushed the large-list-array branch from de53f93 to c885e90 Compare March 11, 2019 06:21

pcmoritz mentioned this pull request Mar 11, 2019

[WIP] [Python] Support 64 bit indices for pyarrow.serialize and pyarrow.deserialize #3858

Closed

emkornfield reviewed Mar 15, 2019

View reviewed changes

pitrou requested changes Mar 18, 2019

View reviewed changes

kszucs force-pushed the master branch 2 times, most recently from ed180da to 85fe336 Compare July 22, 2019 19:29

emkornfield closed this Aug 1, 2019

asfimport mentioned this pull request Aug 1, 2019

[Format][C++] Add "LargeList" type with 64-bit offsets #21327

Closed


		std::string ToString() const override;

		std::string name() const override { return "large_list"; }

ARROW-4810: [Format][C++] Add "LargeList" type with 64-bit offsets #3848

ARROW-4810: [Format][C++] Add "LargeList" type with 64-bit offsets #3848

Uh oh!

Conversation

pcmoritz commented Mar 9, 2019

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!

pcmoritz commented Mar 11, 2019

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Mar 11, 2019

Uh oh!

emkornfield commented Mar 11, 2019

Uh oh!

pcmoritz commented Mar 11, 2019

Uh oh!

emkornfield commented Mar 11, 2019

Uh oh!

wesm commented Mar 11, 2019

Uh oh!

maartenbreddels commented Mar 11, 2019

Uh oh!

emkornfield commented Mar 13, 2019

Uh oh!

emkornfield commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maartenbreddels commented Mar 13, 2019

Uh oh!

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

Uh oh!

Uh oh!

emkornfield commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

emkornfield commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

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!

emkornfield commented Jun 12, 2019

Uh oh!

pitrou commented Jul 30, 2019

Uh oh!

emkornfield commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

emkornfield commented Mar 13, 2019 •

edited

Loading

pcmoritz commented Mar 14, 2019 •

edited

Loading

emkornfield commented Mar 14, 2019 •

edited

Loading

pcmoritz commented Mar 14, 2019 •

edited

Loading