hudi-agent commented on code in PR #19836:
URL: https://github.com/apache/hudi/pull/19836#discussion_r3927488012
##########
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:
🤖 Agree this is worth pinning so the test name doesn't overclaim. One thing
to watch if the transform does get extended: `In(value, list)` isn't binary —
coercion there needs a common wider type computed across `value` and every
element of `list`, then a cast on each, so the current binary
`applyTypeCoercion(left, right, ...)` helper won't map onto it directly.
`EqualNullSafe` is binary and would reuse it cleanly, but `In` likely wants its
own path. Might be cleanest to pin `ts IN (...)` with an issue reference now
and handle `In` as a follow-up.
--
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]
