comphead commented on code in PR #6353:
URL: https://github.com/apache/datafusion-comet/pull/6353#discussion_r4146852194
##########
docs/source/user-guide/latest/compatibility/expressions/_category_template/datetime.md:
##########
@@ -42,6 +42,16 @@ If you need to process dates far in the future with accurate
timezone handling,
- Using timezone-naive types (`timestamp_ntz`) when timezone conversion is not
required
- Falling back to Spark for these specific operations
+### Timezone Database Versions
+
+Comet's native code converts between instants and local time with the IANA
timezone database that
+chrono-tz compiles into the Comet library. Spark uses the JVM's timezone
database (`tzdb.dat`), whose
+version depends on the JDK build and on whether its timezone data has been
updated. When the two versions
+have different rules for a timezone, local times that Comet computes natively
for that timezone can differ
+from Spark's for the affected dates. That covers `hour`, casts between
timestamps and strings or dates,
+`date_trunc`, and parsing strings as timestamps. Comet logs a warning at
startup when the two versions
Review Comment:
Nit: the `date_trunc` bullet at the top of this page says non-UTC sessions
go through the JVM codegen dispatcher by default. So I'd expect native
`date_trunc` to be affected only with
`spark.comet.expression.TruncTimestamp.allowIncompatible=true`. Also, "parsing
strings as timestamps" looks covered by the casts already. Would it make sense
to tighten this list?
##########
spark/src/main/java/org/apache/comet/NativeBase.java:
##########
@@ -177,6 +179,35 @@ private static void initWithLogConf() {
init(logConfPath, logLevel);
}
+ /**
+ * Native code converts between instants and local time with the IANA
timezone database that
+ * chrono-tz compiles into libcomet, while Spark uses the JVM's. When the
two versions differ,
+ * local times in timezones whose rules changed between them can differ from
Spark's.
+ */
+ private static void warnOnTzdataMismatch() {
+ try {
+ String warning =
+ tzdataMismatchWarning(getTzdataVersion(),
ZoneRulesProvider.getVersions("UTC").lastKey());
+ if (warning != null) {
+ LOG.warn(warning);
+ }
+ } catch (Throwable t) {
+ LOG.debug("Could not compare timezone database versions", t);
+ }
+ }
+
+ /** The warning to log when native code and the JVM use different tzdata
versions, or null. */
+ static String tzdataMismatchWarning(String nativeVersion, String jvmVersion)
{
+ if (nativeVersion.equals(jvmVersion)) {
+ return null;
+ }
+ return String.format(
+ "Comet's native library uses timezone database %s, but the JVM uses
%s. Local times that "
+ + "Comet computes natively can differ from Spark's in timezones
whose rules changed "
+ + "between these versions.",
Review Comment:
JDK builds ship different tzdata versions. For example, the `tzdb.dat`
headers of the JDKs I have installed say 2024b and 2026a, so I'd expect this
warning on most clusters (I haven't run it). Would it make sense to end the
message with a pointer to the "Timezone Database Versions" docs section, and
let that section say what users can do about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
