Fix the time zone check issue

[firestarman]

Fixes #5678

There are two settings for time zone, the Spark session local time zone and JVM's default time zone. And they may be different from each other according to issue #5678.

With this PR, plugin now requires both of the two settings being equal to UTC to support timestamp type.

Signed-off-by: Firestarman firestarmanllc@gmail.com

[firestarman]

For early review

[tgravescs]

did you mean this to be against 22.06? Unless specifically requested this should go against 22.08

[firestarman]

did you mean this to be against 22.06? Unless specifically requested this should go against 22.08

It is a bug fix, so I thought it should go into 22.06. Rebased to 22.08

[jlowe]

build

[gerashegalov]

The change looks good as a narrow fix.

However, the logic for timezone error handling in the plugin initialization is not quire clear

• first we stash away the initial value of the default timezone on the driver

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

  Line 141 in 0b2eeb1

  conf.set(RapidsConf.DRIVER_TIMEZONE.key, ZoneId.systemDefault().normalized().toString)

  .
• We don't throw an exception on the driver, if it's not a supported time zone, which I think is OK because it can still be changed for the JVM using java.util.TimeZone.setDefault
• Then on the executor side plugin init we throw an exception on discrepancy between the driver and executor default timezones but only if the driver timezone is UTC

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

  Line 221 in aba390e

  throw new RuntimeException(s" Driver and executor timezone mismatch. " +

  This does not make sense to me. Either always throw when timezones are different. Or adjust the executor timezone with TimeZone.setDefault(TimeZone.getTimeZone(driverTimeZone)) unless it is explicitly overridden with user.timezone in executor Java options

We need to file the issue to clarify this if it's beyond the current PR.

[gerashegalov]

LGTM with caveats regarding Plugin init checks

[firestarman]

We need to file the issue to clarify this if it's beyond the current PR.

Personally the current time zone checks in Plugin init are OK. (Please correct me if I am wrong)

At the init stage on the driver, Plugin does not know whether the time zone will be used. So it is not necessary to check the time zone here. And this check will be executed during the time related operators' overriding process later.

On executors, the time zone is required to be equal to the driver's only for the time related GPU operators, I think. If time zone is not supported, no time related GPU operators exist (all fall back to CPU), then no need to check the difference of the time zones.

[firestarman]

I am going to merge this, if any issue, we can follow up in new issues.

[gerashegalov]

We need to file the issue to clarify this if it's beyond the current PR.

Personally the current time zone checks in Plugin init are OK. (Please correct me if I am wrong)

At the init stage on the driver, Plugin does not know whether the time zone will be used. So it is not necessary to check the time zone here. And this check will be executed during the time related operators' overriding process later.

On executors, the time zone is required to be equal to the driver's only for the time related GPU operators, I think. If time zone is not supported, no time related GPU operators exist (all fall back to CPU), then no need to check the difference of the time zones.

So looks like you are arguing for dropping the check

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

Line 221 in aba390e

throw new RuntimeException(s" Driver and executor timezone mismatch. " +

because it's an init-time not a query-time check.

Also again the default timezone of the driver as of init time may be different at the query time. I'll file an issue to clarify these limitations.