wombatu-kun commented on code in PR #20519:
URL: https://github.com/apache/hudi/pull/20519#discussion_r4478640515
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/dataskipping/rules/TimeZoneShiftRule.scala:
##########
@@ -17,22 +17,39 @@
package org.apache.spark.sql.hudi.dataskipping.rules
-import org.apache.spark.sql.catalyst.expressions.{Expression,
FromUTCTimestamp, ToUTCTimestamp}
+import org.apache.spark.sql.catalyst.expressions.{Expression,
FromUTCTimestamp, Literal, ToUTCTimestamp}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.hudi.dataskipping.{Increasing, RangeRule, Step}
+import scala.util.Try
+
/**
* Time zone shifts: `FromUTCTimestamp`, `ToUTCTimestamp`.
*
- * Shared guard: none; any time zone operand is accepted today.
+ * Shared guard: [[isFixedOffsetTimeZone]], a literal time zone whose offset
never changes.
*
- * NOTE: The offset jumps at DST transitions, so the shift is not monotonic
across a fall-back (#20449)
+ * NOTE: Both shift by the zone's offset at that time, so they only keep the
ordering for a zone whose offset never
+ * changes, DST or historical. Local time runs backwards when the offset
drops (for ex, 05:30Z is 01:30 EDT but
+ * the later 06:10Z is 01:10 EST), and a local time in the gap of a
rising offset shifts past later ones (for ex,
+ * 02:30 maps to 07:30Z but the later 03:00 to 07:00Z)
*/
object TimeZoneShiftRule extends RangeRule {
Review Comment:
No port planned; noted in #20518:
https://github.com/apache/hudi/issues/20518#issuecomment-6654328573
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestHoodieCatalystExpressionUtils.scala:
##########
@@ -117,6 +117,23 @@ class TestHoodieCatalystExpressionUtils extends
SparkAdapterSupport {
assertEquals(Some(dateAttr), matched(DateDiff(dateAttr,
Literal(java.sql.Date.valueOf("2022-03-07")))))
}
+ @Test
+ def testUtcTimestampConversionsRequireFixedOffsetZone(): Unit = {
+ // A zone with DST runs local time backwards across a fall-back and shifts
the spring-forward gap.
+ assertEquals(None, matched(FromUTCTimestamp(tsAttr,
Literal("America/New_York"))))
+ assertEquals(None, matched(ToUTCTimestamp(tsAttr,
Literal("America/New_York"))))
Review Comment:
Keeping it, so this test pins both functions on both sides of the zone check
in one place.
--
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]
