Skip to content

ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464

Closed
rok wants to merge 8 commits into
apache:masterfrom
rok:ARROW-15665
Closed

ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom
rok:ARROW-15665

Conversation

@rok

@rokrok commented Feb 18, 2022

Copy link
Copy Markdown
Member

This is to resolve ARROW-15665.

@github-actions

github-actionsBot commented Feb 18, 2022

Copy link
Copy Markdown

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

@rok
rokforce-pushed the ARROW-15665 branch 2 times, most recently from da7c0b5 to 6352488CompareFebruary 23, 2022 02:58
@rok

rok commented Feb 23, 2022

Copy link
Copy Markdown
MemberAuthor

@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?
Would kernel preallocation settings help this or am I missing something else?

@pitrou

pitrou commented Feb 28, 2022

Copy link
Copy Markdown
Member

can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.

@rok
rokforce-pushed the ARROW-15665 branch 6 times, most recently from 4df261b to a71ed14CompareMarch 17, 2022 03:06
@rok
rok marked this pull request as ready for review March 17, 2022 03:52
@rok

rok commented Mar 17, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.

@rok

rok commented Mar 17, 2022

Copy link
Copy Markdown
MemberAuthor

@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?

lidavidm
lidavidm reviewed Mar 17, 2022
View reviewed changes

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

I think it's fine to remove it from scalar_string_ascii.cc.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
Comment threadcpp/src/arrow/compute/api_scalar.h Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?

@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?

@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?

Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?

@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.

@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)

@rokrokMar 24, 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.

Switched to a sequence counting approach.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
Comment threadpython/pyarrow/_compute.pyx Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
@rok
rokforce-pushed the ARROW-15665 branch 2 times, most recently from 33ce31a to 70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm reviewed Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
rok
rok commented Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
jorisvandenbossche
jorisvandenbossche reviewed Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?

Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)

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

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.

@rok
rokforce-pushed the ARROW-15665 branch 3 times, most recently from e721a39 to 04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm reviewed Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.

@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?

@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls

@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?

@rok

rok commented Mar 23, 2022

Copy link
Copy Markdown
MemberAuthor

@lidavidm thanks for the review. Could you take another look?

lidavidm
lidavidm approved these changes Mar 23, 2022
View reviewed changes

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

Thanks, LGTM overall, just a couple comments

lidavidm
lidavidm approved these changes Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc Outdated
int64_t* out_data = out->GetMutableValues<int64_t>(1);

if (self.error_is_null) {
if (!in.MayHaveNulls()) {

@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed

@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).

@rok
rok requested a review from lidavidmMarch 24, 2022 15:18
@ursabot

ursabot commented Mar 24, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 623a15e and contender = ddb663b. ddb663b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.51% ⬆️0.04%] 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

@asfimportasfimport mentioned this pull request Mar 30, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

Component: C++Component: PythonComponent: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche
, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche

, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche

, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche

, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche

, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche

, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche

, '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-15665: [C++] Add error handling option to StrptimeOptions by rok · Pull Request #12464 · apache/arrow · GitHub
Skip to content
ARROW-15665: [C++] Add error handling option to StrptimeOptions - #12464
Closed
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Closed
ARROW-15665: [C++] Add error handling option to StrptimeOptions#12464
rok wants to merge 8 commits into
apache:masterfrom 
rok:ARROW-15665
Conversation
@rok
@rokrok
commented
Feb 18, 2022

Copy link


Copy Markdown

Member
This is to resolve ARROW-15665.
@github-actions
github-actionsBot
commented
Feb 18, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-15665
@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
da7c0b5 to
6352488CompareFebruary 23, 2022 02:58
@rok
rok
commented
Feb 23, 2022

Copy link


Copy Markdown

MemberAuthor
@pitrou I'm not sure what's causing this (Check failed: out.array()->buffers[0] Null bitmap deleted by kernel but can_write_into_slices = true). Any insight?

Would kernel preallocation settings help this or am I missing something else?
@pitrou
pitrou
commented
Feb 28, 2022

Copy link


Copy Markdown

