voonhous commented on code in PR #20519:
URL: https://github.com/apache/hudi/pull/20519#discussion_r4478029908
##########
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"))))
+ // A fixed-offset zone shifts every timestamp by the same amount.
+ assertEquals(Some(tsAttr), matched(FromUTCTimestamp(tsAttr,
Literal("+08:00"))))
+ assertEquals(Some(tsAttr), matched(ToUTCTimestamp(tsAttr, Literal("UTC"))))
+ // Zones are resolved the way Spark evaluates them: the short ID EST is a
fixed -05:00, and +8:00 is +08:00.
+ assertEquals(Some(tsAttr), matched(FromUTCTimestamp(tsAttr,
Literal("EST"))))
Review Comment:
**minor:** `EST` is a fixed -05:00 only up to JDK 23. From JDK 24,
`ZoneId.SHORT_IDS` maps it to `America/Panama`, which is not fixed-offset, so
this assertion fails there. CI runs JDK 11/17 and production only loses
pruning, so not blocking.
Evidence: `ZoneId.of("EST", ZoneId.SHORT_IDS)` is `-05:00` on 17.0.14 and
21.0.9, `America/Panama` on 25.0.1.
Could we use `Etc/GMT+5` here, and drop the EST example from the comment in
`TimeZoneShiftRule` (line 48)?
```suggestion
// Zones are resolved the way Spark evaluates them: Etc/GMT+5 is a fixed
-05:00, and +8:00 is +08:00.
assertEquals(Some(tsAttr), matched(FromUTCTimestamp(tsAttr,
Literal("Etc/GMT+5"))))
```
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestDataSkippingUtils.scala:
##########
@@ -914,7 +914,46 @@ object TestDataSkippingUtils {
IndexRow("file_1", valueCount = 2, C_minValue = new
Timestamp(1000L), C_maxValue = new Timestamp(50000L), C_nullCount = 0),
IndexRow("file_2", valueCount = 2, C_minValue = new
Timestamp(101000L), C_maxValue = new Timestamp(200000L), C_nullCount = 0)
),
- Seq("file_2"))
+ Seq("file_2")),
+ // New York local time runs backwards across the DST fall-back: 05:30Z
is 01:30 EDT, but the later
+ // 06:10Z is 01:10 EST. Re-applying from_utc_timestamp over min/max
stats would wrongly prune
+ // file_1 even though its 05:30Z row matches; must fall back to
always-true, keeping the file
+ arguments(
+ "from_utc_timestamp(C, 'America/New_York') > TIMESTAMP '2026-11-01
01:20:00'",
Review Comment:
**minor:** The description says the partition stats index is fixed too. That
holds, since it goes through the same translator, but no test drives partition
stats with a DST zone; only the open oracle PR #20439 would. Not blocking, and
#19475 / #20458 did not add one either.
Could we add the #20518 repro as a SQL test over a real table (for ex in
`TestDataSkippingQuery`), or note in the description that partition stats
coverage comes from #20439?
##########
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 {
override def steps(term: Expression): Seq[Step] = term match {
- case FromUTCTimestamp(timestamp, _) => Seq(Step(term, timestamp,
Increasing, fileGuard = None, partial = false))
- case ToUTCTimestamp(timestamp, _) => Seq(Step(term, timestamp, Increasing,
fileGuard = None, partial = false))
+ case FromUTCTimestamp(timestamp, timeZone) if
isFixedOffsetTimeZone(timeZone) =>
+ Seq(Step(term, timestamp, Increasing, fileGuard = None, partial = false))
+ case ToUTCTimestamp(timestamp, timeZone) if
isFixedOffsetTimeZone(timeZone) =>
+ Seq(Step(term, timestamp, Increasing, fileGuard = None, partial = false))
case _ => Nil
}
+
+ // Only a literal zone is matched, since the optimizer folds a constant zone
to a literal before data
+ // skipping runs. It is resolved the way Spark evaluates it, so for ex the
short ID EST is a fixed
+ // -05:00 while PST is America/Los_Angeles; an unknown zone is not matched
+ private def isFixedOffsetTimeZone(expr: Expression): Boolean = expr match {
Review Comment:
**nit:** #20521 adds the same
`Try(DateTimeUtils.getZoneId(zoneId).getRules.isFixedOffset).getOrElse(false)`,
as `isFixedOffsetZone` in `DateTimeFormatRule`. Feel free to ignore here; could
whichever of the two merges second move it into one shared helper in the
`dataskipping` package that both rules call?
##########
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.
Review Comment:
**nit:** The description's changelog still says the change is in
`BaseHoodieCatalystExpressionUtils`; after the rebase onto #20524 it lives
here, in `rules/TimeZoneShiftRule.scala`. Feel free to ignore; could we update
that bullet so the description matches the diff?
##########
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"))))
+ // A fixed-offset zone shifts every timestamp by the same amount.
+ assertEquals(Some(tsAttr), matched(FromUTCTimestamp(tsAttr,
Literal("+08:00"))))
+ assertEquals(Some(tsAttr), matched(ToUTCTimestamp(tsAttr, Literal("UTC"))))
+ // Zones are resolved the way Spark evaluates them: the short ID EST is a
fixed -05:00, and +8:00 is +08:00.
+ assertEquals(Some(tsAttr), matched(FromUTCTimestamp(tsAttr,
Literal("EST"))))
+ assertEquals(Some(tsAttr), matched(ToUTCTimestamp(tsAttr,
Literal("+8:00"))))
+ // Non-literal, null and unknown zones are not matched.
+ assertEquals(None, matched(FromUTCTimestamp(tsAttr, strAttr)))
+ assertEquals(None, matched(FromUTCTimestamp(tsAttr, Literal(null,
StringType))))
+ assertEquals(None, matched(ToUTCTimestamp(tsAttr, Literal("Not/AZone"))))
Review Comment:
**nit:** The description cites `Asia/Kolkata` (no DST, only historical
offset changes) as no longer pruning and `GMT+8` as still pruning, but neither
is asserted, and Kolkata is the least obvious `isFixedOffset` case. Feel free
to ignore; could we pin both?
```suggestion
assertEquals(None, matched(ToUTCTimestamp(tsAttr, Literal("Not/AZone"))))
// A zone with historical offset changes but no DST is not fixed-offset
either; GMT+8 is.
assertEquals(None, matched(FromUTCTimestamp(tsAttr,
Literal("Asia/Kolkata"))))
assertEquals(Some(tsAttr), matched(ToUTCTimestamp(tsAttr,
Literal("GMT+8"))))
```
##########
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 {
override def steps(term: Expression): Seq[Step] = term match {
- case FromUTCTimestamp(timestamp, _) => Seq(Step(term, timestamp,
Increasing, fileGuard = None, partial = false))
- case ToUTCTimestamp(timestamp, _) => Seq(Step(term, timestamp, Increasing,
fileGuard = None, partial = false))
+ case FromUTCTimestamp(timestamp, timeZone) if
isFixedOffsetTimeZone(timeZone) =>
+ Seq(Step(term, timestamp, Increasing, fileGuard = None, partial = false))
+ case ToUTCTimestamp(timestamp, timeZone) if
isFixedOffsetTimeZone(timeZone) =>
+ Seq(Step(term, timestamp, Increasing, fileGuard = None, partial = false))
case _ => Nil
}
+
+ // Only a literal zone is matched, since the optimizer folds a constant zone
to a literal before data
+ // skipping runs. It is resolved the way Spark evaluates it, so for ex the
short ID EST is a fixed
+ // -05:00 while PST is America/Los_Angeles; an unknown zone is not matched
+ private def isFixedOffsetTimeZone(expr: Expression): Boolean = expr match {
+ case Literal(timeZone, _) if timeZone != null =>
+
Try(DateTimeUtils.getZoneId(timeZone.toString).getRules.isFixedOffset).getOrElse(false)
Review Comment:
**minor:** `isFixedOffset` is false for every region ID, so zones whose
offset last changed decades ago now never prune either: `Asia/Shanghai` (1991),
`Asia/Tokyo` (1951), `Asia/Kolkata` (1945). That is sound and intended, so not
blocking.
Could we note a follow-up under #20449: once #19474 makes NULL bounds keep
the file, a `fileGuard` such as "`lo` is after the zone's last transition"
could keep pruning for zones with no future transition rules?
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestDataSkippingUtils.scala:
##########
@@ -914,7 +914,46 @@ object TestDataSkippingUtils {
IndexRow("file_1", valueCount = 2, C_minValue = new
Timestamp(1000L), C_maxValue = new Timestamp(50000L), C_nullCount = 0),
IndexRow("file_2", valueCount = 2, C_minValue = new
Timestamp(101000L), C_maxValue = new Timestamp(200000L), C_nullCount = 0)
),
- Seq("file_2"))
+ Seq("file_2")),
+ // New York local time runs backwards across the DST fall-back: 05:30Z
is 01:30 EDT, but the later
+ // 06:10Z is 01:10 EST. Re-applying from_utc_timestamp over min/max
stats would wrongly prune
+ // file_1 even though its 05:30Z row matches; must fall back to
always-true, keeping the file
+ arguments(
+ "from_utc_timestamp(C, 'America/New_York') > TIMESTAMP '2026-11-01
01:20:00'",
+ Seq(
+ IndexRow("file_1", valueCount = 2,
+ C_minValue = new Timestamp(1793511000000L), // 2026-11-01 05:30Z
+ C_maxValue = new Timestamp(1793513400000L), // 2026-11-01 06:10Z
Review Comment:
**nit:** These two fall-back timestamps now appear three times in this file
(lines 149-150, here, and 948-949). Feel free to ignore; could we hoist them
into named constants in `object TestDataSkippingUtils` (for ex
`NyFallBackEdt0130` / `NyFallBackEst0110`) shared by the three cases?
##########
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:
**nit:** #20518 says `release-1.2.1` and `release-1.1.2` carry the same
arms, but neither it nor this PR says whether a backport is planned. Those
branches still have the pre-#20524 matcher, so this will not cherry-pick
cleanly. Feel free to ignore; could we note in #20518 whether a 1.2.x port is
planned?
##########
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:
**nit:** The regenerated golden file already pins `to_utc_timestamp(ts,
'America/New_York')` as TRUE in both modes (`golden-index-filters.txt`, the 9
flipped lines), so this assertion repeats it. Feel free to ignore, it reads
fine as the negative next to the positive cases; could we drop it if you prefer
one place for 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]
