ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664

Closed
pitrou wants to merge 6 commits into
apache:masterfrom
pitrou:ARROW-9265-metadata-version-unions
Closed

ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom
pitrou:ARROW-9265-metadata-version-unions

Conversation

@pitrou

@pitroupitrou commented Jul 7, 2020

Copy link
Copy Markdown
Member

V4 Union arrays with top-level null slots are disallowed, though.

@pitrou

pitrou commented Jul 7, 2020

Copy link
Copy Markdown
MemberAuthor

Note that apache/arrow-testing#35 needs to be merged first.

@github-actions

github-actionsBot commented Jul 7, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265

@pitrou
pitrouforce-pushed the ARROW-9265-metadata-version-unions branch from 71f42d4 to 9bb7d22CompareJuly 7, 2020 19:04
@lidavidm

lidavidm commented Jul 7, 2020

Copy link
Copy Markdown
Member

Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?

@pitrou

pitrou commented Jul 8, 2020

Copy link
Copy Markdown
MemberAuthor

That is right indeed.

@pitrou

pitrou commented Jul 8, 2020

Copy link
Copy Markdown
MemberAuthor

The ASAN/UBSAN Ci failure should be fixed when merging #7644.

@pitrou
pitrouforce-pushed the ARROW-9265-metadata-version-unions branch from 0dc65e8 to 84d1014CompareJuly 8, 2020 08:42
@wesm

wesm commented Jul 8, 2020

Copy link
Copy Markdown
Member

@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.

@pitrou

pitrou commented Jul 8, 2020

Copy link
Copy Markdown
MemberAuthor

I see, I will update the PR then.

@wesm

wesm commented Jul 8, 2020

Copy link
Copy Markdown
Member

I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.

@pitrou
pitrouforce-pushed the ARROW-9265-metadata-version-unions branch 3 times, most recently from 88dc2a1 to f73125aCompareJuly 8, 2020 14:14
@pitroupitrou mentioned this pull request Jul 8, 2020
Closed
@pitrou
pitrouforce-pushed the ARROW-9265-metadata-version-unions branch from f73125a to 75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou requested a review from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed the ARROW-9265-metadata-version-unions branch from e34d877 to 23c1a0eCompareJuly 9, 2020 09:59
@pitrou

pitrou commented Jul 9, 2020

Copy link
Copy Markdown
MemberAuthor

Rebased.

wesm
wesm approved these changes Jul 9, 2020
View reviewed changes

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

This looks good, there are a couple lines can be removed.

We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff

Comment threadcpp/src/arrow/ipc/file_to_stream.cc Outdated
Comment threadcpp/src/arrow/ipc/stream_to_file.cc Outdated
Comment threadcpp/src/arrow/testing/json_integration_test.cc Outdated
Comment threadpython/pyarrow/ipc.pxi Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?

@lidavidmlidavidmJul 9, 2020•
edited
Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?

@wesmwesmJul 9, 2020•
edited
Loading

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.

Sounds like it (Java does not check)

@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)

Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.

@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0

@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57

So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.

@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399

@wesm

wesm commented Jul 9, 2020

Copy link
Copy Markdown
Member

I'm quickly taking care of these small things so this can be merged

@wesm
wesmforce-pushed the ARROW-9265-metadata-version-unions branch from 23c1a0e to 838d1bbCompareJuly 9, 2020 20:48
@wesmwesm closed this in b3c8631Jul 9, 2020
@pitrou
pitrou deleted the ARROW-9265-metadata-version-unions branch July 9, 2020 22:15
@asfimportasfimport mentioned this pull request Jul 10, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@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-9265: [C++] Allow writing and reading V4-compliant IPC data - #7664
Closed
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Closed
ARROW-9265: [C++] Allow writing and reading V4-compliant IPC data#7664
pitrou wants to merge 6 commits into
apache:masterfrom 
pitrou:ARROW-9265-metadata-version-unions
Conversation
@pitrou
@pitroupitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
V4 Union arrays with top-level null slots are disallowed, though.
@pitrou
pitrou
commented
Jul 7, 2020

Copy link


Copy Markdown

MemberAuthor
Note that apache/arrow-testing#35 needs to be merged first.
@github-actions
github-actionsBot
commented
Jul 7, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9265
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
71f42d4 to
9bb7d22CompareJuly 7, 2020 19:04
@lidavidm
lidavidm
commented
Jul 7, 2020

