Support date_format via Gpu for non-UTC time zone [databricks] #9721

res-life · 2023-11-15T14:13:42Z

PR includes:

Support date_format via Gpu for non-UTC time zone
Split test_date_format to test date and time separately. date_format(date) will introduce cast(date, timestamp) which is not supported in non-UTC now. Only test date_format(timestamp)
Add fall back case for non-supported time zone.

Signed-off-by: Chong Gao [email protected]

revans2

The core changes to dateTimeExpressions.scala looks good, but I don't feel good checking in code that changes how we tag TimeZoneAware/TimestampTypes until we have tests in place that verify that we are still falling back in all the right places.

res-life · 2023-11-16T03:59:49Z

build

sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala

res-life · 2023-11-21T10:33:10Z

build

res-life · 2023-11-27T01:04:55Z

build

Signed-off-by: Chong Gao <[email protected]>

res-life · 2023-12-05T05:31:32Z

Please review the last commit.

res-life · 2023-12-05T05:31:51Z

build

integration_tests/src/main/python/time_zone_utils.py

winningsix · 2023-12-05T05:46:57Z

integration_tests/src/main/python/time_zone_utils.py

+
+# get from Java:
+# ZoneId.getAvailableZoneIds
+# id.getRules.isFixedOffset || id.getRules.getTransitionRules.isEmpty


Hmm, can we use Python Java gateway (e.g., Py4j) to obtain the list instead of hardcoding it here? Besides the maintenance, just want to ensure we're consistent with Java on this behavior id.getRules.isFixedOffset || id.getRules.getTransitionRules.isEmpty.

It's invoked in the pytest marks, Python Java gateway is not available when Spark session is not started.

Done. Now is using the following:

@pytest.mark.skipif(is_supported_time_zone() ......)

The is_supported_time_zone works in the pytest marks.

integration_tests/src/main/python/date_time_test.py

winningsix · 2023-12-05T05:54:13Z

integration_tests/src/main/python/date_time_test.py

    assert_gpu_and_cpu_are_equal_collect(
        lambda spark : unary_op_df(spark, data_gen).selectExpr("date_format(a, '{}')".format(date_format)))

+@pytest.mark.parametrize('date_format', supported_date_formats, ids=idfn)
+@pytest.mark.parametrize('data_gen', [timestamp_gen], ids=idfn)
+@pytest.mark.xfail(is_dst_time_zone(), reason="only support non-DST time zone, refer to https://github.com/NVIDIA/spark-rapids/issues/6839")


Can we avoid xfail here?

If is_dst_time_zone = false, test result equals without fallback.
If is_dst_time_zone = true, test result equals with CPU operator fallback. This one can be combined with test_date_format_for_date.

Using xfail is to record the xfail/xpass cases.
I'm OK to use skipif, but we may forget it in the future.

can we use fallback check instead? I am nervous on xfail which hides errors.

OK, will do it

res-life · 2023-12-05T07:57:01Z

build

res-life · 2023-12-06T07:13:46Z

This one is depending on #9814
When #9814 is merged, I'll then update this one.

res-life · 2023-12-08T03:41:39Z

Test passed locally:

export TZ=Iran; pass
export TZ=America/Los_Angeles; fallback works

res-life · 2023-12-08T03:42:04Z

build

winningsix · 2023-12-12T02:16:40Z

integration_tests/src/main/python/date_time_test.py

-@allow_non_gpu(*non_utc_allow)
-def test_date_format(data_gen, date_format):
+@pytest.mark.parametrize('data_gen', [date_gen], ids=idfn)
+@allow_non_gpu('ProjectExec')


need to check whether it's supported timezone?

date_format(date) will introduce cast(date as timestamp) which is not supported in non-UTC now.
After we support cast(date as timestamp), then we will update this case.

winningsix · 2023-12-12T02:18:27Z

integration_tests/src/main/python/date_time_test.py

+    assert_gpu_fallback_collect(
+        lambda spark : unary_op_df(spark, data_gen).selectExpr("date_format(a, '{}')".format(date_format)),
+        'ProjectExec',
+        conf)


nit: you can merge https://github.com/NVIDIA/spark-rapids/pull/9721/files#diff-ec41a643cf16a9946f6df2498c6d853812f526eb0a3f8d3299b4a911346e1703R491 here directly without creating conf variable.

winningsix · 2023-12-12T02:19:09Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