Member
can_write_into_slices means the kernel exec machinery won't let you redefine buffer addresses: you can write into the buffers given to you by the machinery but you are not allowed to point them elsewhere.
@rok
rokforce-pushed
the
ARROW-15665
branch
6 times, most recently
from
4df261b to
a71ed14CompareMarch 17, 2022 03:06
@rok
rok
marked this pull request as ready for review
March 17, 2022 03:52
@rok
rok
commented
Mar 17, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@pitrou I think this is ready for review. I'm not sure why can_write_into_slices=true is required here, could you please check that I'm not doing something ill advised.
@rok
rok
commented
Mar 17, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm I moved Strptime to scalar_temporal_unary.cc. Do you think that makes sense or better keep it in scalar_string_ascii.cc?
lidavidm
lidavidm
reviewed
Mar 17, 2022
View reviewed changes
@lidavidmlidavidm
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.

I think it's fine to remove it from scalar_string_ascii.cc.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@lidavidmlidavidmMar 17, 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.

What is the behavior if we don't raise parse errors?
@pitroupitrouMar 17, 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.

Indeed, we may want to make this more descriptive, e.g. bool error_is_null or something. Opinions?
@rokrokMar 17, 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.

Changed to error_is_null. How about error_returns_null?
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

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

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

@lidavidmlidavidmMar 17, 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.

Do we want to respect %% for escaping?
@rokrokMar 23, 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.

Switched to regex replace to do escaping. Feels a bit over engineered.
@lidavidmlidavidmMar 23, 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.

Regex is a bit much, how about just checking if the next char is '%' and advancing the string if so? (I think we do such things elsewhere)
@rokrokMar 24, 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.

Switched to a sequence counting approach.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

Comment threadpython/pyarrow/_compute.pyx
Outdated

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

@rok
rokforce-pushed
the
ARROW-15665
branch
2 times, most recently
from
33ce31a to
70d6b28CompareMarch 17, 2022 20:37
lidavidm
lidavidm
reviewed
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_string_test.cc
Outdated

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

rok
rok
commented
Mar 18, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
Mar 21, 2022
View reviewed changes
Comment threadpython/pyarrow/_compute.pyx
Outdated

@jorisvandenbosschejorisvandenbosscheMar 21, 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 should be False for the default?
Can you also add a test for this default? (a test that checks an error is raised for the default options without explicitly passing error_is_null)
@rokrokMar 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.

Ah sorry, it was a remainder from the previous raise_errors parameter. Changed and added a test.
@rok
rokforce-pushed
the
ARROW-15665
branch
3 times, most recently
from
e721a39 to
04202c9CompareMarch 22, 2022 19:14
lidavidm
lidavidm
reviewed
Mar 22, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

@lidavidmlidavidmMar 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, we might need to test this more. I thought we merged an optimization where INTERSECTION wouldn't allocate a null bitmap if all inputs were non-null.
@rokrokMar 23, 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.

Hm, what would you recommend we test? Bigger arrays?
@lidavidmlidavidmMar 23, 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.

Arrays with no input nulls
@rokrokMar 24, 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.

Got it. Added bitmap allocation and tests. Could you please check if we're still missing something?
@rok
rok
commented
Mar 23, 2022

Copy link


Copy Markdown

MemberAuthor
@lidavidm thanks for the review. Could you take another look?
lidavidm
lidavidm
approved these changes
Mar 23, 2022
View reviewed changes
@lidavidmlidavidm
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.

Thanks, LGTM overall, just a couple comments
lidavidm
lidavidm
approved these changes
Mar 24, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Outdated

 int64_t* out_data = out->GetMutableValues<int64_t>(1);

 if (self.error_is_null) {
 if (!in.MayHaveNulls()) {
@lidavidmlidavidmMar 24, 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.

nit: it might be better to directly check out->buffers[0] instead of relying on the optimization being performed
@rokrokMar 24, 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.

Switched to if (out->buffers[0] == nullptr).
@rok
rok
requested a review
from lidavidmMarch 24, 2022 15:18
@ursabot
ursabot
commented
Mar 24, 2022•

edited

Loading

Copy link


Copy Markdown

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

Conbench compare runs links:

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

[Finished ⬇️0.34% ⬆️0.08%] test-mac-arm

[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.51% ⬆️0.04%] 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
@asfimportasfimport
mentioned this pull request
Mar 30, 2022

Closed

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

Reviewers

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

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@pitrou@ursabot@lidavidm@jorisvandenbossche