anshulsingh-py commented on code in PR #19853:
URL: https://github.com/apache/hudi/pull/19853#discussion_r3988718790
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/procedure/TestHoodieProcedureFilterUtils.scala:
##########
@@ -354,15 +371,100 @@ class TestHoodieProcedureFilterUtils extends
HoodieSparkProcedureTestBase {
assertResult(Seq(rows(1)))(keep(rows, "`50% overlap` > 15", schema))
}
- test("evaluateFilter silently drops rows for expressions it cannot resolve")
{
- assertResult(Seq.empty)(keep(scalarRows, "concat(name, 'x') = 'a1x'",
scalarSchema))
- assertResult(Seq.empty)(keep(scalarRows, "instr(name, 'a') = 1",
scalarSchema))
- assertResult(Seq.empty)(keep(scalarRows, "if(name = 'a1', true, false)",
scalarSchema))
+ test("evaluateFilter resolves functions outside the hardcoded table via
FunctionRegistry") {
+ // Functions missing from the hardcoded table now fall back to Spark's own
FunctionRegistry
+ // instead of being rejected as unsupported. See #19852.
+ assertKeeps(scalarRows, "concat(name, 'x') = 'a1x'", Seq(scalarRows.head))
+ assertKeeps(scalarRows, "instr(name, 'a') = 1", Seq(scalarRows.head))
+ assertKeeps(scalarRows, "if(name = 'a1', true, false)",
Seq(scalarRows.head))
assertResult(Seq(scalarRows.head))(
keep(scalarRows, "case when name = 'a1' then true else false end",
scalarSchema))
// Or short-circuits on the resolved side, which is what the
unresolved-operand guard preserves.
- assertResult(Seq(scalarRows.head))(
- keep(scalarRows, "id = 1 OR concat(name, 'x') = 'a1x'", scalarSchema))
+ assertKeeps(scalarRows, "id = 1 OR concat(name, 'x') = 'a1x'",
Seq(scalarRows.head))
+
+ // RuntimeReplaceable builtins (nvl, left, right, ...) resolve to a
placeholder node that
+ // FunctionRegistry.lookupFunction doesn't substitute on its own - make
sure we unwrap it
+ // rather than letting eval() blow up on the raw placeholder.
+ assertKeeps(scalarRows, "nvl(name, 'z') = 'a1'", Seq(scalarRows.head))
+ assertKeeps(scalarRows, "left(name, 1) = 'a'", Seq(scalarRows.head))
+ assertKeeps(scalarRows, "right(name, 1) = '1'", Seq(scalarRows.head))
+
+ // A hardcoded-table entry called with an arity the table doesn't handle
(substring only
+ // handles 3 args) should still fall back to the registry instead of
getting stuck.
+ assertKeeps(scalarRows, "substring(name, 2) = '1'", Seq(scalarRows.head))
+
+ // The rejection message for multiple unknown functions lists every name,
sorted.
+ assert(validate("no_such_fn(name) = 'x' OR other_missing(name) = 1")
+ .left.exists(_ == "Unsupported functions: no_such_fn, other_missing"))
+
+ // A 3+ part name (catalog.db.func) isn't safe to look up by bare function
name alone - make
+ // sure it's rejected rather than silently resolved against a same-named
function elsewhere.
+ assert(validate("some_catalog.some_db.upper(name) = 'A1'").isLeft)
+ assertResult(Seq.empty)(keep(scalarRows, "some_catalog.some_db.upper(name)
= 'A1'", scalarSchema))
+ // Same story for a 2-part db-qualified name: builtins register with no
database, so
+ // FunctionRegistry has no "default.upper" to find, and guessing by
dropping the qualifier
+ // would risk the same wrong-function-match problem as the 3+ part case.
+ assert(validate("default.upper(name) = 'A1'").isLeft)
+ assertResult(Seq.empty)(keep(scalarRows, "default.upper(name) = 'A1'",
scalarSchema))
+ }
+
+ test("evaluateFilter still rejects aggregate/generator/nondeterministic
functions resolved via FunctionRegistry") {
+ // Aggregate functions resolve fine as expressions but can't be eval()'d
per row - make sure
+ // those still go through the existing #19850 rejection path instead of
silently resolving to
+ // a broken, always-false filter. Same story for generators (explode only
makes sense in a
+ // projection) and non-deterministic functions (rand()/uuid() rely on
per-partition
+ // initialization this evaluator never does). any_value is covered
separately below - the
+ // parser lowers it straight to an AggregateExpression before it ever
reaches this guard.
+ // max(id) is an unambiguous AggregateFunction case (no decimal-literal
argument to complicate
+ // why it's rejected, unlike percentile's 0.5), so it's what actually pins
the guard clause.
+ assert(validate("max(id) > 0").left.exists(_.contains("Unsupported
functions: max")))
+ assertResult(Seq.empty)(keep(scalarRows, "max(id) > 0", scalarSchema))
+ assert(validate("percentile(id, 0.5) = 1").isLeft)
+ assert(validate("explode(array(1, 2)) =
1").left.exists(_.contains("Unsupported functions: explode")))
+ assert(validate("rand() = 1").isLeft)
+ assert(validate("uuid() = 'x'").isLeft)
+ assertResult(Seq.empty)(keep(scalarRows, "rand() = 1", scalarSchema))
+ // monotonically_increasing_id/input_file_name are also Nondeterministic,
so the same
+ // deterministic check catches them without needing their own case.
+ assert(validate("monotonically_increasing_id() = 1").isLeft)
+ assert(validate("input_file_name() = 'x'").isLeft)
+ // current_timestamp is deterministic-at-eval-time on most of the Spark
line this builds
+ // against (Spark computes it directly rather than requiring rule
substitution), so it
+ // resolves and evaluates for real instead of needing denylist treatment -
except on 4.0
+ // specifically, where CurrentTimestampLike briefly implemented
FoldableUnevaluable and threw
+ // on eval(); the eval-safety probe correctly rejects it there instead of
crashing the whole
+ // procedure call. current_date doesn't share the evaluating half at all:
it's a
+ // TimeZoneAwareExpression that stays unresolved without a session zone
the same way hour(t)
+ // does above, not because of anything this guard rejects.
+ if (HoodieSparkUtils.gteqSpark4_0 && !HoodieSparkUtils.gteqSpark4_1) {
+ assert(validate("current_timestamp() > t").isLeft)
+ assertResult(Seq.empty)(keep(scalarRows, "current_timestamp() > t",
scalarSchema))
+ } else {
+ assertResult(scalarRows)(keep(scalarRows, "current_timestamp() > t",
scalarSchema))
+ }
+ assert(validate("current_date() > d").isLeft)
+ }
+
+ test("evaluateFilter runs the same coercion rules the analyzer would for
concat/if/functions") {
+ // lookupFunction skips the analyzer's implicit-cast pass, but
applyImplicitCasts now runs
Review Comment:
Fixed - missed that one in the rename.
--
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]
