hudi-agent commented on code in PR #19836:
URL: https://github.com/apache/hudi/pull/19836#discussion_r3947966138
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -521,11 +521,15 @@ object HoodieProcedureFilterUtils {
right: org.apache.spark.sql.catalyst.expressions.Expression,
constructor:
(org.apache.spark.sql.catalyst.expressions.Expression,
org.apache.spark.sql.catalyst.expressions.Expression) => T,
original: T): T = {
- (left, right) match {
- case (boundRef:
org.apache.spark.sql.catalyst.expressions.BoundReference, literal:
org.apache.spark.sql.catalyst.expressions.Literal)
- if boundRef.dataType == org.apache.spark.sql.types.LongType &&
literal.dataType == org.apache.spark.sql.types.IntegerType =>
- val castExpr =
org.apache.spark.sql.catalyst.expressions.Cast(boundRef,
org.apache.spark.sql.types.IntegerType)
- constructor(castExpr, literal)
+ (left.dataType, right.dataType) match {
+ case (_: NumericType, _: NumericType) =>
Review Comment:
🤖 Since this now handles all numeric pairs, is it worth wiring the same
coercion into `EqualNullSafe` too? `ts <=> 1000` on a Long column still hits
the un-coerced path, and the ordering comparison of a boxed Long against a
boxed Int throws, which the per-row `Try` swallows into a dropped row.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -496,11 +496,15 @@ object HoodieProcedureFilterUtils {
right: org.apache.spark.sql.catalyst.expressions.Expression,
constructor:
(org.apache.spark.sql.catalyst.expressions.Expression,
org.apache.spark.sql.catalyst.expressions.Expression) => T,
original: T): T = {
- (left, right) match {
- case (boundRef:
org.apache.spark.sql.catalyst.expressions.BoundReference, literal:
org.apache.spark.sql.catalyst.expressions.Literal)
- if boundRef.dataType == org.apache.spark.sql.types.LongType &&
literal.dataType == org.apache.spark.sql.types.IntegerType =>
- val castExpr =
org.apache.spark.sql.catalyst.expressions.Cast(boundRef,
org.apache.spark.sql.types.IntegerType)
- constructor(castExpr, literal)
+ (left.dataType, right.dataType) match {
Review Comment:
🤖 Confirmed the mechanism from the source: `concat` isn't in the
function-resolution whitelist here (only upper/lower/length/trim/abs/... are),
so it stays an `UnresolvedFunction`, and `Or.eval` returns early on a true left
child — which is why only OR regresses (AND lands on `false` either way, before
and after). `resolved` is the right guard since it's recursive, so it also
covers nesting like `ts > abs(concat(name, 'x'))` where the top-level operand
isn't itself an `Unresolved*` node; alternatively wrapping the coercion body in
`Try(...).getOrElse(original)` would cover any node whose `dataType` throws.
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -496,11 +496,15 @@ object HoodieProcedureFilterUtils {
right: org.apache.spark.sql.catalyst.expressions.Expression,
constructor:
(org.apache.spark.sql.catalyst.expressions.Expression,
org.apache.spark.sql.catalyst.expressions.Expression) => T,
original: T): T = {
- (left, right) match {
- case (boundRef:
org.apache.spark.sql.catalyst.expressions.BoundReference, literal:
org.apache.spark.sql.catalyst.expressions.Literal)
- if boundRef.dataType == org.apache.spark.sql.types.LongType &&
literal.dataType == org.apache.spark.sql.types.IntegerType =>
- val castExpr =
org.apache.spark.sql.catalyst.expressions.Cast(boundRef,
org.apache.spark.sql.types.IntegerType)
- constructor(castExpr, literal)
+ (left.dataType, right.dataType) match {
+ case (_: NumericType, _: NumericType) =>
+ TypeCoercion.findWiderTypeForTwo(left.dataType, right.dataType)
Review Comment:
🤖 Worth weighing which way we'd rather this fail: `AnsiTypeCoercion` gives
parity with what the engine's analyzer would do, but as you point out it picks
FLOAT for BIGINT/FLOAT, which is strictly less accurate than the DOUBLE
`TypeCoercion` yields — and this util evaluates the predicate itself rather
than feeding the analyzer, so precision may matter more than parity here.
Either way, one thing to keep in mind is that `Cast`'s default `evalMode` is
derived from `SQLConf.get`, so under ANSI an overflowing coercion throws and
the per-row `Try` silently drops the row instead of producing null.
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -496,11 +496,15 @@ object HoodieProcedureFilterUtils {
right: org.apache.spark.sql.catalyst.expressions.Expression,
constructor:
(org.apache.spark.sql.catalyst.expressions.Expression,
org.apache.spark.sql.catalyst.expressions.Expression) => T,
original: T): T = {
- (left, right) match {
- case (boundRef:
org.apache.spark.sql.catalyst.expressions.BoundReference, literal:
org.apache.spark.sql.catalyst.expressions.Literal)
- if boundRef.dataType == org.apache.spark.sql.types.LongType &&
literal.dataType == org.apache.spark.sql.types.IntegerType =>
- val castExpr =
org.apache.spark.sql.catalyst.expressions.Cast(boundRef,
org.apache.spark.sql.types.IntegerType)
- constructor(castExpr, literal)
+ (left.dataType, right.dataType) match {
+ case (_: NumericType, _: NumericType) =>
+ TypeCoercion.findWiderTypeForTwo(left.dataType, right.dataType)
+ .map { widerType =>
+ val coercedLeft = if (left.dataType == widerType) left else
Cast(left, widerType)
Review Comment:
🤖 The `original` fallback does look safe: `BinaryComparison` takes its
ordering from `left.dataType`, and `Decimal.compare` falls back to
`toJavaBigDecimal.compareTo` whenever the two scales differ, so two
DecimalTypes compare by value with no cast needed. Also worth noting
`convertValueToInternal` already normalizes the column value to the declared
(precision, scale), so the cast isn't buying anything on that side beyond the
overflow path you describe.
--
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]
