voonhous commented on code in PR #20521:
URL: https://github.com/apache/hudi/pull/20521#discussion_r4479531901
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestHoodieCatalystExpressionUtils.scala:
##########
@@ -125,6 +125,28 @@ class TestHoodieCatalystExpressionUtils extends
SparkAdapterSupport {
assertEquals(Some(strAttr), matched(new ParseToDate(strAttr)))
}
+ @Test
+ def testTimeOfDayPatternsRequireFixedOffsetSessionZone(): Unit = {
+ val newYork = Some("America/New_York")
+ // A time-of-day or offset field reorders values across a DST fall-back of
the session zone.
+ assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
HH:mm"), newYork)))
+ assertEquals(None, matched(FromUnixTime(longAttr, Literal("yyyy-MM-dd
HH:mm:ss"), newYork)))
+ assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
XXX"), newYork)))
+ // A date-only pattern renders the local date, which never runs backwards
in New York; neither quoted text nor
+ // a non-ASCII character is a field.
+ assertEquals(Some(tsAttr), matched(DateFormatClass(tsAttr,
Literal("yyyy-MM-dd"), newYork)))
+ assertEquals(Some(longAttr), matched(FromUnixTime(longAttr,
Literal("yyyy-MM-dd 'at' 'H'"), newYork)))
+ assertEquals(Some(tsAttr), matched(DateFormatClass(tsAttr,
Literal("yyyy\u5e74MM\u6708dd\u65e5"), newYork)))
+ // Any pattern keeps the ordering in a session zone whose offset never
changes.
+ assertEquals(Some(tsAttr), matched(DateFormatClass(tsAttr,
Literal("yyyy-MM-dd HH:mm"), Some("UTC"))))
+ assertEquals(Some(longAttr), matched(FromUnixTime(longAttr,
Literal("yyyy-MM-dd HH:mm:ss"), Some("+08:00"))))
+ // A non-literal or null pattern, an unknown zone or an unresolved session
zone is not matched.
+ assertEquals(None, matched(DateFormatClass(tsAttr, strAttr, newYork)))
+ assertEquals(None, matched(DateFormatClass(tsAttr, Literal(null,
StringType), newYork)))
+ assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
HH:mm"), Some("Not/AZone"))))
Review Comment:
**minor:** The new cases cannot catch two easy regressions: a greedy `'.*'`
quote strip still passes (it would call `"yyyy-MM-dd 'at' HH 'h'"` date-only),
and so does adding `a`, `h`, `z` or `V` to `DATE_ONLY_PATTERN_LETTERS`, since
only `H`, `m`, `s` and `X` are pinned. Also a reminder from #20519:
`Asia/Kolkata` is cited in the description but not asserted. Not blocking;
could we add these?
```suggestion
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
HH:mm"), Some("Not/AZone"))))
// A field outside the quotes, an escaped quote, 12-hour and zone-name
fields still reorder.
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
'at' HH 'h'"), newYork)))
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy''HH"),
newYork)))
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
hh a"), newYork)))
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
VV"), newYork)))
// PST resolves to America/Los_Angeles; Kolkata has no DST but
historical offset changes.
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
HH:mm"), Some("PST"))))
assertEquals(None, matched(DateFormatClass(tsAttr, Literal("yyyy-MM-dd
HH:mm"), Some("Asia/Kolkata"))))
```
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/dataskipping/rules/DateTimeFormatRule.scala:
##########
@@ -17,26 +17,58 @@
package org.apache.spark.sql.hudi.dataskipping.rules
-import org.apache.spark.sql.catalyst.expressions.{DateFormatClass, Expression,
FromUnixTime, ParseToDate, ParseToTimestamp, ToUnixTimestamp}
+import org.apache.spark.sql.catalyst.expressions.{DateFormatClass, Expression,
FromUnixTime, Literal, ParseToDate, ParseToTimestamp, ToUnixTimestamp}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.hudi.dataskipping.{Increasing, RangeRule, Step}
+import scala.util.Try
+
/**
* Date/time formatting and parsing: `DateFormatClass`, `FromUnixTime`,
`ToUnixTimestamp`,
* `ParseToDate` and `ParseToTimestamp`. The two parsing expressions have
different constructor
* arities across Spark versions, so they are matched by type and step into
`left` (the parsed
* input, the first constructor field on every supported version) rather than
by extractor.
*
- * Shared guard: none; ANY format is accepted today, though most (for ex,
'dd-MM-yyyy') do not
- * preserve ordering (#19481). Parsing is `partial`: unparseable input yields
NULL.
+ * Guard: `DateFormatClass` and `FromUnixTime` need [[isZoneSafePattern]];
past that, ANY format is
+ * accepted today, though most (for ex, 'dd-MM-yyyy') do not preserve ordering
(#19481). Parsing is
+ * `partial`: unparseable input yields NULL.
*/
object DateTimeFormatRule extends RangeRule {
override def steps(term: Expression): Seq[Step] = term match {
- case DateFormatClass(timestamp, _, _) => Seq(Step(term, timestamp,
Increasing, fileGuard = None, partial = false))
- case FromUnixTime(seconds, _, _) => Seq(Step(term, seconds, Increasing,
fileGuard = None, partial = false))
+ // NOTE: date_format and from_unixtime render the time in the session time
zone, so a pattern field that
+ // depends on the time of day or on the offset (for ex, HH, mm or
XXX) only keeps the ordering when the
+ // zone's offset never changes: local time runs backwards across a
DST fall-back (for ex, 05:30Z is
+ // 01:30 EDT but the later 06:10Z is 01:10 EST). A date-only pattern
is assumed unaffected: the local
+ // date only runs backwards where the clocks fall back across
midnight, which tzdb last records in 2010
+ // (America/St_Johns at 00:01)
+ case DateFormatClass(timestamp, pattern, timeZoneId) if
isZoneSafePattern(pattern, timeZoneId) =>
+ Seq(Step(term, timestamp, Increasing, fileGuard = None, partial = false))
+ case FromUnixTime(seconds, pattern, timeZoneId) if
isZoneSafePattern(pattern, timeZoneId) =>
+ Seq(Step(term, seconds, Increasing, fileGuard = None, partial = false))
case ToUnixTimestamp(time, _, _, _) => Seq(Step(term, time, Increasing,
fileGuard = None, partial = true))
case p: ParseToDate => Seq(Step(term, p.left, Increasing, fileGuard =
None, partial = true))
case p: ParseToTimestamp => Seq(Step(term, p.left, Increasing, fileGuard =
None, partial = true))
case _ => Nil
}
+
+ // Pattern letters that only render the local date: era, year, quarter,
month, week and day
+ private val DATE_ONLY_PATTERN_LETTERS: Set[Char] = "GuyYQqMLwWDdFEec".toSet
+
+ // Whether the session time zone cannot reorder what the pattern renders:
either the pattern only renders the
+ // local date, or the zone's offset never changes. Only a literal pattern is
recognized as date-only, since the
+ // optimizer folds a constant one to a literal before data skipping runs,
and neither quoted text nor a
+ // non-ASCII character is a field. An unresolved session zone is not fixed
+ private def isZoneSafePattern(pattern: Expression, timeZoneId:
Option[String]): Boolean = {
+ val isDateOnly = pattern match {
+ case Literal(value, _) if value != null =>
+ value.toString.replaceAll("'[^']*'", "").forall(c => !(c < 128 &&
c.isLetter) || DATE_ONLY_PATTERN_LETTERS.contains(c))
+ case _ => false
+ }
+ isDateOnly || timeZoneId.exists(isFixedOffsetZone)
+ }
+
+ // Whether the zone, resolved the way Spark evaluates it, never changes its
offset. An unknown zone is not fixed
+ private def isFixedOffsetZone(zoneId: String): Boolean =
Review Comment:
**minor:** Reminder of the #20519 thread on sharing this helper, now with a
third copy: #19474 adds `DateTimeOrdering.isFixedOffsetZone` and already
classifies patterns with `Pattern.hasTimeOfDay`, so this letter scan would be a
second classifier. The two PRs conflict in this file and in
`TestHoodieCatalystExpressionUtils.scala`. Not blocking; if #19474 lands first,
could this guard reduce to `!p.hasTimeOfDay || isFixedOffsetZone(tz)` over one
shared helper?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/dataskipping/rules/DateTimeFormatRule.scala:
##########
@@ -17,26 +17,58 @@
package org.apache.spark.sql.hudi.dataskipping.rules
-import org.apache.spark.sql.catalyst.expressions.{DateFormatClass, Expression,
FromUnixTime, ParseToDate, ParseToTimestamp, ToUnixTimestamp}
+import org.apache.spark.sql.catalyst.expressions.{DateFormatClass, Expression,
FromUnixTime, Literal, ParseToDate, ParseToTimestamp, ToUnixTimestamp}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.hudi.dataskipping.{Increasing, RangeRule, Step}
+import scala.util.Try
+
/**
* Date/time formatting and parsing: `DateFormatClass`, `FromUnixTime`,
`ToUnixTimestamp`,
* `ParseToDate` and `ParseToTimestamp`. The two parsing expressions have
different constructor
* arities across Spark versions, so they are matched by type and step into
`left` (the parsed
* input, the first constructor field on every supported version) rather than
by extractor.
*
- * Shared guard: none; ANY format is accepted today, though most (for ex,
'dd-MM-yyyy') do not
- * preserve ordering (#19481). Parsing is `partial`: unparseable input yields
NULL.
+ * Guard: `DateFormatClass` and `FromUnixTime` need [[isZoneSafePattern]];
past that, ANY format is
+ * accepted today, though most (for ex, 'dd-MM-yyyy') do not preserve ordering
(#19481). Parsing is
+ * `partial`: unparseable input yields NULL.
*/
object DateTimeFormatRule extends RangeRule {
override def steps(term: Expression): Seq[Step] = term match {
- case DateFormatClass(timestamp, _, _) => Seq(Step(term, timestamp,
Increasing, fileGuard = None, partial = false))
- case FromUnixTime(seconds, _, _) => Seq(Step(term, seconds, Increasing,
fileGuard = None, partial = false))
+ // NOTE: date_format and from_unixtime render the time in the session time
zone, so a pattern field that
+ // depends on the time of day or on the offset (for ex, HH, mm or
XXX) only keeps the ordering when the
+ // zone's offset never changes: local time runs backwards across a
DST fall-back (for ex, 05:30Z is
+ // 01:30 EDT but the later 06:10Z is 01:10 EST). A date-only pattern
is assumed unaffected: the local
+ // date only runs backwards where the clocks fall back across
midnight, which tzdb last records in 2010
+ // (America/St_Johns at 00:01)
+ case DateFormatClass(timestamp, pattern, timeZoneId) if
isZoneSafePattern(pattern, timeZoneId) =>
+ Seq(Step(term, timestamp, Increasing, fileGuard = None, partial = false))
+ case FromUnixTime(seconds, pattern, timeZoneId) if
isZoneSafePattern(pattern, timeZoneId) =>
+ Seq(Step(term, seconds, Increasing, fileGuard = None, partial = false))
case ToUnixTimestamp(time, _, _, _) => Seq(Step(term, time, Increasing,
fileGuard = None, partial = true))
case p: ParseToDate => Seq(Step(term, p.left, Increasing, fileGuard =
None, partial = true))
case p: ParseToTimestamp => Seq(Step(term, p.left, Increasing, fileGuard =
None, partial = true))
case _ => Nil
}
+
+ // Pattern letters that only render the local date: era, year, quarter,
month, week and day
+ private val DATE_ONLY_PATTERN_LETTERS: Set[Char] = "GuyYQqMLwWDdFEec".toSet
+
+ // Whether the session time zone cannot reorder what the pattern renders:
either the pattern only renders the
+ // local date, or the zone's offset never changes. Only a literal pattern is
recognized as date-only, since the
+ // optimizer folds a constant one to a literal before data skipping runs,
and neither quoted text nor a
+ // non-ASCII character is a field. An unresolved session zone is not fixed
+ private def isZoneSafePattern(pattern: Expression, timeZoneId:
Option[String]): Boolean = {
+ val isDateOnly = pattern match {
+ case Literal(value, _) if value != null =>
+ value.toString.replaceAll("'[^']*'", "").forall(c => !(c < 128 &&
c.isLetter) || DATE_ONLY_PATTERN_LETTERS.contains(c))
+ case _ => false
+ }
+ isDateOnly || timeZoneId.exists(isFixedOffsetZone)
Review Comment:
**minor:** Reminder, same as the `TimeZoneShiftRule` thread on #20519:
`isFixedOffset` is false for every region zone, so time-of-day patterns now
never prune in `Asia/Shanghai` (last change 1991), `Asia/Tokyo` (1951) or
`Asia/Kolkata` (1945) either. Sound, so not blocking. The #20449 follow-up note
(https://github.com/apache/hudi/issues/20449#issuecomment-6654220058) only
names `TimeZoneShiftRule`; could we extend it to these two arms, where the same
"`lo` after the zone's last transition" `fileGuard` would apply once #19474
lands?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/dataskipping/rules/DateTimeFormatRule.scala:
##########
@@ -17,26 +17,58 @@
package org.apache.spark.sql.hudi.dataskipping.rules
-import org.apache.spark.sql.catalyst.expressions.{DateFormatClass, Expression,
FromUnixTime, ParseToDate, ParseToTimestamp, ToUnixTimestamp}
+import org.apache.spark.sql.catalyst.expressions.{DateFormatClass, Expression,
FromUnixTime, Literal, ParseToDate, ParseToTimestamp, ToUnixTimestamp}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.hudi.dataskipping.{Increasing, RangeRule, Step}
+import scala.util.Try
+
/**
* Date/time formatting and parsing: `DateFormatClass`, `FromUnixTime`,
`ToUnixTimestamp`,
* `ParseToDate` and `ParseToTimestamp`. The two parsing expressions have
different constructor
* arities across Spark versions, so they are matched by type and step into
`left` (the parsed
* input, the first constructor field on every supported version) rather than
by extractor.
*
- * Shared guard: none; ANY format is accepted today, though most (for ex,
'dd-MM-yyyy') do not
- * preserve ordering (#19481). Parsing is `partial`: unparseable input yields
NULL.
+ * Guard: `DateFormatClass` and `FromUnixTime` need [[isZoneSafePattern]];
past that, ANY format is
+ * accepted today, though most (for ex, 'dd-MM-yyyy') do not preserve ordering
(#19481). Parsing is
+ * `partial`: unparseable input yields NULL.
*/
object DateTimeFormatRule extends RangeRule {
override def steps(term: Expression): Seq[Step] = term match {
- case DateFormatClass(timestamp, _, _) => Seq(Step(term, timestamp,
Increasing, fileGuard = None, partial = false))
- case FromUnixTime(seconds, _, _) => Seq(Step(term, seconds, Increasing,
fileGuard = None, partial = false))
+ // NOTE: date_format and from_unixtime render the time in the session time
zone, so a pattern field that
+ // depends on the time of day or on the offset (for ex, HH, mm or
XXX) only keeps the ordering when the
+ // zone's offset never changes: local time runs backwards across a
DST fall-back (for ex, 05:30Z is
+ // 01:30 EDT but the later 06:10Z is 01:10 EST). A date-only pattern
is assumed unaffected: the local
+ // date only runs backwards where the clocks fall back across
midnight, which tzdb last records in 2010
+ // (America/St_Johns at 00:01)
Review Comment:
**nit:** The date-only assumption has an open tracker: #20525 covers the
fall-backs across midnight (St_Johns, Goose_Bay, through 2010) that this leaves
out. Feel free to ignore; could the NOTE point at it?
```suggestion
// (America/St_Johns at 00:01; see #20525)
```
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestDataSkippingUtils.scala:
##########
@@ -122,6 +122,36 @@ class TestDataSkippingUtils extends
HoodieSparkClientTestBase with SparkAdapterS
Seq("file_1"))
}
+ @Test
+ def testTimeOfDayPatternsAcrossDstFallBack(): Unit = {
+ spark.sqlContext.setConf(SESSION_LOCAL_TIMEZONE.key, "America/New_York")
+ try {
+ // 05:30Z renders as 01:30 EDT but the later 06:10Z as 01:10 EST, so a
pattern with the time of day puts
+ // the max below the min. Re-applying it 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
+ val fallBack = IndexRow("file_1", valueCount = 2,
+ A_minValue = 1793511000L, A_maxValue = 1793513400L, A_nullCount = 0,
// epoch seconds of C's bounds
+ C_minValue = new Timestamp(1793511000000L), // 2026-11-01 05:30Z
Review Comment:
**nit:** Reminder, as on #20519: this re-inlines the fall-back fixture
(`1793511000000L` / `1793513400000L`) and the New York set/reset block of
`testTimestampToStringCastAcrossDstFallBack` (line 171). Feel free to ignore;
could these asserts join that test, using the `NyFallBackEdt0130` /
`NyFallBackEst0110` constants from #20519 once it lands?
--
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]
