ARROW-750: [Format] [C++] Add LargeBinary and LargeString types - #4921

Closed
pitrou wants to merge 2 commits into
apache:masterfrom
pitrou:ARROW-750-large-binary-2
Closed

ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom
pitrou:ARROW-750-large-binary-2

Conversation

@pitrou

@pitroupitrou commented Jul 22, 2019

Copy link
Copy Markdown
Member

These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.

@pitroupitrou mentioned this pull request Jul 22, 2019
Closed
@pitrou
pitrouforce-pushed the ARROW-750-large-binary-2 branch from b32e7c9 to e6418b0CompareJuly 22, 2019 19:50
@emkornfield

emkornfield commented Jul 23, 2019

Copy link
Copy Markdown
Contributor

@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?

@pitrou

pitrou commented Jul 23, 2019

Copy link
Copy Markdown
MemberAuthor

@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.

@pitrou

pitrou commented Jul 23, 2019

Copy link
Copy Markdown
MemberAuthor

Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.

@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)

@pitrou
pitrouforce-pushed the ARROW-750-large-binary-2 branch from e6418b0 to daa3802CompareJuly 23, 2019 10:21
@emkornfield

emkornfield commented Jul 23, 2019

Copy link
Copy Markdown
Contributor

I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.

The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.

@pitrou

pitrou commented Jul 23, 2019

Copy link
Copy Markdown
MemberAuthor

I don't know. I thought Java was int64-challenged? @wesm

@emkornfield

emkornfield commented Jul 23, 2019 via email

Copy link
Copy Markdown
Contributor

@codecov-io

codecov-io commented Jul 23, 2019•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #4921 into master will decrease coverage by 13.24%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.

emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?

@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?
Also note this is mostly moving method definitions around, there is little to no new code in this file.

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?

@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.

emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield reviewed Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield requested changes Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).

@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.

@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?

anyways for a separate PR.

@pitroupitrouJul 26, 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's possible. Can you open a JIRA?

Comment threadcpp/src/arrow/array/builder_binary.h Outdated
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
Comment threadcpp/src/arrow/array/builder_binary.h Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?

@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.

@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>

Comment threadcpp/src/arrow/array/builder_binary.h Outdated
Comment threadcpp/src/arrow/compare.cc Outdated
Comment threadcpp/src/arrow/compare.cc Outdated
Comment threadcpp/src/arrow/ipc/feather.cc Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?

@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?

@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?

@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.

Comment threadformat/Schema.fbs Outdated
Comment threadformat/Schema.fbs Outdated
@pitrou
pitrouforce-pushed the ARROW-750-large-binary-2 branch from daa3802 to bd59891CompareJuly 24, 2019 10:19
@pitrou

pitrou commented Jul 25, 2019

Copy link
Copy Markdown
MemberAuthor

@emkornfield You may want to review again.

@emkornfield

emkornfield commented Jul 25, 2019 via email

Copy link
Copy Markdown
Contributor

emkornfield
emkornfield approved these changes Jul 26, 2019
View reviewed changes

@emkornfieldemkornfield 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.

I think at this point my comments are mostly nits or things to follow-up.

The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)

Comment threadcpp/src/arrow/array-binary-test.cc Outdated
Comment threadcpp/src/arrow/array-binary-test.cc Outdated
Comment threadcpp/src/arrow/array/builder_binary.h Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?

Comment threadcpp/src/arrow/array/builder_binary.h Outdated
Comment threadcpp/src/arrow/array/builder_binary.h Outdated
emkornfield
emkornfield requested changes Jul 26, 2019
View reviewed changes

@emkornfieldemkornfield 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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.

@pitrou

pitrou commented Jul 26, 2019

Copy link
Copy Markdown
MemberAuthor

I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.

@emkornfield

emkornfield commented Jul 26, 2019

Copy link
Copy Markdown
Contributor

As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.

Maybe be worth sending something to the ML?

@pitrou

pitrou commented Jul 26, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Maybe be worth sending something to the ML?

Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.

