ryux1 commented on code in PR #19850:
URL: https://github.com/apache/hudi/pull/19850#discussion_r3943932542
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -468,9 +475,13 @@ object HoodieProcedureFilterUtils {
val columnNames = schema.fieldNames.toSet
val referencedColumns = extractColumnReferences(parsedExpr)
val invalidColumns = referencedColumns -- columnNames
+ val unsupportedFunctions = extractFunctionReferences(parsedExpr) --
SupportedFunctionNames
Review Comment:
Addressed in 1dd694b. I extracted the evaluator’s bind/function-resolution
pass and reuse it during validation. Residual `UnresolvedFunction` nodes catch
unknown names and wrong arity, while non-resolved or `Unevaluable` nodes catch
aggregates and subqueries. The hand-maintained allowlist is gone.
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -468,9 +475,13 @@ object HoodieProcedureFilterUtils {
val columnNames = schema.fieldNames.toSet
val referencedColumns = extractColumnReferences(parsedExpr)
val invalidColumns = referencedColumns -- columnNames
+ val unsupportedFunctions = extractFunctionReferences(parsedExpr) --
SupportedFunctionNames
if (invalidColumns.nonEmpty) {
Left(s"Invalid column references: ${invalidColumns.mkString(", ")}.
Available columns: ${columnNames.mkString(", ")}")
+ } else if (unsupportedFunctions.nonEmpty) {
Review Comment:
Added the short-circuit case in 1dd694b. `id = 1 OR concat(name, 'x') =
'a1x'` is now explicitly required to fail validation even though row evaluation
could skip the unsupported right branch.
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -468,9 +475,13 @@ object HoodieProcedureFilterUtils {
val columnNames = schema.fieldNames.toSet
val referencedColumns = extractColumnReferences(parsedExpr)
val invalidColumns = referencedColumns -- columnNames
+ val unsupportedFunctions = extractFunctionReferences(parsedExpr) --
SupportedFunctionNames
if (invalidColumns.nonEmpty) {
Left(s"Invalid column references: ${invalidColumns.mkString(", ")}.
Available columns: ${columnNames.mkString(", ")}")
+ } else if (unsupportedFunctions.nonEmpty) {
+ Left(s"Unsupported functions:
${unsupportedFunctions.toSeq.sorted.mkString(", ")}. "
Review Comment:
Addressed in 1dd694b: residual unresolved functions are now reported with
`nameParts.mkString(".")`, so a qualified call names the full token.
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -481,6 +492,12 @@ object HoodieProcedureFilterUtils {
}
}
+ private def extractFunctionReferences(expression: Expression): Set[String] =
expression match {
+ case unresolved: UnresolvedFunction =>
+ Set(unresolved.nameParts.head.toLowerCase) ++
unresolved.children.flatMap(extractFunctionReferences)
Review Comment:
Addressed in 1dd694b. Function dispatch now lowercases with `Locale.ROOT`;
the obsolete name-extraction lowercase path was removed with the allowlist.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/procedure/TestHoodieProcedureFilterUtils.scala:
##########
@@ -120,21 +120,26 @@ class TestHoodieProcedureFilterUtils extends
HoodieSparkProcedureTestBase {
assertResult(Seq.empty)(keep(scalarRows, "price > 15.0", scalarSchema))
}
- test("evaluateFilter silently drops rows for functions outside the
resolution table") {
- // Known limitation: a function missing from the resolution table falls
through as an
- // UnresolvedFunction. validateFilterExpression only checks column
references, so nothing
- // rejects it; instead evaluation fails per row and the row is dropped,
which looks like an
- // empty result rather than an error. Pinned here so a fix flips these;
see #19638.
+ test("validateFilterExpression rejects functions outside the resolution
table") {
Review Comment:
Reorganized in 1dd694b. The direct `evaluateFilter` regressions remain under
an evaluation-focused test, and all validation assertions now live with the
existing validation coverage.
--
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]
