') + ')', '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-14808 [R] Implement bindings for `lubridate::date()` by dragosmg · Pull Request #12433 · apache/arrow · GitHub
Skip to content

ARROW-14808 [R] Implement bindings for lubridate::date() - #12433

Closed
dragosmg wants to merge 40 commits into
apache:masterfrom
dragosmg:lubridate_date
Closed

ARROW-14808 [R] Implement bindings for lubridate::date()#12433
dragosmg wants to merge 40 commits into
apache:masterfrom
dragosmg:lubridate_date

Conversation

@dragosmg

@dragosmgdragosmg commented Feb 15, 2022

Copy link
Copy Markdown
Contributor

The following will be supported in arrow (base::as.Date() included to show the difference to date(), but not supported):

library(dplyr)
library(lubridate)
df<- tibble(a= as.POSIXct("2012-03-26 23:12:13", tz="America/New_York"))
df %>%
mutate(
a_date= date(a),
a_date_base= as.Date(a))
#> # A tibble: 1 × 3#> a a_date a_date_base#> <dttm> <date> <date> #> 1 2012-03-26 23:12:13 2012-03-26 2012-03-27
library(arrow)
library(dplyr)
library(lubridate)
df<- tibble(a= as.POSIXct("2012-03-26 23:12:13", tz="America/New_York"))
df %>% arrow_table() %>% mutate(
a_date= date(a)
) %>% collect()
#> # A tibble: 1 × 2#> a a_date #> <dttm> <date> #> 1 2012-03-26 23:12:13 2012-03-26

Created on 2022-02-15 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@dragosmg
dragosmg marked this pull request as draft February 15, 2022 17:26
@dragosmg
dragosmg marked this pull request as ready for review February 15, 2022 17:30
@dragosmg
dragosmg marked this pull request as draft February 15, 2022 17:45
@dragosmg
dragosmg marked this pull request as ready for review February 15, 2022 19:36

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

Looking good — a few questions for you

Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
Comment threadr/R/dplyr-funcs-datetime.R Outdated
@dragosmg
dragosmgforce-pushed the lubridate_date branch 2 times, most recently from 57ce8c9 to 2d72983CompareFebruary 23, 2022 14:27

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

One comment + suggestion.

Do we have a ticket to do as.Date separately? Could we do it with this one since it is very very similar?

Comment threadr/R/dplyr-funcs-datetime.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated

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

A few comments

Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated

@jonkeanejonkeane 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 is making progress! I didn't quite realize that as.Date() would be as much of a can of worms as it's been — but that's ok, I think it is useful additional functionality!

In the as.Date() implementation it might be nice to restructure the flow so that you have one call at the end to build_expr("cast", intermediate, options = cast_options(to_type = date32())) after having built up intermediate getting things into the right format and then do the cast-to-date (I put intermediate here but that's probably not a good name for it, maybe "just" x like we do elsewhere — I trust you!).

Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
@dragosmg

dragosmg commented Feb 28, 2022

Copy link
Copy Markdown
ContributorAuthor

I have created an umbrella issue (ARROW-15805) for the possible improvements to the as.Date() binding + ARROW-15800 to implement as_date().

@jonkeanejonkeane 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 is looking good, a few more questions. I didn't realize just how much extra scope as.Date() would add — though I think this is good functionality to add, so that's ok, but don't feel dispirited this is taking longer!

Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
@dragosmg
dragosmg requested a review from jonkeaneMarch 1, 2022 13:27

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

Still getting there, most of the code changes I proposed are attempts to avoid some of the nesting in if/elses. Though I also do think we can support as.Date() with any timezone which will simplify + remove some of the tests down below catching that error.

Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/R/dplyr-funcs-type.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-type.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-type.R Outdated
Comment threadr/tests/testthat/test-dplyr-funcs-type.R Outdated
@dragosmg
dragosmg requested a review from jonkeaneMarch 2, 2022 12:10
@dragosmg

Copy link
Copy Markdown
ContributorAuthor

Thanks @jonkeane. Would you mind having another look?

@jonkeanejonkeane 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 is fantastic, thank you for all of the pushes. This is incredibly close — one minor comment about an additional comment and one suggestion to use compare_dplyr_binding for testing the warning flow "not supported in arrow -> pulling into R". It's functionally the same, but follows the pattern we use elsewhere.

I'm happy to merge even without these, but these two would add a bit of polish + fit

Comment threadr/R/dplyr-funcs-type.R
Comment threadr/R/dplyr-funcs-type.R
Comment threadr/tests/testthat/test-dplyr-funcs-type.R
Comment threadr/R/dplyr-funcs-type.R

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

Once the CI is green, I'll merge.

Thank you again for all of the work you did on this!

@dragosmg

Copy link
Copy Markdown
ContributorAuthor

Thanks for your patience and support 😉

@ursabot

ursabot commented Mar 3, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ce46c1a and contender = 9719eae. 9719eae 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.13% ⬆️0.0%] test-mac-arm
[Failed ⬇️3.21% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.26% ⬆️0.09%] 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

@dragosmg
dragosmg deleted the lubridate_date branch March 3, 2022 18:15
jonkeane pushed a commit that referenced this pull request Mar 8, 2022
`build_expr()` handles the conversion of the inputs to `Array` or `Scalar`.
#12433 (comment)Closes#12563 from dragosmg/build_expr_in_as_methods
Authored-by: Dragoș Moldovan-Grünfeld <dragos.mold@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@dragosmg@ursabot@jonkeane@thisisnic