@wesm Thoughts about the process here?

@wesm

wesm commented Jul 26, 2019

Copy link
Copy Markdown
Member

I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing

pitrou added 2 commits July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed the ARROW-750-large-binary-2 branch from bd59891 to 3e17dcdCompareJuly 29, 2019 09:44
@pitrou

pitrou commented Jul 29, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I rebased and addressed most of the review comments. Also (re-)added some tests.

@pitrou

pitrou commented Jul 29, 2019

Copy link
Copy Markdown
MemberAuthor

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

@emkornfield

emkornfield commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).

@emkornfield

emkornfield commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

I might be more randomized this week than I expected but still think a Java implementation should be doable.

@pitrou
pitrou deleted the ARROW-750-large-binary-2 branch July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm
, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm

, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm

, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm

, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm

, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm

, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm

, '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-750: [Format] [C++] Add LargeBinary and LargeString types - #4921
Closed
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Closed
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types#4921
pitrou wants to merge 2 commits into
apache:masterfrom 
pitrou:ARROW-750-large-binary-2
Conversation
@pitrou
@pitroupitrou
commented
Jul 22, 2019

Copy link


Copy Markdown

Member
These are like Binary and String respectively, except with 64-bit offsets

so as to allow extremely large individual values.
@pitroupitrou
mentioned this pull request
Jul 22, 2019
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
b32e7c9 to
e6418b0CompareJuly 22, 2019 19:50
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
@pitrou were you planning on adding the java implementation as well? Or were you looking for someone to collaborate on that?
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield I need someone else to do the Java implementation. Also I think that can be in a separate PR.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
Note this was more or less reviewed in #4919, but the review comments seem to have been lost in the post-release merge hiccups.
@emkornfield would you like to do a final review of this? I'd like to move on rather quickly if possible :-)
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
e6418b0 to
daa3802CompareJuly 23, 2019 10:21
@emkornfield
emkornfield
commented
Jul 23, 2019

Copy link


Copy Markdown

Contributor
I can review tonight if no one gets to it sooner. I can also help with the Java implementation of this if there are no other volunteers, but probably won't be able to that until next week (I had a poc commit for large lists). If this is really high priority I can try to rearrange some things to do more work this week.
The last email thread on new types seemed to indicate we want the two implementation checked in at the same time.
@pitrou
pitrou
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
I don't know. I thought Java was int64-challenged? @wesm
@emkornfield
emkornfield
commented
Jul 23, 2019
via email 

Copy link


Copy Markdown

Contributor
@codecov-io
codecov-io
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #4921 into master will decrease coverage by 13.24%.

The diff coverage is n/a.
Impacted file tree graph
@@ Coverage Diff @@## master #4921 +/- ##
===========================================
- Coverage 88.51% 75.27% -13.25% 
===========================================
Files 908 57 -851 Lines 115478 3713 -111765 Branches 1418 0 -1418 ===========================================
- Hits 102218 2795 -99423 + Misses 12898 918 -11980 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/src/csv.cpp100% <0%> (ø)⬆️
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
... and 809 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 ff8c63b...bd59891. Read the comment docs.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

should this be inside the valid_bytes value?
@pitroupitrouJul 24, 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 understand what you mean. Can you elaborate?

Also note this is mostly moving method definitions around, there is little to no new code in this file.
@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
@pitroupitrouJul 29, 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.

Wouldn't that be incorrect? You need to build the null bitmap somehow.
emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
reviewed
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 24, 2019
View reviewed changes
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

not for this change, but I am kind of curious why we made zero out in buffer builder always versus only zero-ing for the bitmap (which I think is the only thing that requires it).
@pitroupitrouJul 24, 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 really know... Ideally zeroing the padding should be cheap operation, but perhaps not if you resize often.
@emkornfieldemkornfieldJul 26, 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 would have to look I think when we resize we pad everything from size to capacity. If capacity doubles size each time, it seems like it is effectively an O(N) cost?
anyways for a separate PR.
@pitroupitrouJul 26, 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's possible. Can you open a JIRA?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 24, 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.