Copy link


Copy Markdown

Member
Just a high level comment: if I'm reading this right, V4 is still the default metadata version and applications opt in to V5 when they want to read/write unions. Am I understanding this right?
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
That is right indeed.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
The ASAN/UBSAN Ci failure should be fixed when merging #7644.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
0dc65e8 to
84d1014CompareJuly 8, 2020 08:42
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
@lidavidm@pitrou I haven't looked at the patch yet, but V5 must be the default produced by writers but V4 can be opted in to.
@pitrou
pitrou
commented
Jul 8, 2020

Copy link


Copy Markdown

MemberAuthor
I see, I will update the PR then.
@wesm
wesm
commented
Jul 8, 2020

Copy link


Copy Markdown

Member
I just sent an e-mail to the ML. We don't want to continue producing V4 metadata unless we need to for forward compatibility reasons.
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
3 times, most recently
from
88dc2a1 to
f73125aCompareJuly 8, 2020 14:14
@pitroupitrou
mentioned this pull request
Jul 8, 2020
 Closed

@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
f73125a to
75bb873CompareJuly 8, 2020 18:19
@pitrou
pitrou
requested a review
from wesmJuly 9, 2020 09:56
@pitrou
pitrouforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
e34d877 to
23c1a0eCompareJuly 9, 2020 09:59
@pitrou
pitrou
commented
Jul 9, 2020

Copy link


Copy Markdown

MemberAuthor
Rebased.
wesm
wesm
approved these changes
Jul 9, 2020
View reviewed changes
@wesmwesm
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.

This looks good, there are a couple lines can be removed.
We need to expose the metadata version configuration along with an environment variable option to set the default to V4 (similar to what we did for the IPC alignment changes) in a separate PR before releasing. @BryanCutler can help validate that we have enough to allow e.g. Spark users to upgrade to 1.0.0 without breaking stuff
Comment threadcpp/src/arrow/ipc/file_to_stream.cc
Outdated

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

Comment threadcpp/src/arrow/testing/json_integration_test.cc
Outdated

Comment threadpython/pyarrow/ipc.pxi
Outdated

@wesmwesmJul 9, 2020

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.

Out of curiosity, where will we break (presumably earlier than this) if we were to encounter an unrecognized version?
@lidavidmlidavidmJul 9, 2020•

edited

Loading

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.

The fact that integration still passes in the corresponding Java PR #7685 (before rebasing on top of this) would imply that the version doesn't get checked, no?
@wesmwesmJul 9, 2020•

edited

Loading

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.

Sounds like it (Java does not check)
@lidavidmlidavidmJul 10, 2020

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.

Well, neither checks, because #7685 passes (Java sending V5 while C++ sends V4) and this passed (Java sends V4 while C++ sends V5)
Though that may be reassuring to anyone planning to use 0.17.1 with 1.0.0.
@wesmwesmJul 10, 2020

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.

Yikes, well I will open a JIRA about adding appropriate checks at least for 1.0.0
@wesmwesmJul 10, 2020

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.

See https://github.com/apache/arrow/blob/maint-0.17.x/cpp/src/arrow/ipc/message.cc#L57
So it only checks for old versions but new versions pass silently, which is quite scary to me. But on the other hand the risk of V5 data breaking a V4 application (e.g. Spark) at the moment is low.
@wesmwesmJul 10, 2020

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.

https://issues.apache.org/jira/browse/ARROW-9399
@wesm
wesm
commented
Jul 9, 2020

Copy link


Copy Markdown

Member
I'm quickly taking care of these small things so this can be merged
@wesm
wesmforce-pushed
the
ARROW-9265-metadata-version-unions
branch
from
23c1a0e to
838d1bbCompareJuly 9, 2020 20:48
@wesmwesm
closed this
in
b3c8631Jul 9, 2020
@pitrou
pitrou
deleted the

ARROW-9265-metadata-version-unions

branch
July 9, 2020 22:15
@asfimportasfimport
mentioned this pull request
Jul 10, 2020

Closed

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

Reviewers

@lidavidmlidavidmlidavidm left review comments
@wesmwesmwesm 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.

3 participants

@pitrou@lidavidm@wesm