Skip to content

ARROW-15680: [C++] Temporal floor/ceil/round should accept week_starts_monday when rounding to multiple of week - #12515

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

ARROW-15680: [C++] Temporal floor/ceil/round should accept week_starts_monday when rounding to multiple of week#12515
rok wants to merge 2 commits into
apache:masterfrom
rok:ARROW-15680

Conversation

@rok

@rokrok commented Feb 25, 2022

Copy link
Copy Markdown
Member

This is to resolve ARROW-15680.

@github-actions

github-actionsBot commented Feb 25, 2022

Copy link
Copy Markdown

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

@rokrok changed the title ARROW-15680: [C++] Temporal floor/ceil/round should accept week_start when rounding to multiple of weekARROW-15680: [C++] Temporal floor/ceil/round should accept week_starts_monday when rounding to multiple of weekFeb 25, 2022
lidavidm
lidavidm approved these changes Feb 28, 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.

LGTM.

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

@lidavidmlidavidmFeb 28, 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: is there any point templating on Unit when it's apparently always weeks (and doesn't seem to make sense for any other unit)?

@rokrokFeb 28, 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.

Good point! I originally wanted to use Ceil/Floor/RoundTimePoint but then changed the design and forgot this there. Will fix.

@rokrokFeb 28, 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.

@rok

rok commented Feb 28, 2022

Copy link
Copy Markdown
MemberAuthor

@lidavidm I rebased to see if appveyor issue goes away.

@lidavidm

lidavidm commented Feb 28, 2022

Copy link
Copy Markdown
Member

@rok I think it's conda-forge/abseil-cpp-feedstock#25 (comment)

@rok

rok commented Feb 28, 2022

Copy link
Copy Markdown
MemberAuthor

@rok I think it's conda-forge/abseil-cpp-feedstock#25 (comment)

Yeah I think it was that indeed.

@lidavidm

lidavidm commented Feb 28, 2022

Copy link
Copy Markdown
Member

I think we can ignore these CI failures for now, merging.

@ursabot

ursabot commented Feb 28, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = b16329e and contender = 6907398. 6907398 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.92% ⬆️0.17%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented Mar 1, 2022•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = b16329e and contender = 6907398. 6907398 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️1.72% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees

No one assigned

Labels

Component: C++Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rok@lidavidm@ursabot