hudi-agent commented on code in PR #19853:
URL: https://github.com/apache/hudi/pull/19853#discussion_r3945055886
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -367,12 +368,29 @@ object HoodieProcedureFilterUtils {
}
}
- private def evaluateExpressionOnRow(expression: Expression, row: Row,
schema: StructType): Boolean = {
+ // didn't match anything above, so ask Spark itself before we give up -
saves us from having
+ // to hand-list every builtin (concat, instr, if, ...) one by one
+ private def resolveViaFunctionRegistry(unresolvedFunc: UnresolvedFunction,
sparkSession: SparkSession): Expression = {
+ Try {
+ val nameParts = unresolvedFunc.nameParts
+ val functionIdentifier = nameParts match {
+ case Seq(funcName) => FunctionIdentifier(funcName)
+ case Seq(db, funcName) => FunctionIdentifier(funcName, Some(db))
+ case _ => FunctionIdentifier(nameParts.last)
+ }
+
sparkSession.sessionState.functionRegistry.lookupFunction(functionIdentifier,
unresolvedFunc.arguments)
Review Comment:
🤖 `lookupFunction` returns the raw expression without the analyzer rules
that normally follow it, so `RuntimeReplaceable` builtins (`nvl`, `ifnull`,
`left`, `right`, `nvl2`, ...) come back un-substituted. They aren't
`Unevaluable`, so `validateFilterExpression` accepts them, but
`RuntimeReplaceable.eval` is `assert(input == null); replacement.eval()` —
evaluating against a real row throws, gets swallowed by the `Try` in
`evaluateExpressionOnRow`, and every row is silently dropped. Could you unwrap
`replacement` (e.g. `transformUp { case r: RuntimeReplaceable => r.replacement
}`) after the lookup? Same concern for
`ImperativeAggregate`/`TypedImperativeAggregate` results like
`percentile`/`collect_list`, which are also resolved-but-not-`Unevaluable` and
blow up on `eval`. @yihua
<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:
##########
@@ -348,7 +349,7 @@ object HoodieProcedureFilterUtils {
} else {
unresolvedFunc
}
- case _ => unresolvedFunc
+ case _ => resolveViaFunctionRegistry(unresolvedFunc, sparkSession)
Review Comment:
🤖 The fallback only fires from the `case _` default, so a function name that
*is* in the hardcoded table but with an arity the table doesn't handle still
returns `unresolvedFunc` and gets rejected — e.g. the 2-arg `substring(name,
2)` (line 129 only handles 3 args), which the test at line 313 still pins as
`isLeft`. Would it be cleaner to apply `resolveViaFunctionRegistry` to whatever
the match produces if it's still an `UnresolvedFunction`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
