voonhous commented on code in PR #19475:
URL: https://github.com/apache/hudi/pull/19475#discussion_r4398752407
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/BaseHoodieCatalystExpressionUtils.scala:
##########
@@ -82,9 +82,9 @@ abstract class BaseHoodieCatalystExpressionUtils extends
HoodieCatalystExpressio
// Binary
case Add(OrderPreservingTransformation(attrRef), _, _) => Some(attrRef)
case Add(_, OrderPreservingTransformation(attrRef), _) => Some(attrRef)
- case Multiply(OrderPreservingTransformation(attrRef), _, _) =>
Some(attrRef)
- case Multiply(_, OrderPreservingTransformation(attrRef), _) =>
Some(attrRef)
- case Divide(OrderPreservingTransformation(attrRef), _, _) =>
Some(attrRef)
+ case Multiply(OrderPreservingTransformation(attrRef), factor, _) if
isPositiveNumericLiteral(factor) => Some(attrRef)
Review Comment:
Good catch. Multiply/Divide now only match a float/double result with a
positive, finite literal factor; integral results wrap and decimal results
overflow into NULL, so both are rejected. Your `{51, Long.MaxValue}` / `A * 2L
> 100` case is now a regression test in `TestDataSkippingUtils` (5d9213f).
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/HoodieSparkTypeUtils.scala:
##########
@@ -35,10 +36,20 @@ object HoodieSparkTypeUtils {
*/
def isCastPreservingOrdering(from: DataType, to: DataType): Boolean =
(from, to) match {
- // NOTE: In the casting rules defined by Spark, only casting from String
to Numeric
- // (and vice versa) are the only casts that might break the ordering of
the elements after casting
- case (StringType, _: NumericType) => false
- case (_: NumericType, StringType) => false
+ // NOTE: Casting between String and Numeric types re-orders elements
(for ex, "10" < "9"
+ // lexicographically). These arms must stay ahead of the numeric
arm below, since
+ // Cast.canUpCast treats atomic-to-string casts as legal up-casts
+ case (_: StringType, _: NumericType) => false
+ case (_: NumericType, _: StringType) => false
+ // NOTE: On Spark 4 StringType carries a collation (and constraint) and
its equals compares
+ // both; casting to a different collation changes the sort order.
On Spark 3
+ // StringType is a singleton, making this arm trivially true
+ case (fromStr: StringType, toStr: StringType) => fromStr == toStr
+ // NOTE: Narrowing numeric casts (for ex, bigint to int) overflow and
wrap around in
+ // non-ANSI mode, breaking ordering; only up-casts are guaranteed
order-preserving.
+ // This is conservative (for ex, double to float rounds
monotonically but is
+ // rejected), which only costs pruning opportunity, never
correctness
+ case (fromNum: NumericType, toNum: NumericType) =>
Cast.canUpCast(fromNum, toNum)
Review Comment:
Not intended. Added an explicit `decimal -> float/double` arm ahead of the
`canUpCast` one: rounding is monotonic and every decimal fits in float range.
Covered by a `CAST(A AS DECIMAL(20, 0)) > 100.5D` pruning case (5d9213f).
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/BaseHoodieCatalystExpressionUtils.scala:
##########
@@ -110,5 +110,24 @@ abstract class BaseHoodieCatalystExpressionUtils extends
HoodieCatalystExpressio
}
}
}
+
+ // Multiplying or dividing by a constant preserves ordering only when the
constant is a
+ // strictly positive numeric literal: negative factors reverse the
ordering, zero collapses
+ // it (and makes division undefined), and non-literal operands cannot be
validated
+ // statically (the optimizer folds constant factors to literals before
data skipping runs).
+ // Typed null literals carry a null value and fail the value match
+ private def isPositiveNumericLiteral(expr: Expression): Boolean = expr
match {
+ case Literal(value, _) => value match {
Review Comment:
Replaced with `isOrderPreservingScaling` and a shorter comment.
--
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]
