wombatu-kun commented on code in PR #20521:
URL: https://github.com/apache/hudi/pull/20521#discussion_r4480315744
##########
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:
Done 5d2de096d593. Besides the suggested lines, each non-date letter now
gets its own pattern, since `hh a` stays time-of-day when only one of `h` and
`a` joins the set.
##########
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:
Extended the #20449 note to both arms:
https://github.com/apache/hudi/issues/20449#issuecomment-6654220058
--
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]
