hudi-agent commented on code in PR #19853:
URL: https://github.com/apache/hudi/pull/19853#discussion_r3957049419
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -389,6 +398,48 @@ object HoodieProcedureFilterUtils {
}
}
+ // 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 = {
Review Comment:
🤖 nit: could these be tightened into a short sentence-case comment (and
dropped as a scaladoc on the method)? The chatty first-person style and PR
references will age poorly compared to just stating why registry fallback is
used.
<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:
##########
@@ -95,7 +96,7 @@ object HoodieProcedureFilterUtils {
// Second pass: resolve functions
val functionResolved = attributeBound.transform {
case unresolvedFunc:
org.apache.spark.sql.catalyst.analysis.UnresolvedFunction =>
- unresolvedFunc.nameParts.head.toLowerCase(Locale.ROOT) match {
+ val tableResolved =
unresolvedFunc.nameParts.head.toLowerCase(Locale.ROOT) match {
Review Comment:
🤖 nit: `tableResolved` reads like it has something to do with a Hoodie table
— maybe `builtinTableResolved` or `hardcodedResolved` to make it clear it means
the hardcoded function table?
<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:
##########
@@ -389,6 +398,48 @@ object HoodieProcedureFilterUtils {
}
}
+ // 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
+ // Filter expressions only ever call plain or db-qualified builtins, so
this is really just
+ // name/db.name. A 3+ part name (catalog.db.func) isn't something we can
look up safely -
+ // FunctionIdentifier only carries one qualifier, and guessing by
dropping the extra parts
+ // risks matching a same-named function that isn't the one that was
actually asked for. Bail
+ // out to unresolvedFunc instead and let it fall through to the existing
rejection path.
+ val functionIdentifier = nameParts match {
+ case Seq(funcName) => Some(FunctionIdentifier(funcName))
+ case Seq(db, funcName) => Some(FunctionIdentifier(funcName, Some(db)))
+ case _ => None
+ }
+ val resolved = functionIdentifier
+ .map(sparkSession.sessionState.functionRegistry.lookupFunction(_,
unresolvedFunc.arguments))
+ .getOrElse(unresolvedFunc)
+ // lookupFunction on its own leaves nvl/ifnull/left/right etc as a
placeholder - normally
+ // the analyzer swaps it for the real expression right after, but nobody
does that here, so
+ // eval() just throws. Unwrap it ourselves instead.
+ val unwrapped = resolved.transformUp { case r: RuntimeReplaceable =>
r.replacement }
+ // a few more things resolve fine here but still can't be eval()'d one
row at a time:
+ // aggregates (percentile, collect_list) need real aggregation,
generators (explode,
+ // inline) only work inside a projection, and non-deterministic funcs
(rand, uuid,
+ // spark_partition_id) expect per-partition init we never do. Push all
of those back to
+ // unresolved so #19850's rejection path catches them instead of quietly
dropping every row.
+ val stillUnsupported =
+
unwrapped.isInstanceOf[org.apache.spark.sql.catalyst.expressions.aggregate.AggregateFunction]
||
Review Comment:
🤖 Should this also reject expressions that resolve structurally but fail
`checkInputDataTypes()`? `lookupFunction` skips the analyzer's implicit-cast
pass, so e.g. `concat(id, 'x')` on an int column builds a `Concat` that isn't
`Unevaluable` — validation passes, then `eval` throws ClassCastException and
the `Try` in `evaluateExpressionOnRow` swallows it, dropping every row silently
(exactly what #19850 set out to prevent). Adding `unwrapped.resolved &&
unwrapped.checkInputDataTypes().isSuccess` to the guard would push those back
to the rejection path.
<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]
