ARROW-1280: [C++] add fixed size list type #4278

bkietz · 2019-05-08T17:14:19Z

FixedSizeListType, parameterized by the list size (similar to FixedSizeBinaryType), constructed with fixed_size_list(<value field or type>, list_size)
FixedSizeListArray, including Equal and pretty print impls
FixedSizeListBuilder, works with MakeBuilder

fsaintjacques

It think it's worth adding support in the integration tests as Java already support this type:

https://github.com/apache/arrow/blob/master/integration/integration_test.py

fsaintjacques · 2019-05-09T12:05:54Z

cpp/src/arrow/array.h

I think I prefer the explicit int64_t i as parameter. A bit better for the ABI.

Even though it's independent of index?

I would rather remove the argument. Or you can provide two separate overrides. A variadic function looks bizarre here.

Allowing it to be called with an argument means I don't have to rewrite pretty-print :)

fsaintjacques · 2019-05-09T12:06:13Z

cpp/src/arrow/array.h

is it worth having common abstract class?

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.

FixedSizeListScalar and ListScalar are identical but also trivial.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).

pitrou

Thank you. Overall this looks good. Still some comments below.

Side question: should support for FixedSizeListType added to ArrayFromJSON? Perhaps as a separate PR?

cpp/src/arrow/scalar.cc

cpp/src/arrow/pretty_print.cc

cpp/src/arrow/ipc/metadata-internal.cc

pitrou · 2019-05-09T17:53:53Z

cpp/src/arrow/ipc/json-internal.cc

You might add a simple test for this.

How's this: https://github.com/apache/arrow/pull/4278/files#diff-4e2d1d19217cc5481a51f3aa0b65b38eR193

pitrou · 2019-05-09T17:55:21Z

cpp/src/arrow/compare.cc

It would be nice if you could dig a bit (or perhaps open a JIRA about this, explaining the issue and/or adding a reproducer).

Confirmed, this is a bug.

cpp/src/arrow/array.cc

cpp/src/arrow/array.h

cpp/src/arrow/array.cc

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

pitrou · 2019-05-09T18:16:07Z

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

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

bkietz · 2019-05-09T20:57:27Z

@pitrou Added ArrayFromJSON impl

pitrou · 2019-05-13T12:03:24Z

@kou There is a meson-related error on Travis-CI, could you advise?
See https://travis-ci.org/apache/arrow/jobs/530907671

pitrou · 2019-05-13T12:10:02Z

Looks like there's also a MinGW test failure:
https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/24461942/job/mper2jipn6kj5bxj

