ARROW-4810: [Format] [C++] Add LargeList type by pitrou · Pull Request #4969 · apache/arrow

pitrou · 2019-07-30T15:48:27Z

No description provided.

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

@emkornfield

emkornfield · 2019-07-30T18:26:19Z

cpp/src/arrow/array.cc

You might want to check someplace that the offset buffer has the expected size?

It's done in ValidateArray (well, it checks it has at least the required size)

emkornfield · 2019-07-30T18:26:47Z

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

why this change?

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.

emkornfield · 2019-07-30T18:41:18Z

cpp/src/arrow/array.cc

ListArrayType?

That implies it's an Array type. I can call it TypeClass if you want.

I think it is OK for now.

emkornfield · 2019-07-30T18:54:31Z

cpp/src/arrow/extension_type.h

this change seems unrelated?

No, I added type_name() to all instantiable type classes. The motivation is to allow producing nice error messages even when you don't have a DataType instance, only class (for example in a concrete type-templated function).

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

emkornfield · 2019-07-30T19:02:51Z

cpp/src/arrow/type.h

Could this limit people that want to extend the code base in the future?

I don't know, why would it?

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.

I don't know, it seems the non-trivial logic ends up in ToString() by design.
As for the rationale behind name(), perhaps @wesm or @xhochy know about the history?

I don't think the name() function is important. It's been around since the beginning of the project but I don't see it used much

format/Schema.fbs

codecov-io · 2019-07-30T19:10:45Z

Codecov Report

Merging #4969 into master will decrease coverage by 0.41%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #4969      +/-   ##
==========================================
- Coverage   87.98%   87.56%   -0.42%     
==========================================
  Files         910     1000      +90     
  Lines      133521   142570    +9049     
  Branches     1418     1418              
==========================================
+ Hits       117483   124848    +7365     
- Misses      16028    17360    +1332     
- Partials       10      362     +352

Impacted Files	Coverage Δ
cpp/src/arrow/extension_type.cc	`95.45% <ø> (+2.12%)`	⬆️
cpp/src/arrow/array/builder_nested.cc	`90.55% <ø> (-2.12%)`	⬇️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <ø> (ø)`	⬆️
cpp/src/arrow/type_traits.h	`86.2% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.cc	`96.53% <0%> (-0.19%)`	⬇️
cpp/src/arrow/scalar.cc	`63.33% <0%> (ø)`	⬆️
cpp/src/arrow/scalar.h	`87.5% <0%> (ø)`	⬆️
cpp/src/arrow/extension_type.h	`85.71% <0%> (-14.29%)`	⬇️
... and 122 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 cac4957...a1d93b2. Read the comment docs.

emkornfield · 2019-07-30T19:16:43Z

Note to self, old proof of concept for java 03956ca

fsaintjacques

Bravo.

cpp/src/arrow/type_traits.h

pitrou · 2019-07-31T12:56:05Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521

emkornfield · 2019-08-01T03:34:01Z

+1, thank you

pitrou mentioned this pull request Jul 30, 2019

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

Closed

pitrou force-pushed the ARROW-4810-large-list branch 2 times, most recently from 87bb90a to ca91e2f Compare July 30, 2019 16:57

emkornfield self-requested a review July 30, 2019 18:22

emkornfield requested changes Jul 30, 2019

View reviewed changes

fsaintjacques approved these changes Jul 30, 2019

View reviewed changes

cpp/src/arrow/type_traits.h Outdated Show resolved Hide resolved

ARROW-4810: [Format] [C++] Add LargeList type

a1d93b2

pitrou force-pushed the ARROW-4810-large-list branch from ca91e2f to a1d93b2 Compare July 31, 2019 11:40

emkornfield closed this in 8cdf567 Aug 1, 2019

pitrou deleted the ARROW-4810-large-list branch August 1, 2019 08:26

asfimport mentioned this pull request Aug 1, 2019

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

Closed

Conversation

pitrou commented Jul 30, 2019

Uh oh!

pitrou commented Jul 30, 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!

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!

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 Jul 31, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

emkornfield commented Jul 30, 2019

Uh oh!

fsaintjacques left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Jul 31, 2019

Uh oh!

emkornfield commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou Jul 31, 2019 •

edited

Loading

codecov-io commented Jul 30, 2019 •

edited

Loading