can we use TypedBufferBuilder instead?
@pitroupitrouJul 24, 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 really see the point. It's a preallocated temporary container.
@emkornfieldemkornfieldJul 26, 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.

The main rationale, I had for this it is theoretically "data" and not metadata. For data related allocations I think we should be consistently be trying to use memory backed structures so users get a consistent OOM experience. Not necessary for this PR>
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/compare.cc
Outdated

Comment threadcpp/src/arrow/ipc/feather.cc
Outdated

@emkornfieldemkornfieldJul 24, 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.

Would it make sense to push this method to ArrayType?
@pitroupitrouJul 24, 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 fail to find a good, non-confusing name for it... Any idea?
@emkornfieldemkornfieldJul 26, 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.

CalculateOffsetBufferLength?
@pitroupitrouJul 29, 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.

The offsets buffer could be larger, e.g. if the array is sliced.
Comment threadformat/Schema.fbs
Outdated

Comment threadformat/Schema.fbs
Outdated

@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
daa3802 to
bd59891CompareJuly 24, 2019 10:19
@pitrou
pitrou
commented
Jul 25, 2019

Copy link


Copy Markdown

MemberAuthor
@emkornfield You may want to review again.
@emkornfield
emkornfield
commented
Jul 25, 2019
via email 

Copy link


Copy Markdown

Contributor
emkornfield
emkornfield
approved these changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

I think at this point my comments are mostly nits or things to follow-up.
The one remaining question on if we should check this in or wait for the Java implementation (like I said, I think I should be able to have something next week)
Comment threadcpp/src/arrow/array-binary-test.cc
Outdated

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

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

@emkornfieldemkornfieldJul 26, 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.

In the if block above on line 135?
Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

Comment threadcpp/src/arrow/array/builder_binary.h
Outdated

emkornfield
emkornfield
requested changes
Jul 26, 2019
View reviewed changes
@emkornfieldemkornfield
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.

Change to request changes just so we don't hastily merge, but per my previous comment, I think the main question now is resolving the Java side.
@pitrou
pitrou
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
I'll take a look at most comments on Monday. As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
@emkornfield
emkornfield
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
As for the Java side, though, I'd rather not wait for it as #4927 also pends on this PR.
Maybe be worth sending something to the ML?
@pitrou
pitrou
commented
Jul 26, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Maybe be worth sending something to the ML?
Is that necessary? Since you're planning to do the Java soon, it won't be long until there are two independent implementations of this feature.
@wesm Thoughts about the process here?
@wesm
wesm
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
I'll write an e-mail to the ML about it now. We definitely should not release again without the other implementation landing
pitrou
added 2 commits
July 29, 2019 11:25
@pitrou
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types
9a29953
These are like Binary and String respectively, except with 64-bit offsets
so as to allow extremely large individual values.
@pitrou
Address review comments
3e17dcd
@pitrou
pitrouforce-pushed
the
ARROW-750-large-binary-2
branch
from
bd59891 to
3e17dcdCompareJuly 29, 2019 09:44
@pitrou
pitrou
commented
Jul 29, 2019•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I rebased and addressed most of the review comments. Also (re-)added some tests.
@pitrou
pitrou
commented
Jul 29, 2019

Copy link


Copy Markdown

MemberAuthor
Travis-CI: https://travis-ci.org/pitrou/arrow/builds/564894917
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
+1, LGTM. (Feel free to merge, otherwise I'll do so in the morning to give a little bit more time to see if anyone responds to @wesm's e-mail).
@emkornfield
emkornfield
commented
Jul 30, 2019

Copy link


Copy Markdown

Contributor
I might be more randomized this week than I expected but still think a Java implementation should be doable.
@pitrou
pitrou
deleted the

ARROW-750-large-binary-2

branch
July 30, 2019 07:11
@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

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

4 participants

@pitrou@emkornfield@codecov-io@wesm