Error: test: csv.gz(TableTest::#save and .load::path:::format::load: auto detect): Arrow::Error::Io: [csv-reader][read]: IOError: zlib inflate failed: invalid distance too far back
c:/Ruby26/lib/ruby/gems/2.6.0/gems/gobject-introspection-3.3.6/lib/gobject-introspection/loader.rb:616:in `invoke'
c:/Ruby26/lib/ruby/gems/2.6.0/gems/gobject-introspection-3.3.6/lib/gobject-introspection/loader.rb:616:in `invoke'
c:/Ruby26/lib/ruby/gems/2.6.0/gems/gobject-introspection-3.3.6/lib/gobject-introspection/loader.rb:533:in `block in define_method'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:158:in `block (2 levels) in load_from_path'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:147:in `block (2 levels) in wrap_input'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:140:in `open_encoding_convert_stream'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:146:in `block in wrap_input'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:125:in `block in open_decompress_input'
C:/projects/arrow/ruby/red-arrow/lib/arrow/block-closable.rb:25:in `open'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:124:in `open_decompress_input'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:145:in `wrap_input'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:157:in `block in load_from_path'
C:/projects/arrow/ruby/red-arrow/lib/arrow/block-closable.rb:25:in `open'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:156:in `load_from_path'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:39:in `load'
C:/projects/arrow/ruby/red-arrow/lib/arrow/csv-loader.rb:26:in `load'
C:/projects/arrow/ruby/red-arrow/lib/arrow/table-loader.rb:158:in `load_as_csv'
C:/projects/arrow/ruby/red-arrow/lib/arrow/table-loader.rb:50:in `load'
C:/projects/arrow/ruby/red-arrow/lib/arrow/table-loader.rb:22:in `load'
C:/projects/arrow/ruby/red-arrow/lib/arrow/table.rb:27:in `load'
C:/projects/arrow/ruby/red-arrow/test/test-table.rb:503:in `block (5 levels) in <class:TableTest>'

pitrou · 2019-05-13T12:41:52Z

Ok, apparently the Travis-CI failure is unrelated:
https://issues.apache.org/jira/browse/ARROW-5306

pitrou

LGTM.

pitrou · 2019-05-13T15:02:45Z

Turns out the MinGW failure was sporadic. Merging.

pitrou · 2019-05-13T15:04:10Z

Thanks @bkietz !

@fsaintjacques Integration tests are tracked in https://issues.apache.org/jira/browse/ARROW-1278.

kou · 2019-05-13T20:26:53Z

If the MinGW failure is caused again, I'll look into it.

Adds integration tests for fixed_size_list Also adds support for fixed_size_list to RecordBatchSerializer, which was omitted in #4278 Author: Benjamin Kietzman <bengilgit@gmail.com> Closes #4309 from bkietz/1278-integration-tests-for-fixed-size-list and squashes the following commits: 8b356f3 <Benjamin Kietzman> revert removal of ninja-build from dockerfile e7ed001 <Benjamin Kietzman> fix flake8 error 8ab4efc <Benjamin Kietzman> Adding integration tests for fixed_size_list

Adds integration tests for fixed_size_list Also adds support for fixed_size_list to RecordBatchSerializer, which was omitted in apache#4278 Author: Benjamin Kietzman <bengilgit@gmail.com> Closes apache#4309 from bkietz/1278-integration-tests-for-fixed-size-list and squashes the following commits: 8b356f3 <Benjamin Kietzman> revert removal of ninja-build from dockerfile e7ed001 <Benjamin Kietzman> fix flake8 error 8ab4efc <Benjamin Kietzman> Adding integration tests for fixed_size_list

fsaintjacques reviewed May 9, 2019

View reviewed changes

bkietz force-pushed the 1280-Implement-Fixed-Size-List-type branch from 45e309c to b47e059 Compare May 9, 2019 17:34

pitrou reviewed May 9, 2019

View reviewed changes

bkietz force-pushed the 1280-Implement-Fixed-Size-List-type branch from 78982ab to 80f444f Compare May 10, 2019 19:50

pitrou approved these changes May 13, 2019

View reviewed changes

bkietz added 8 commits May 13, 2019 15:24

add fixed size list type

f9258dd

removing unused offset buffer from constructor

dae1275

fixing some typos/copypasta

59ce758

Add ArrayFromJSON impl for fixed size list

3256051

Add pretty-print test for fixed size list

ddb46fd

Fix underline in Layout.rst

17b55c5

add test and missing impl for ipc/json+fixed_size_list

ca02bda

correct ListArray equality comparison

920a251

pitrou force-pushed the 1280-Implement-Fixed-Size-List-type branch from 80f444f to 920a251 Compare May 13, 2019 13:25

pitrou closed this in ea271b3 May 13, 2019

bkietz mentioned this pull request May 14, 2019

ARROW-1278: [Integration] Adding integration tests for fixed_size_list #4309

Closed

bkietz deleted the 1280-Implement-Fixed-Size-List-type branch February 25, 2021 16:10

asfimport mentioned this pull request May 15, 2019

[C++] Implement Fixed Size List type #17216

Closed

ARROW-1280: [C++] add fixed size list type #4278

ARROW-1280: [C++] add fixed size list type #4278

Uh oh!

Conversation

bkietz commented May 8, 2019

Uh oh!

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

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!

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!

bkietz commented May 9, 2019

Uh oh!

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

Uh oh!

Uh oh!

pitrou commented May 13, 2019

Uh oh!

pitrou commented May 13, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented May 13, 2019

Uh oh!

pitrou commented May 13, 2019

Uh oh!

kou commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitrou commented May 13, 2019 •

edited

Loading