voonhous commented on code in PR #19836:
URL: https://github.com/apache/hudi/pull/19836#discussion_r3948177933
##########
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)
+ val coercedRight = if (right.dataType == widerType) right else
Cast(right, widerType)
+ constructor(coercedLeft, coercedRight)
Review Comment:
Both done. `widenNumericOperands` returns `None` when the operand types
already match, so an unchanged node is never rebuilt. Bind and resolve now run
once per batch, wrapped in their own `Try` so an unbindable expression still
yields an empty result rather than throwing.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/procedure/TestHoodieProcedureFilterUtils.scala:
##########
@@ -74,32 +74,23 @@ class TestHoodieProcedureFilterUtils extends
HoodieSparkProcedureTestBase {
assertResult(Seq(scalarRows.head))(keep(scalarRows, "flag = true",
scalarSchema))
}
- test("evaluateFilter coerces Long columns against integer literals") {
+ test("evaluateFilter widens Long columns and integer literals") {
// Exercises applyTypeCoercion for every comparison operator (Long
boundRef vs Int literal).
assertResult(Seq(scalarRows.head))(keep(scalarRows, "ts = 1000",
scalarSchema))
assertResult(Seq(scalarRows(1)))(keep(scalarRows, "ts > 1500",
scalarSchema))
assertResult(Seq(scalarRows(1)))(keep(scalarRows, "ts >= 2000",
scalarSchema))
assertResult(Seq(scalarRows.head))(keep(scalarRows, "ts < 2000",
scalarSchema))
assertResult(Seq(scalarRows.head))(keep(scalarRows, "ts <= 1000",
scalarSchema))
- // Known limitation: the coercion narrows the Long column to Int instead
of widening the Int
- // literal, so a Long value beyond Int range never matches (wrong results
under non-ANSI Spark,
- // swallowed overflow error under ANSI). Pinned here so a fix flips this
assertion; see #19632.
+ // The integer literal is widened, preserving Long values beyond the Int
range.
val bigRow = Seq(Row(3, "c3", 30.0d, 3000000000L, true, -9,
Date.valueOf("2024-03-16"), Timestamp.valueOf("2024-03-16 12:30:00")))
- assertResult(Seq.empty)(keep(bigRow, "ts > 2000", scalarSchema))
- // Known limitation: the coercion only matches column-on-left, so a
literal-on-left comparison
- // never coerces and drops every row instead of mirroring the equivalent
column-on-left filter.
- // Pinned here so a fix flips these assertions; see #19632.
- assertResult(Seq.empty)(keep(scalarRows, "1500 < ts", scalarSchema))
- assertResult(Seq.empty)(keep(scalarRows, "1000 = ts", scalarSchema))
+ assertResult(bigRow)(keep(bigRow, "ts > 2000", scalarSchema))
Review Comment:
Fixed: both halves run over one `scalarRows :+ bigRow` fixture. `ts < 2000`
has to exclude the big row, which is the assertion that catches the wrap to
`-1294967296`.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/procedure/TestHoodieProcedureFilterUtils.scala:
##########
@@ -74,32 +74,23 @@ class TestHoodieProcedureFilterUtils extends
HoodieSparkProcedureTestBase {
assertResult(Seq(scalarRows.head))(keep(scalarRows, "flag = true",
scalarSchema))
}
- test("evaluateFilter coerces Long columns against integer literals") {
+ test("evaluateFilter widens Long columns and integer literals") {
// Exercises applyTypeCoercion for every comparison operator (Long
boundRef vs Int literal).
assertResult(Seq(scalarRows.head))(keep(scalarRows, "ts = 1000",
scalarSchema))
assertResult(Seq(scalarRows(1)))(keep(scalarRows, "ts > 1500",
scalarSchema))
assertResult(Seq(scalarRows(1)))(keep(scalarRows, "ts >= 2000",
scalarSchema))
assertResult(Seq(scalarRows.head))(keep(scalarRows, "ts < 2000",
scalarSchema))
assertResult(Seq(scalarRows.head))(keep(scalarRows, "ts <= 1000",
scalarSchema))
- // Known limitation: the coercion narrows the Long column to Int instead
of widening the Int
- // literal, so a Long value beyond Int range never matches (wrong results
under non-ANSI Spark,
- // swallowed overflow error under ANSI). Pinned here so a fix flips this
assertion; see #19632.
+ // The integer literal is widened, preserving Long values beyond the Int
range.
val bigRow = Seq(Row(3, "c3", 30.0d, 3000000000L, true, -9,
Date.valueOf("2024-03-16"), Timestamp.valueOf("2024-03-16 12:30:00")))
- assertResult(Seq.empty)(keep(bigRow, "ts > 2000", scalarSchema))
- // Known limitation: the coercion only matches column-on-left, so a
literal-on-left comparison
- // never coerces and drops every row instead of mirroring the equivalent
column-on-left filter.
- // Pinned here so a fix flips these assertions; see #19632.
- assertResult(Seq.empty)(keep(scalarRows, "1500 < ts", scalarSchema))
- assertResult(Seq.empty)(keep(scalarRows, "1000 = ts", scalarSchema))
+ assertResult(bigRow)(keep(bigRow, "ts > 2000", scalarSchema))
+ // Coercion applies symmetrically when the literal is on the left.
+ assertResult(Seq(scalarRows(1)))(keep(scalarRows, "1500 < ts",
scalarSchema))
+ assertResult(Seq(scalarRows.head))(keep(scalarRows, "1000 = ts",
scalarSchema))
}
- test("evaluateFilter does not coerce other numeric column/literal type
pairs") {
- // Known limitation: applyTypeCoercion only special-cases a Long column
against an Int literal.
- // Every other numeric column/literal pair is left alone, so the
mismatched comparison fails to
- // evaluate; the per-row Try swallows the failure and drops the row. The
filter therefore
- // returns no rows instead of erroring on the type mismatch.
- // Pinned here so a fix flips the Seq.empty assertions; see #19632.
+ test("evaluateFilter coerces numeric column and literal type pairs") {
Review Comment:
Wired rather than pinned. `In` is n-ary as noted below, so the shared helper
takes the operand list and uses `findWiderCommonType` over `value +: list`;
`EqualNullSafe` reuses the binary path. Tests cover `ts IN (1000, 2000)` and
`ts <=> 1000`.
--
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]
