Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, '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" + '
ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, '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('^' + ".*" + ' ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, '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('^' + ".*" + ' ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, '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" + ' ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, '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('^' + ".*" + ' ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, '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); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() by paleolimbot · Pull Request #11850 · apache/arrow · GitHub
Skip to content

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() - #11850

Closed
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type
Closed

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration()#11850
paleolimbot wants to merge 13 commits into
apache:masterfrom
paleolimbot:r-duration-type

Conversation

@paleolimbot

Copy link
Copy Markdown
Member

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
// TODO: look in lubridate
returnStatus::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts=FALSE)
Array$create(as.difftime(123, units="secs"))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1)

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Comment threadr/src/r_to_arrow.cpp Outdated
Co-authored-by: Romain François <romain@rstudio.com>
Comment threadr/src/r_to_arrow.cpp Outdated
@paleolimbot
paleolimbot marked this pull request as ready for review December 10, 2021 16:06
@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Ok! This now works:

library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <time32[s]>#> [#> 00:02:03#> ]
as.vector(arr)
#> Time difference of 123 secs

Created on 2021-12-10 by the reprex package (v2.0.1)

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts=FALSE)
(arr<-Array$create(as.difftime(123, units="secs")))
#> Array#> <duration[s]>#> [#> 123#> ]
as.vector(arr)
#> Time difference of 123 secs

@nealrichardson

nealrichardson commented Dec 13, 2021

Copy link
Copy Markdown
Member

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

@jonkeane

Copy link
Copy Markdown
Member

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

@ursabot

ursabot commented Dec 13, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@thisisnic

Copy link
Copy Markdown
Member

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

@paleolimbot

Copy link
Copy Markdown
MemberAuthor

Done! See ARROW-15098.

pitrou pushed a commit that referenced this pull request Dec 15, 2021
This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker?
Closes#11947 from paleolimbot/r-fix-duration
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Antoine Pitrou <antoine@python.org>
jonkeane pushed a commit that referenced this pull request Dec 16, 2021
The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't.
Closes#11948 from paleolimbot/r-duration-in-vignette
Authored-by: Dewey Dunnington <dewey@fishandwhistle.net>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@paleolimbot
paleolimbot deleted the r-duration-type branch December 9, 2022 16:38
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.

6 participants

@paleolimbot@nealrichardson@jonkeane@ursabot@thisisnic@romainfrancois