ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361

Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom
wesm:two-part-eos
Closed

ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom
wesm:two-part-eos

Conversation

@wesm

@wesmwesm commented Sep 11, 2019

Copy link
Copy Markdown
Member

No description provided.

eerhardt
eerhardt approved these changes Sep 11, 2019
View reviewed changes

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

Looks good to me.

@wesm

wesm commented Sep 12, 2019

Copy link
Copy Markdown
MemberAuthor

Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build

@wesm

wesm commented Sep 12, 2019

Copy link
Copy Markdown
MemberAuthor

I can repro, not sure why it occurs in Python 2.7 but not 3.6/7

@wesm

wesm commented Sep 12, 2019

Copy link
Copy Markdown
MemberAuthor

@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)

BryanCutler
BryanCutler approved these changes Sep 12, 2019
View reviewed changes

@BryanCutlerBryanCutler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

emkornfield
emkornfield reviewed Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;

@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.

@bkietzbkietzSep 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare

externconstexprint32_tkIpcContinuationToken = -1;

... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.

emkornfield
emkornfield approved these changes Sep 12, 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.

Looks OK to me, feel free to merge when build passes.

@pitrou

pitrou commented Sep 12, 2019

Copy link
Copy Markdown
Member

I found that Python 2.7 wasn't returning aligned memory from io.BytesIO

Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.

bkietz
bkietz reviewed Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
return Write(&kEos, sizeof(kEos));
constexpr int32_t kZeroLength = 0;
if (!options_.write_legacy_ipc_format) {
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));

@bkietzbkietzSep 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));

@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect

wesm added a commit that referenced this pull request Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm closed this Sep 12, 2019
wesm added a commit that referenced this pull request Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit to pprudhvi/arrow that referenced this pull request Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport mentioned this pull request Sep 12, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield
, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield

, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield

, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield

, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield

, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield

, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield

, '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-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation - #5361
Closed
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Closed
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, update C++ implementation#5361
wesm wants to merge 3 commits into
apache:ARROW-6313-flatbuffer-alignmentfrom 
wesm:two-part-eos
Conversation
@wesm
@wesmwesm
commented
Sep 11, 2019

Copy link


Copy Markdown

Member
 No description provided. 
eerhardt
eerhardt
approved these changes
Sep 11, 2019
View reviewed changes
@eerhardteerhardt
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.

Looks good to me.
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
Trying to reproduce the failure that's occurring in Ursabot Python 2.7 build
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
I can repro, not sure why it occurs in Python 2.7 but not 3.6/7
@wesm
wesm
commented
Sep 12, 2019

Copy link


Copy Markdown

MemberAuthor
@pitrou@emkornfield I found that Python 2.7 wasn't returning aligned memory from io.BytesIO and so I am making the metadata read path copy the metadata unconditionally if it is unaligned (even if the IPC format is implemented properly)
BryanCutler
BryanCutler
approved these changes
Sep 12, 2019
View reviewed changes
@BryanCutlerBryanCutler
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
emkornfield
emkornfield
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/metadata_internal.h
namespace internal {

// This 0xFFFFFFFF value is the first 4 bytes of a valid IPC message
constexpr int32_t kIpcContinuationToken = -1;
@emkornfieldemkornfieldSep 12, 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.

for some reason i thought there were linkage issues in C++11 using constexpr in a header.
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

constexpr implies inline/static linkage. It's not an error (though I sort of expected it to be) in clang to declare
externconstexprint32_tkIpcContinuationToken = -1;
... if you want to declare something constexpr within this translation unit but also export it as a symbol for other translation units, but the expectation is that you'll just include the header into whatever TU needs that constant.
emkornfield
emkornfield
approved these changes
Sep 12, 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.

Looks OK to me, feel free to merge when build passes.
@pitrou
pitrou
commented
Sep 12, 2019

Copy link


Copy Markdown

Member
I found that Python 2.7 wasn't returning aligned memory from io.BytesIO
Interesting. Python 3 doesn't make any such guarantee either, but probably it turns out ok in the current bytes object implementation.
bkietz
bkietz
reviewed
Sep 12, 2019
View reviewed changes
Comment threadcpp/src/arrow/ipc/writer.cc
 return Write(&kEos, sizeof(kEos));
 constexpr int32_t kZeroLength = 0;
 if (!options_.write_legacy_ipc_format) {
 RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
@bkietzbkietzSep 12, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit:

Suggested change

RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(int32_t)));
RETURN_NOT_OK(Write(&internal::kIpcContinuationToken, sizeof(internal::kIpcContinuationToken)));
@wesmwesmSep 12, 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'm going to let this one slide -- I kind of prefer int32_t for the documentation aspect
wesm added a commit
that referenced
this pull request
Sep 12, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
d17ac7d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@wesmwesm
closed this
Sep 12, 2019
wesm added a commit
that referenced
this pull request
Sep 13, 2019
@wesm
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
7f4d50d
…e C++ implementation
Closes#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
pprudhvi pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
@wesm@pprudhvi
ARROW-6541: [Format][C++] Update Columnar.rst for two-part EOS, updat…
955d7ed
…e C++ implementation
Closesapache#5361 from wesm/two-part-eos and squashes the following commits:
cd632ff <Wes McKinney> Copy metadata unconditionally since input buffers may not start on aligned offsets
70d1b3a <Wes McKinney> Remove unneeded header
4071528 <Wes McKinney> Use 2-part EOS in C++, update in format docs. Write old EOS if option is set
Authored-by: Wes McKinney <wesm+git@apache.org>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
@asfimportasfimport
mentioned this pull request
Sep 12, 2019

Closed

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

Reviewers

@bkietzbkietzbkietz left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@eerhardteerhardteerhardt approved these changes
@emkornfieldemkornfieldemkornfield approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@wesm@pitrou@bkietz@BryanCutler@eerhardt@emkornfield