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

Closed
pitrou wants to merge 1 commit into
apache:masterfrom
pitrou:ARROW-4810-large-list
Closed

ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom
pitrou:ARROW-4810-large-list

Conversation

@pitrou

@pitroupitrou commented Jul 30, 2019

Copy link
Copy Markdown
Member

No description provided.

@pitroupitrou mentioned this pull request Jul 30, 2019
Closed
@pitrou

pitrou commented Jul 30, 2019

Copy link
Copy Markdown
MemberAuthor

@emkornfield

@pitrou
pitrouforce-pushed the ARROW-4810-large-list branch 2 times, most recently from 87bb90a to ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield self-requested a review July 30, 2019 18:22
emkornfield
emkornfield requested changes Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitroupitrouJul 30, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadcpp/src/arrow/array-list-test.cc Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

@pitroupitrouJul 30, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadcpp/src/arrow/array.cc Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?

@pitroupitrouJul 30, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@emkornfieldemkornfieldJul 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.

Comment threadcpp/src/arrow/extension_type.h Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?

@pitroupitrouJul 30, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment threadcpp/src/arrow/ipc/json-internal.cc Outdated
Comment threadcpp/src/arrow/type.h Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitroupitrouJul 30, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?

@emkornfieldemkornfieldJul 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitroupitrouJul 31, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@wesmwesmJul 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadformat/Schema.fbs Outdated
@codecov-io

codecov-io commented Jul 30, 2019•
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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

emkornfield commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

fsaintjacques
fsaintjacques approved these changes Jul 30, 2019
View reviewed changes

@fsaintjacquesfsaintjacques left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.

Comment threadcpp/src/arrow/type_traits.h Outdated
@pitrou
pitrouforce-pushed the ARROW-4810-large-list branch from ca91e2f to a1d93b2CompareJuly 31, 2019 11:40
@pitrou

pitrou commented Jul 31, 2019

Copy link
Copy Markdown
MemberAuthor

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

@emkornfield

emkornfield commented Aug 1, 2019

Copy link
Copy Markdown
Contributor

+1, thank you

@pitrou
pitrou deleted the ARROW-4810-large-list branch August 1, 2019 08:26
@asfimportasfimport mentioned this pull request Aug 1, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
ARROW-4810: [Format] [C++] Add LargeList type - #4969
Closed
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Closed
ARROW-4810: [Format] [C++] Add LargeList type#4969
pitrou wants to merge 1 commit into
apache:masterfrom 
pitrou:ARROW-4810-large-list
Conversation
@pitrou
@pitroupitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

Member
 No description provided. 
@pitroupitrou
mentioned this pull request
Jul 30, 2019
 Closed

@pitrou
pitrou
commented
Jul 30, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield
@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
2 times, most recently
from
87bb90a to
ca91e2fCompareJuly 30, 2019 16:57
@emkornfield
emkornfield
self-requested a review
July 30, 2019 18:22
emkornfield
emkornfield
requested changes
Jul 30, 2019
View reviewed changes
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to check someplace that the offset buffer has the expected size?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done in ValidateArray (well, it checks it has at least the required size)
Comment threadcpp/src/arrow/array-list-test.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing a list type with int32 values while offsets are also int32 is not gonna catch some possible sources of errors.
Comment threadcpp/src/arrow/array.cc
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListArrayType?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That implies it's an Array type. I can call it TypeClass if you want.
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK for now.
Comment threadcpp/src/arrow/extension_type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems unrelated?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Comment threadcpp/src/arrow/ipc/json-internal.cc
Outdated

Comment threadcpp/src/arrow/type.h
Outdated

@emkornfieldemkornfieldJul 30, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this limit people that want to extend the code base in the future?
@pitroupitrouJul 30, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, why would it?
@emkornfieldemkornfieldJul 31, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name() actually had to contain non-trivial logic? I wonder why it had this signature to begin with.
@pitroupitrouJul 31, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@wesmwesmJul 31, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadformat/Schema.fbs
Outdated

@codecov-io
codecov-io
commented
Jul 30, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4969 into master will decrease coverage by 0.41%.

The diff coverage is 95.83%.
Impacted file tree graph
@@ 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 FilesCoverage Δ
cpp/src/arrow/extension_type.cc95.45% <ø> (+2.12%)⬆️
cpp/src/arrow/array/builder_nested.cc90.55% <ø> (-2.12%)⬇️
cpp/src/arrow/visitor.h50% <ø> (ø)⬆️
cpp/src/arrow/pretty_print.cc83.33% <ø> (ø)⬆️
cpp/src/arrow/visitor_inline.h88.78% <ø> (ø)⬆️
cpp/src/arrow/type_traits.h86.2% <ø> (ø)⬆️
cpp/src/parquet/arrow/writer.cc96.53% <0%> (-0.19%)⬇️
cpp/src/arrow/scalar.cc63.33% <0%> (ø)⬆️
cpp/src/arrow/scalar.h87.5% <0%> (ø)⬆️
cpp/src/arrow/extension_type.h85.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
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
Note to self, old proof of concept for java 03956ca
fsaintjacques
fsaintjacques
approved these changes
Jul 30, 2019
View reviewed changes
@fsaintjacquesfsaintjacques
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo.
Comment threadcpp/src/arrow/type_traits.h
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-4810-large-list
branch
from
ca91e2f to
a1d93b2CompareJuly 31, 2019 11:40
@pitrou
pitrou
commented
Jul 31, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565932521
@emkornfield
emkornfield
commented
Aug 1, 2019

Copy link


Copy Markdown

Contributor
+1, thank you
@pitrou
pitrou
deleted the

ARROW-4810-large-list

branch
August 1, 2019 08:26
@asfimportasfimport
mentioned this pull request
Aug 1, 2019

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@fsaintjacquesfsaintjacquesfsaintjacques approved these changes
@emkornfieldemkornfieldemkornfield requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@codecov-io@emkornfield@wesm@fsaintjacques