wombatu-kun commented on code in PR #20521:
URL: https://github.com/apache/hudi/pull/20521#discussion_r4480317739
##########
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:
Done 5d2de096d593: both rules now call `TimeZones.isFixedOffsetZone`, so
#19474 can drop its private copy. If #19474 lands first, the rebase here
reduces the letter scan to `!p.hasTimeOfDay`; if this lands first, #19474's
rebase can.
##########
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:
Done 5d2de096d593; the joined test is renamed
`testSessionZoneConversionsAcrossDstFallBack`, since it no longer covers only
casts.
--
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]
