') + ')', '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-14101: [C++] multiply(duration, integer) -> duration kernel by rok · Pull Request #12215 · apache/arrow · GitHub
Skip to content

ARROW-14101: [C++] multiply(duration, integer) -> duration kernel - #12215

Closed
rok wants to merge 2 commits into
apache:masterfrom
rok:ARROW-14101
Closed

ARROW-14101: [C++] multiply(duration, integer) -> duration kernel#12215
rok wants to merge 2 commits into
apache:masterfrom
rok:ARROW-14101

Conversation

@rok

@rokrok commented Jan 20, 2022

Copy link
Copy Markdown
Member

This is to resolve ARROW-14101 and ARROW-14102.

@rokrok changed the title ARROW-14101: [C++] multiply(interval, integer) -> interval kernelARROW-14101: [C++] multiply(duration, integer) -> duration kernelJan 20, 2022
@github-actions

github-actionsBot commented Jan 20, 2022

Copy link
Copy Markdown

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

@jorisvandenbossche

jorisvandenbossche commented Jan 27, 2022

Copy link
Copy Markdown
Member

For the mutiply_checked, should we add a test for a duration that would go out of bounds? Or is this already sufficiently covered by integer multiply_checked tests if that is using the same underlying implementation?

@rok

rok commented Jan 27, 2022

Copy link
Copy Markdown
MemberAuthor

For the mutiply_checked, should we add a test for a duration that would go out of bounds? Or is this already sufficiently covered by integer multiply_checked tests if that is using the same underlying implementation?

Another test won't hurt :) I'll add it.

@rok
rokforce-pushed the ARROW-14101 branch 3 times, most recently from 1fd50f0 to 9f40218CompareJanuary 27, 2022 19:40
@rok

rok commented Jan 27, 2022

Copy link
Copy Markdown
MemberAuthor

Added the test and removed divide checked (seemed redundant since we're only dividing with integers).

@rok
rokforce-pushed the ARROW-14101 branch 2 times, most recently from c27af81 to 6e4f4bdCompareFebruary 2, 2022 17:55
@rokrok mentioned this pull request Feb 8, 2022
Closed
@rok
rokforce-pushed the ARROW-14101 branch 3 times, most recently from 7ad426d to 33d1458CompareFebruary 9, 2022 19:23
@rok
rokforce-pushed the ARROW-14101 branch 2 times, most recently from d4fdb74 to 5c015d4CompareFebruary 17, 2022 17:35
@rok

rok commented Feb 18, 2022

Copy link
Copy Markdown
MemberAuthor

@pitrou could you please take a look at this one? It covers multiply and divide (ARROW-14102).

I think the CI error is due to s3fs.

jorisvandenbossche
jorisvandenbossche reviewed Feb 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated

@jorisvandenbosschejorisvandenbosscheFeb 22, 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.

Can you have a case in this test where the division is not exact?

@rokrokFeb 22, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added.

jorisvandenbossche
jorisvandenbossche reviewed Feb 22, 2022
View reviewed changes
Comment threaddocs/source/cpp/compute.rst Outdated
@jorisvandenbossche

jorisvandenbossche commented Feb 22, 2022

Copy link
Copy Markdown
Member

Added the test and removed divide checked (seemed redundant since we're only dividing with integers).

I would keep divide_checked. You could give the same argument for divide_checked being redundant for integer types, but we still allow you to use it. It could be annoying that, when using divide_checked, you need to switch to divide, only if your input type is duration.

@rok

rok commented Feb 22, 2022

Copy link
Copy Markdown
MemberAuthor

Thanks for reviewing @jorisvandenbossche. Agreed on the divide_checked. Added.

pitrou
pitrou reviewed Feb 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/codegen_internal_test.cc Outdated
Comment on lines 205 to 206

@pitroupitrouFeb 22, 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.

This looks unexpected to me since int64 is not a temporal at all. @lidavidm What do you think?

@lidavidmlidavidmFeb 22, 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 think this is fine, but maybe we need a better name for what this function does.

That said, I don't think this change is needed for this function to work, anyways. There's only a single temporal argument so there isn't really a meaningful common resolution to cast to.

@pitroupitrouFeb 22, 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.

To be honest, the function doesn't seem to have changed :-). It's just the additional test that surprised me (but adding a test for existing behaviour is useful, thank you @rok ).

@rokrokFeb 22, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well duration has a unit and int64 is dimensionless. The goal of the function is to find the best common unit and I'd argue it still does.

Comment threadcpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated

@pitroupitrouFeb 22, 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.

Is it worth also supporting multiply(int64, duration) or would commutativity be handled at an upper level? @lidavidm

@lidavidmlidavidmFeb 22, 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.

Hmm, I don't see why not. It could just use the same exec function.

@rokrokFeb 22, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated

@pitroupitrouFeb 22, 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.

Can you some negative value(s) into the mix?

@rokrokFeb 22, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated

@pitroupitrouFeb 22, 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.

Also check that division by zero raises?

@rokrokFeb 22, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added.

@rok
rok requested a review from pitrouFebruary 23, 2022 11:07
@ursabot

ursabot commented Feb 23, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 194ace5 and contender = 2f8f1e8. 2f8f1e8 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.25% ⬆️0.13%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.38% ⬆️0.21%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. 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

@rok

rok commented Feb 23, 2022

Copy link
Copy Markdown
MemberAuthor

Thanks all!

@rokrok mentioned this pull request Nov 17, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm left review comments
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@ursabot@lidavidm@pitrou