@@ -1681,8 +1681,9 @@ object GpuOverrides extends Logging {
            .withPsNote(TypeEnum.STRING, "A limited number of formats are supported"),
            TypeSig.STRING)),
      (a, conf, p, r) => new UnixTimeExprMeta[DateFormatClass](a, conf, p, r) {
+        override def isTimeZoneSupported = true
        override def convertToGpu(lhs: Expression, rhs: Expression): GpuExpression =


nit: code style. not sure whether a line break need here.

ttnghia · 2023-12-12T21:34:31Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

@@ -1681,8 +1681,9 @@ object GpuOverrides extends Logging {
            .withPsNote(TypeEnum.STRING, "A limited number of formats are supported"),
            TypeSig.STRING)),
      (a, conf, p, r) => new UnixTimeExprMeta[DateFormatClass](a, conf, p, r) {
+        override def isTimeZoneSupported = true


Why do we need this?

This is due to the fact that DateFormatClass is not considered a TimeZoneAwareExpression, but requires support for non-UTC timezones. It's the last check for timezone requirement in Expressions. By default, this flag is false.

@ttnghia FYI. Some original notes around this.

It seems not a good practice to set some value as false by default then override true like this. Can we compute that value based on the input in the object constructor?

res-life · 2023-12-13T02:15:19Z

build

NVnavkumar · 2023-12-14T01:11:22Z

pre-merge failed due to #10050

NVnavkumar · 2023-12-14T01:11:26Z

build

res-life requested review from winningsix and revans2 November 15, 2023 14:13

res-life mentioned this pull request Nov 15, 2023

[WIP] Move timezone check to each operator [databricks] #9482

Closed

revans2 reviewed Nov 15, 2023

View reviewed changes

winningsix reviewed Nov 16, 2023

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala Outdated Show resolved Hide resolved

NVnavkumar linked an issue Nov 21, 2023 that may be closed by this pull request

[FEA] Support DateFormat on GPU with a non-UTC timezone #9801

Closed

sameerz added the task Work required that improves the product but is not user facing label Nov 21, 2023

res-life changed the title ~~Support CPU path for date_format function with timezone~~ Support GPU path for date_format function with timezone Nov 27, 2023

res-life changed the base branch from branch-23.12 to branch-24.02 November 28, 2023 01:57

Chong Gao added 5 commits December 4, 2023 18:41

Support from_unixtime via Gpu for non-UTC time zone

7625e16

Signed-off-by: Chong Gao <[email protected]>

Add is_dst_tz; Fix

6c961fb

Update comments; Refactor; Use Gpu path

4c6133e

Add licence info; re-order imports

92741c6

xfail case instead of skipif

877130a

res-life changed the title ~~Support GPU path for date_format function with timezone~~ Support date_format via Gpu for non-UTC time zone Dec 5, 2023

Support date_format via GPU for non-UTC time zone

e3941c2

Signed-off-by: Chong Gao <[email protected]>

res-life force-pushed the non-utc-date_format branch from 4fbfbb5 to e3941c2 Compare December 5, 2023 05:29

winningsix reviewed Dec 5, 2023

View reviewed changes

Fix; Update license; Update allow onon gpu list

1538d82

Chong Gao added 3 commits December 8, 2023 10:53

Merge branch-24.02

4eaa4dd

Remove useless code

7e61915

Add date_format test cases

96924eb

res-life changed the title ~~Support date_format via Gpu for non-UTC time zone~~ Support date_format via Gpu for non-UTC time zone [databricks] Dec 8, 2023

res-life requested a review from NVnavkumar December 8, 2023 10:48

winningsix reviewed Dec 12, 2023

View reviewed changes

winningsix previously approved these changes Dec 12, 2023

View reviewed changes

ttnghia reviewed Dec 12, 2023

View reviewed changes

Merge branch-24.02

f1314e0

res-life dismissed winningsix’s stale review via f1314e0 December 13, 2023 02:14

thirtiseven approved these changes Dec 14, 2023

View reviewed changes

res-life merged commit 782de5c into NVIDIA:branch-24.02 Dec 14, 2023
37 of 38 checks passed

res-life deleted the non-utc-date_format branch December 14, 2023 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support date_format via Gpu for non-UTC time zone [databricks] #9721

Support date_format via Gpu for non-UTC time zone [databricks] #9721

res-life commented Nov 15, 2023 •

edited

Loading

revans2 left a comment

res-life commented Nov 16, 2023

res-life commented Nov 21, 2023

res-life commented Nov 27, 2023

res-life commented Dec 5, 2023

res-life commented Dec 5, 2023

winningsix Dec 5, 2023

res-life Dec 5, 2023

res-life Dec 11, 2023

winningsix Dec 5, 2023

res-life Dec 5, 2023

winningsix Dec 6, 2023

res-life Dec 6, 2023

res-life Dec 11, 2023

res-life commented Dec 5, 2023

res-life commented Dec 6, 2023

res-life commented Dec 8, 2023

res-life commented Dec 8, 2023

winningsix Dec 12, 2023

res-life Dec 12, 2023

winningsix Dec 12, 2023

winningsix Dec 12, 2023

ttnghia Dec 12, 2023

NVnavkumar Dec 12, 2023

winningsix Dec 12, 2023

ttnghia Dec 12, 2023

res-life commented Dec 13, 2023

NVnavkumar commented Dec 14, 2023 •

edited

Loading

NVnavkumar commented Dec 14, 2023

Support date_format via Gpu for non-UTC time zone [databricks] #9721

Support date_format via Gpu for non-UTC time zone [databricks] #9721

Conversation

res-life commented Nov 15, 2023 • edited Loading

revans2 left a comment

Choose a reason for hiding this comment

res-life commented Nov 16, 2023

res-life commented Nov 21, 2023

res-life commented Nov 27, 2023

res-life commented Dec 5, 2023

res-life commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

res-life commented Dec 5, 2023

res-life commented Dec 6, 2023

res-life commented Dec 8, 2023

res-life commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

res-life commented Dec 13, 2023

NVnavkumar commented Dec 14, 2023 • edited Loading

NVnavkumar commented Dec 14, 2023

res-life commented Nov 15, 2023 •

edited

Loading

NVnavkumar commented Dec 14, 2023 •

edited

Loading