') + ')', '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('^' + ".*" + ', '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" + ', '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('^' + ".*" + ', '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); } })(); })(); ARROW-16419: [Python] Properly wait for ExecPlan to finish by lidavidm · Pull Request #13035 · apache/arrow · GitHub
Skip to content

ARROW-16419: [Python] Properly wait for ExecPlan to finish - #13035

Closed
lidavidm wants to merge 1 commit into
apache:masterfrom
lidavidm:arrow-16419
Closed

ARROW-16419: [Python] Properly wait for ExecPlan to finish#13035
lidavidm wants to merge 1 commit into
apache:masterfrom
lidavidm:arrow-16419

Conversation

@lidavidm

@lidavidmlidavidm commented Apr 29, 2022

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actionsBot commented Apr 29, 2022

Copy link
Copy Markdown

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

westonpace
westonpace approved these changes Apr 29, 2022
View reviewed changes
Comment threadpython/pyarrow/includes/libarrow.pxd
Comment on lines +84 to +86
cdef extern from "arrow/util/future.h" namespace "arrow" nogil:
cdef cppclass CFuture_Void" arrow::Future<>":
CStatus status()

@westonpacewestonpaceApr 29, 2022

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.

Surprising we've made it this far without exposing Future. I wonder if we want to add a WaitForFinish method on ExecPlan instead of exposing Future (essentially a sync mirror-api) like we do in the Scanner.

I don't really have any problems with this approach though. Just making an observation.

@pitroupitrouMay 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose at some point we'll want to expose async APIs in Python, so we're probably bound to interface with Future in Cython anyway.

@westonpacewestonpace mentioned this pull request Apr 30, 2022
Closed
@kszucs

kszucs commented May 2, 2022

Copy link
Copy Markdown
Member

How severe this issue is? Does this belong to a new feature? I'm not sure whether we should block the release on it or not.

cc @pitrou

pitrou
pitrou approved these changes May 2, 2022
View reviewed changes

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

+1, thank you @lidavidm

@pitrou

pitrou commented May 2, 2022•
edited
Loading

Copy link
Copy Markdown
Member

@kszucs It should probably not block the release, but worth including if you cut another RC. And, yes, it's part of a new feature.

cc @amol-

@pitroupitrou closed this in 3f2c33fMay 2, 2022
@lidavidm
lidavidm deleted the arrow-16419 branch May 2, 2022 11:55
@lidavidm

lidavidm commented May 2, 2022

Copy link
Copy Markdown
MemberAuthor

This by itself is not blocking but see #13036 which is the fix for the segfault I was running into on the ML.

lidavidm added a commit that referenced this pull request May 2, 2022
@westonpace@lidavidm
ARROW-16417: [C++][Python] Segfault in test_exec_plan.py / test_joins
7809c6d
This builds on top of #13035 which is also important for avoiding segmentation faults. On top of that there were a few more problems:
* The python was using `SourceNodeOptions::FromTable` which is a rather dangerous method (mainly useful for unit testing) as it doesn't share ownership of the input table (even worse, it takes a const ref). Python was not keeping the table alive and it was maybe possible for the table to deleted out from under the plan (I'm not entirely sure this was causing issues but it seemed risky). I switched to TableSourceNode which shares ownership of the table (and is a bit more efficient).
* Setting use_threads to False did nothing because `_perform_join` was not passing the arg on to `execplan`.
* When fixing the above and running with `use_threads=False` it was creating a single thread executor but the current best practice is to pass in nullptr.
* Finally, the actual bug was my improper fix in #12894 . I had still left a small window open for `End` to be called between `Submit` and `AddTask` which would allow the task to be submitted but not participate in setting `finished` on the node.
Closes#13036 from westonpace/bugfix/ARROW-16417--segfault-in-python-join
Lead-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@ursabot

ursabot commented May 6, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 22a1885 and contender = 3f2c33f. 3f2c33f 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
[Finished ⬇️0.35% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.16% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 3f2c33f5 ec2-t3-xlarge-us-east-2
[Finished] 3f2c33f5 test-mac-arm
[Failed] 3f2c33f5 ursa-i9-9960x
[Finished] 3f2c33f5 ursa-thinkcentre-m75q
[Finished] 22a18852 ec2-t3-xlarge-us-east-2
[Finished] 22a18852 test-mac-arm
[Finished] 22a18852 ursa-i9-9960x
[Finished] 22a18852 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

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

Reviewers

@westonpacewestonpacewestonpace approved these changes
@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lidavidm@kszucs@pitrou@ursabot@westonpace