Skip to content

ARROW-14092: [C++] subtract(date, duration) -> timestamp kernel - #12136

Closed
rok wants to merge 3 commits into
apache:masterfrom
rok:ARROW-14092
Closed

ARROW-14092: [C++] subtract(date, duration) -> timestamp kernel#12136
rok wants to merge 3 commits into
apache:masterfrom
rok:ARROW-14092

Conversation

@rok

@rokrok commented Jan 12, 2022

Copy link
Copy Markdown
Member

@github-actions

github-actionsBot commented Jan 12, 2022

Copy link
Copy Markdown

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

lidavidm
lidavidm reviewed Jan 18, 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.

As discussed on other PRs we probably need to add more implicit casts for this (e.g. date32 - duration[ns] => timestamp[ns] - duration[ns] perhaps)

Comment threadcpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
@rokrok mentioned this pull request Jan 18, 2022
Closed
@rok
rok requested a review from lidavidmJanuary 20, 2022 00:04
lidavidm
lidavidm reviewed Jan 20, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/codegen_internal_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.cc Outdated
Comment threaddocs/source/cpp/compute.rst Outdated

@lidavidmlidavidmJan 20, 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.

Should we add a note about temporal casting here?

@rokrokJan 20, 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.

Will do.

@rokrokJan 20, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

lidavidm
lidavidm reviewed Jan 21, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated

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

Now that I think about it, this is kind of weird to use CommonTemporal since there isn't actually a "common" type between date32 and duration (for instance) and what we actually care about here is the common unit - maybe it's worth splitting that off separately? Since future/other uses of CommonTemporal might be 'surprised' to find that CommonTemporal(date, duration) => date instead of nullptr.

@rokrokJan 21, 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.

How about renaming CommonTemporal to CommonResolution and returning finest time unit resolution?

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

CommonTemporal is used elsewhere, but having CommonResolution alongside it makes sense.

@rokrokJan 25, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added CommonTemporalResolution and refactored ReplaceTemporalTypes. This PR should hopefully cover all temporal casts needed for ARROW-11901.

@rok
rokforce-pushed the ARROW-14092 branch 2 times, most recently from bc71a08 to 7e7b417CompareJanuary 25, 2022 13:32
lidavidm
lidavidm reviewed Jan 25, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.cc Outdated
lidavidm
lidavidm reviewed Jan 25, 2022
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/codegen_internal.cc Outdated
@rok

rok commented Jan 25, 2022

Copy link
Copy Markdown
MemberAuthor

@lidavidm rebased one more time and changed back to checked_cast. Should be good to go now.

@rok
rok requested a review from lidavidmJanuary 26, 2022 14:01
@lidavidm

lidavidm commented Jan 26, 2022

Copy link
Copy Markdown
Member

Sorry for the delay - time to rebase the other PRs now :) (feel free to ping me on anything that needs looking at)

@ursabot

ursabot commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 9e69ccf and contender = c4eb8dc. c4eb8dc 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 ⬇️2.03% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.35% ⬆️0.3%] 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

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

Reviewers

@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rok@lidavidm@ursabot