Skip to content

GH-34059: [C++] Add a fetch node based on a batch index - #34060

Merged
westonpace merged 8 commits into
apache:masterfrom
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged

GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom
westonpace:feature/GH-34059--add-fetch-node

Conversation

@westonpace

@westonpacewestonpace commented Feb 7, 2023•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).

This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.

This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.

So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).

This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.

@github-actions

github-actionsBot commented Feb 7, 2023

Copy link
Copy Markdown

@github-actions

github-actionsBot commented Feb 7, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.

lidavidm
lidavidm reviewed Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h Outdated
Comment threadcpp/src/arrow/compute/exec/exec_plan.h Outdated
Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h Outdated
Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc Outdated
Comment threadcpp/src/arrow/testing/generator.h Outdated
Comment threadcpp/src/arrow/compute/exec/fetch_node.cc Outdated
@westonpace
westonpaceforce-pushed the feature/GH-34059--add-fetch-node branch from e8a716b to e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace merged commit b056e07 into apache:masterFeb 11, 2023
@ursabot

ursabot commented Feb 11, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm
[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] b056e07b ec2-t3-xlarge-us-east-2
[Failed] b056e07b test-mac-arm
[Finished] b056e07b ursa-i9-9960x
[Finished] b056e07b ursa-thinkcentre-m75q
[Finished] 24e5a580 ec2-t3-xlarge-us-east-2
[Failed] 24e5a580 test-mac-arm
[Finished] 24e5a580 ursa-i9-9960x
[Finished] 24e5a580 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented Feb 11, 2023

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

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

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property

3 participants

@westonpace@ursabot@lidavidm
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
GH-34059: [C++] Add a fetch node based on a batch index by westonpace · Pull Request #34060 · apache/arrow · GitHub
Skip to content
GH-34059: [C++] Add a fetch node based on a batch index - #34060
Merged
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged
GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Conversation
@westonpace
@westonpacewestonpace
commented
Feb 7, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).
This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.
This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.
So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).
This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.
@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.
lidavidm
lidavidm
reviewed
Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h
Outdated

Comment threadcpp/src/arrow/compute/exec/exec_plan.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc
Outdated

Comment threadcpp/src/arrow/testing/generator.h
Outdated

Comment threadcpp/src/arrow/compute/exec/fetch_node.cc
Outdated

@westonpace
westonpaceforce-pushed
the
feature/GH-34059--add-fetch-node
branch
from
e8a716b to
e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace
merged commit b056e07
into
apache:masterFeb 11, 2023
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] b056e07b ec2-t3-xlarge-us-east-2

[Failed] b056e07b test-mac-arm

[Finished] b056e07b ursa-i9-9960x

[Finished] b056e07b ursa-thinkcentre-m75q

[Finished] 24e5a580 ec2-t3-xlarge-us-east-2

[Failed] 24e5a580 test-mac-arm

[Finished] 24e5a580 ursa-i9-9960x

[Finished] 24e5a580 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property


3 participants

@westonpace@ursabot@lidavidm

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34059: [C++] Add a fetch node based on a batch index by westonpace · Pull Request #34060 · apache/arrow · GitHub
Skip to content
GH-34059: [C++] Add a fetch node based on a batch index - #34060
Merged
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged
GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Conversation
@westonpace
@westonpacewestonpace
commented
Feb 7, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).
This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.
This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.
So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).
This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.
@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.
lidavidm
lidavidm
reviewed
Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h
Outdated

Comment threadcpp/src/arrow/compute/exec/exec_plan.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc
Outdated

Comment threadcpp/src/arrow/testing/generator.h
Outdated

Comment threadcpp/src/arrow/compute/exec/fetch_node.cc
Outdated

@westonpace
westonpaceforce-pushed
the
feature/GH-34059--add-fetch-node
branch
from
e8a716b to
e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace
merged commit b056e07
into
apache:masterFeb 11, 2023
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] b056e07b ec2-t3-xlarge-us-east-2

