hudi-agent commented on code in PR #19474:
URL: https://github.com/apache/hudi/pull/19474#discussion_r3702482028
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/BaseHoodieCatalystExpressionUtils.scala:
##########
@@ -53,7 +53,11 @@ abstract class BaseHoodieCatalystExpressionUtils extends
HoodieCatalystExpressio
* Matches order-preserving date/time parsing expressions whose case-class
shapes differ across
* Spark versions (currently
[[org.apache.spark.sql.catalyst.expressions.ParseToDate]] and
* [[org.apache.spark.sql.catalyst.expressions.ParseToTimestamp]]),
returning the source child
- * expression that the order-preserving transformation matching should
recurse into
+ * expression that the order-preserving transformation matching should
recurse into.
+ *
+ * These are the pre-ReplaceExpressions (analysis-time) shapes, still
inspected by the
+ * expression-index and partition-stats paths; the post-replacement
[[GetTimestamp]] shape
+ * seen on the read path is matched directly in
[[OrderPreservingTransformation]]
Review Comment:
🤖 nit: this sentence is missing a trailing period — could you add one after
`[[OrderPreservingTransformation]]` to match the style of the rest of the
Scaladoc block?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/BaseHoodieCatalystExpressionUtils.scala:
##########
@@ -70,6 +74,12 @@ abstract class BaseHoodieCatalystExpressionUtils extends
HoodieCatalystExpressio
case FromUTCTimestamp(OrderPreservingTransformation(attrRef), _) =>
Some(attrRef)
case ToUnixTimestamp(OrderPreservingTransformation(attrRef), _, _, _)
=> Some(attrRef)
case ToUTCTimestamp(OrderPreservingTransformation(attrRef), _) =>
Some(attrRef)
+ // Post-ReplaceExpressions shape of to_date/to_timestamp with an
explicit format:
+ // since SPARK-38240 the optimizer rewrites those RuntimeReplaceable
nodes into
+ // GetTimestamp(source, fmt) (Cast-wrapped to date for to_date) before
filters
+ // reach file pruning. GetTimestamp's constructor arity differs across
Spark
+ // versions but its left() accessor is stable, hence the typed match
+ case gt: GetTimestamp => unapply(gt.left)
Review Comment:
🤖 This treats `GetTimestamp(source, fmt)` as order-preserving for any `fmt`,
which newly enables real min/max pruning on the read path (before this it fell
through to TrueLiteral = no skip). For a format where lexicographic string
order diverges from chronological order — e.g. `to_timestamp(B, 'dd-MM-yyyy')`
where '02-01-2020' < '01-01-2023' lexically but is later chronologically —
could pruning on the string column's min/max stats now incorrectly skip a file
that actually matches? I realize this mirrors how the analysis-time
ParseTo*/DateFormatClass arms already behave, so if that assumption is
considered safe elsewhere this is just parity — but since it's now reachable
for column-stats skipping on the read path, wanted to confirm it's intentional.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
