Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support to_utc_timestamp [databricks] #10144

Merged
merged 6 commits into from
Jan 8, 2024
Merged

Support to_utc_timestamp [databricks] #10144

merged 6 commits into from
Jan 8, 2024

Conversation

thirtiseven
Copy link
Collaborator

@thirtiseven thirtiseven commented Jan 3, 2024 •
edited
Loading

Closes #10145

This PR supports to_utc_timestamp and refactors some test cases from from_utc_timestamp.

It's a simple wrap for kernel fromTimestampToUtcTimestamp, I implemented it to test some overlap and gap issues in TimeAdd. (It turns out that to_utc_timestamp has a slightly different way of handling overlap and gap than TimeAdd, I'll describe it in the TimeAdd PR later).

Perf tests:

test,type,zone,used MS
to_utc_timestamp,Cpu,Asia/Shanghai,1075
to_utc_timestamp,Gpu,Asia/Shanghai,101
to_utc_timestamp,Cpu,Asia/Shanghai,984
to_utc_timestamp,Gpu,Asia/Shanghai,95
to_utc_timestamp,Cpu,Asia/Shanghai,986
to_utc_timestamp,Gpu,Asia/Shanghai,111
to_utc_timestamp,Cpu,Asia/Shanghai,1034
to_utc_timestamp,Gpu,Asia/Shanghai,100
to_utc_timestamp,Cpu,Asia/Shanghai,1046
to_utc_timestamp,Gpu,Asia/Shanghai,108
to_utc_timestamp, Asia/Shanghai: mean cpu time: 1025.00 ms, mean gpu time: 103.00 ms, speedup: 9.95 x
to_utc_timestamp,Cpu,Japan,906
to_utc_timestamp,Gpu,Japan,104
to_utc_timestamp,Cpu,Japan,892
to_utc_timestamp,Gpu,Japan,100
to_utc_timestamp,Cpu,Japan,941
to_utc_timestamp,Gpu,Japan,96
to_utc_timestamp,Cpu,Japan,989
to_utc_timestamp,Gpu,Japan,96
to_utc_timestamp,Cpu,Japan,887
to_utc_timestamp,Gpu,Japan,92
to_utc_timestamp, Japan: mean cpu time: 923.00 ms, mean gpu time: 97.60 ms, speedup: 9.46 x

@thirtiseven
Support to_utc_timestamp
1fb69ac 
Signed-off-by: Haoyang Li <[email protected]>
@thirtiseven
add tests
76a99d6 
Signed-off-by: Haoyang Li <[email protected]>
@thirtiseven thirtiseven self-assigned this Jan 3, 2024
@thirtiseven thirtiseven changed the title Support to_utc_timestamp for non-utc timezone Support to_utc_timestamp Jan 3, 2024
@thirtiseven
perf tests
c5daeec 
Signed-off-by: Haoyang Li <[email protected]>
@thirtiseven
claen up
86e825e 
Signed-off-by: Haoyang Li <[email protected]>
@thirtiseven
claen up
c3e27a8 
Signed-off-by: Haoyang Li <[email protected]>
thirtiseven
thirtiseven commented Jan 3, 2024
View reviewed changes
integration_tests/src/main/python/date_time_test.py

@pytest.mark.parametrize('time_zone', ["Asia/Shanghai", "UTC", "UTC+0", "UTC-0", "GMT", "GMT+0", "GMT-0"], ids=idfn)
@pytest.mark.parametrize('data_gen', [timestamp_gen], ids=idfn)
@tz_sensitive_test
Copy link
Collaborator Author

@thirtiseven thirtiseven Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In my understanding it's not a tz_sensitive_test case because it will only run on gpu under utc timezone and fallback for all other timezones. We can add the timezones we want to test to the supported_timezones list.

integration_tests/src/main/python/date_time_test.py
@allow_non_gpu(*non_utc_allow)
def test_from_utc_timestamp_supported_timezones(data_gen, time_zone):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This case looks duplicated with test_from_utc_timestamp so I combined them.

@sameerz sameerz added the feature request New feature or request label Jan 3, 2024
NVnavkumar
NVnavkumar reviewed Jan 5, 2024
View reviewed changes
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/datetimeExpressions.scala Outdated

private[this] var timezoneId: ZoneId = null

override def tagExprForGpu(): Unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like ToUTCTimestampExprMeta has basically the same logic for whether it will run on the GPU as FromUTCTimestampExprMeta. I think these 2 classes should be refactored to share this logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@res-life
Copy link
Collaborator

res-life commented Jan 5, 2024

LGTM

@res-life res-life changed the title Support to_utc_timestamp Support to_utc_timestamp [databricks] Jan 5, 2024
@res-life
Copy link
Collaborator

res-life commented Jan 5, 2024

build

@thirtiseven
address comment
7283527 
Signed-off-by: Haoyang Li <[email protected]>
@thirtiseven
Copy link
Collaborator Author

build

res-life
res-life approved these changes Jan 8, 2024
View reviewed changes
Copy link
Collaborator

@res-life res-life left a comment

Choose a reason for hiding this comment

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

LGTM

@thirtiseven thirtiseven merged commit 328a514 into NVIDIA:branch-24.02 Jan 8, 2024
39 checks passed
@amahussein amahussein mentioned this pull request Jan 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NVnavkumar NVnavkumar NVnavkumar left review comments

@res-life res-life res-life approved these changes

Assignees

@thirtiseven thirtiseven

Labels
feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Support to_utc_timestamp
4 participants
@thirtiseven @res-life @NVnavkumar @sameerz