[Failed] b056e07b test-mac-arm

[Finished] b056e07b ursa-i9-9960x

[Finished] b056e07b ursa-thinkcentre-m75q

[Finished] 24e5a580 ec2-t3-xlarge-us-east-2

[Failed] 24e5a580 test-mac-arm

[Finished] 24e5a580 ursa-i9-9960x

[Finished] 24e5a580 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property


3 participants

@westonpace@ursabot@lidavidm

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34059: [C++] Add a fetch node based on a batch index by westonpace · Pull Request #34060 · apache/arrow · GitHub
Skip to content
GH-34059: [C++] Add a fetch node based on a batch index - #34060
Merged
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged
GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Conversation
@westonpace
@westonpacewestonpace
commented
Feb 7, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).
This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.
This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.
So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).
This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.
@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.
lidavidm
lidavidm
reviewed
Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h
Outdated

Comment threadcpp/src/arrow/compute/exec/exec_plan.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc
Outdated

Comment threadcpp/src/arrow/testing/generator.h
Outdated

Comment threadcpp/src/arrow/compute/exec/fetch_node.cc
Outdated

@westonpace
westonpaceforce-pushed
the
feature/GH-34059--add-fetch-node
branch
from
e8a716b to
e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace
merged commit b056e07
into
apache:masterFeb 11, 2023
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] b056e07b ec2-t3-xlarge-us-east-2

[Failed] b056e07b test-mac-arm

[Finished] b056e07b ursa-i9-9960x

[Finished] b056e07b ursa-thinkcentre-m75q

[Finished] 24e5a580 ec2-t3-xlarge-us-east-2

[Failed] 24e5a580 test-mac-arm

[Finished] 24e5a580 ursa-i9-9960x

[Finished] 24e5a580 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property


3 participants

@westonpace@ursabot@lidavidm

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
GH-34059: [C++] Add a fetch node based on a batch index by westonpace · Pull Request #34060 · apache/arrow · GitHub
Skip to content
GH-34059: [C++] Add a fetch node based on a batch index - #34060
Merged
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged
GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Conversation
@westonpace
@westonpacewestonpace
commented
Feb 7, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).
This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.
This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.
So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).
This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.
@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.
lidavidm
lidavidm
reviewed
Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h
Outdated

Comment threadcpp/src/arrow/compute/exec/exec_plan.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc
Outdated

Comment threadcpp/src/arrow/testing/generator.h
Outdated

Comment threadcpp/src/arrow/compute/exec/fetch_node.cc
Outdated

@westonpace
westonpaceforce-pushed
the
feature/GH-34059--add-fetch-node
branch
from
e8a716b to
e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace
merged commit b056e07
into
apache:masterFeb 11, 2023
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] b056e07b ec2-t3-xlarge-us-east-2

[Failed] b056e07b test-mac-arm

[Finished] b056e07b ursa-i9-9960x

[Finished] b056e07b ursa-thinkcentre-m75q

[Finished] 24e5a580 ec2-t3-xlarge-us-east-2

[Failed] 24e5a580 test-mac-arm

[Finished] 24e5a580 ursa-i9-9960x

[Finished] 24e5a580 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property


3 participants

@westonpace@ursabot@lidavidm

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34059: [C++] Add a fetch node based on a batch index by westonpace · Pull Request #34060 · apache/arrow · GitHub
Skip to content
GH-34059: [C++] Add a fetch node based on a batch index - #34060
Merged
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged
GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Conversation
@westonpace
@westonpacewestonpace
commented
Feb 7, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).
This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.
This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.
So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).
This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.
@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.
lidavidm
lidavidm
reviewed
Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h
Outdated

Comment threadcpp/src/arrow/compute/exec/exec_plan.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc
Outdated

Comment threadcpp/src/arrow/testing/generator.h
Outdated

Comment threadcpp/src/arrow/compute/exec/fetch_node.cc
Outdated

@westonpace
westonpaceforce-pushed
the
feature/GH-34059--add-fetch-node
branch
from
e8a716b to
e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace
merged commit b056e07
into
apache:masterFeb 11, 2023
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] b056e07b ec2-t3-xlarge-us-east-2

[Failed] b056e07b test-mac-arm

[Finished] b056e07b ursa-i9-9960x

[Finished] b056e07b ursa-thinkcentre-m75q

[Finished] 24e5a580 ec2-t3-xlarge-us-east-2

[Failed] 24e5a580 test-mac-arm

[Finished] 24e5a580 ursa-i9-9960x

[Finished] 24e5a580 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property


3 participants

@westonpace@ursabot@lidavidm

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
GH-34059: [C++] Add a fetch node based on a batch index by westonpace · Pull Request #34060 · apache/arrow · GitHub
Skip to content
GH-34059: [C++] Add a fetch node based on a batch index - #34060
Merged
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Feb 11, 2023
Merged
GH-34059: [C++] Add a fetch node based on a batch index#34060
westonpace merged 8 commits into
apache:masterfrom 
westonpace:feature/GH-34059--add-fetch-node
Conversation
@westonpace
@westonpacewestonpace
commented
Feb 7, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
This PR introduces the concept of ExecBatch:index but does not yet do much with it. As a proof of concept this PR adds a fetch node which can be inserted anywhere in the plan (not just at the sink) to satisfy LIMIT x OFFSET y (Substrait calls this fetch and so I have also).
This PR also introduces two sequencing accumulation queues which will be useful, I hope, for anyone implementing nodes that rely on ordered execution.
This PR unfortunately introduces a new query option which is whether or not the sink node should pay the small performance hit required to sequence output. While considering how best to add this option I realized we will probably have more query options in the near future regarding "how much RAM to use" (e.g. spillover) and potentially more beyond that.
So I have taken all the options and put them into arrow::compute::QueryOptions (this already existed but it was not user facing and I added more things to it). I added a new DeclarationToXyz overload that accepts QueryOptions. This has, unfortunately, led to a bit of overload explosion but I think this should be the last new addition to the overload set (and we can deprecate the older overloads at some point).
This PR also includes a new gen::Gen / gen::TestGen facility for generating test tables for input. I'd like to eventually use this to simplify some of the existing exec plan tests as well. I'm willing to split this into a separate PR if that makes sense.
@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Feb 7, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34059has been automatically assigned in GitHub to PR creator.
lidavidm
lidavidm
reviewed
Feb 7, 2023
View reviewed changes
Comment threadcpp/src/arrow/compute/exec.h
Outdated

Comment threadcpp/src/arrow/compute/exec/exec_plan.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.h
Outdated

Comment threadcpp/src/arrow/compute/exec/accumulation_queue.cc
Outdated

Comment threadcpp/src/arrow/testing/generator.h
Outdated

Comment threadcpp/src/arrow/compute/exec/fetch_node.cc
Outdated

@westonpace
westonpaceforce-pushed
the
feature/GH-34059--add-fetch-node
branch
from
e8a716b to
e1e6dd7CompareFebruary 9, 2023 19:53
@westonpace
westonpace
merged commit b056e07
into
apache:masterFeb 11, 2023
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 24e5a58 and contender = b056e07. b056e07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.43% ⬆️0.98%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.22%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] b056e07b ec2-t3-xlarge-us-east-2

[Failed] b056e07b test-mac-arm

[Finished] b056e07b ursa-i9-9960x

[Finished] b056e07b ursa-thinkcentre-m75q

[Finished] 24e5a580 ec2-t3-xlarge-us-east-2

[Failed] 24e5a580 test-mac-arm

[Finished] 24e5a580 ursa-i9-9960x

[Finished] 24e5a580 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
Feb 11, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes
@bkietzbkietzAwaiting requested review from bkietz

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Create a fetch node based on a batch index property


3 participants

@westonpace@ursabot@lidavidm