voonhous commented on code in PR #19850:
URL: https://github.com/apache/hudi/pull/19850#discussion_r3943742597
##########
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:
**minor:** Not blocking, but this is a behavior change worth pinning.
`Or.eval` skips its right child when the left is true, so `id = 1 OR
concat(name, 'x') = 'a1x'` returns row 1 on master; after this change
`BaseProcedure.validateFilter` throws for the same string. Rejecting it looks
like the better contract, the test is just that validation is now
presence-based rather than reachability-based.
Could we add an assertion pinning that, so the change is deliberate rather
than incidental?
##########
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:
**major:** Matching on names alone under-approximates what actually
evaluates, so #19638 stays half-open. Both shapes below pass validation and
still silently drop every row.
Wrong arity of a listed name falls through `case _ => unresolvedFunc` at
:360 -- `substring(name, 2)` parses as `UnresolvedFunction(nArgs=2)` while :137
requires 3. And several shapes never become an `UnresolvedFunction` at all, so
`extractFunctionReferences` returns an empty set for them.
Could we lift the bind+resolve transform out of `evaluateExpressionOnRow`
and reject here when the resolved tree still contains an `Unevaluable` node?
That would cover arity, aggregates and subqueries in one check, and let
`SupportedFunctionNames` go away instead of being hand-maintained alongside the
41 match arms.
<details><summary>Verified instances (parsed with Spark 3.5.5
<code>CatalystSqlParser</code>)</summary>
Wrong arity: accepted by the name check, dropped at eval.
```
substring(name, 2) -> UnresolvedFunction nameParts=[substring] nArgs=2
(:137 requires 3)
substr(name, 2) -> UnresolvedFunction nameParts=[substr] nArgs=2
ceil(price, 1) -> UnresolvedFunction nameParts=[ceil] nArgs=2
(:161 requires 1)
floor(price, 1) -> UnresolvedFunction nameParts=[floor] nArgs=2
size(arr, 1) -> UnresolvedFunction nameParts=[size] nArgs=2
```
Invisible to the name check entirely: function set empty, tree still
unevaluable.
```
any_value(id) = 1 fnsSeen=[] unevaluable=true
first(id) = 1 fnsSeen=[] unevaluable=true
last(id) = 1 fnsSeen=[] unevaluable=true
id = (select 1) fnsSeen=[] unevaluable=true
exists (select 1) fnsSeen=[] unevaluable=true
```
</details>
##########
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") {
+ // Direct evaluation still treats an unresolved function as a non-match,
but procedure callers
+ // validate first so unsupported functions are reported instead of
silently dropping every row.
assertResult(Seq.empty)(keep(scalarRows, "concat(name, 'x') = 'a1x'",
scalarSchema))
assertResult(Seq.empty)(keep(scalarRows, "instr(name, 'a') = 1",
scalarSchema))
- assertResult(Right(()))(
- HoodieProcedureFilterUtils.validateFilterExpression("concat(name, 'x') =
'a1x'", scalarSchema, spark))
+ val unsupported = HoodieProcedureFilterUtils.validateFilterExpression(
+ "concat(name, 'x') = 'a1x' OR instr(name, 'a') = 1", scalarSchema, spark)
+ assert(unsupported.isLeft)
+ val unsupportedMsg = unsupported.fold(identity, _ => "")
+ assert(unsupportedMsg.contains("Unsupported functions: concat, instr"))
+ assert(unsupportedMsg.contains("Supported functions:"))
+ assert(unsupportedMsg.contains("upper"))
// if() is parsed as a function call and hits the same gap, while the
equivalent CASE WHEN is
// lowered by the parser without an UnresolvedFunction and evaluates fine.
assertResult(Seq.empty)(keep(scalarRows, "if(name = 'a1', true, false)",
scalarSchema))
assertResult(Seq(scalarRows.head))(
keep(scalarRows, "case when name = 'a1' then true else false end",
scalarSchema))
// Control: a function that is in the resolution table resolves and
matches.
+ assertResult(Right(()))(
+ HoodieProcedureFilterUtils.validateFilterExpression("upper(name) =
'A1'", scalarSchema, spark))
assertResult(Seq(scalarRows.head))(keep(scalarRows, "upper(name) = 'A1'",
scalarSchema))
Review Comment:
**nit:** Feel free to ignore. This line is byte-identical to :156 in
"evaluateFilter resolves string functions", and the new validate control just
above already carries the "upper is supported" claim.
Could we drop it?
##########
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") {
+ // Direct evaluation still treats an unresolved function as a non-match,
but procedure callers
+ // validate first so unsupported functions are reported instead of
silently dropping every row.
assertResult(Seq.empty)(keep(scalarRows, "concat(name, 'x') = 'a1x'",
scalarSchema))
assertResult(Seq.empty)(keep(scalarRows, "instr(name, 'a') = 1",
scalarSchema))
- assertResult(Right(()))(
- HoodieProcedureFilterUtils.validateFilterExpression("concat(name, 'x') =
'a1x'", scalarSchema, spark))
+ val unsupported = HoodieProcedureFilterUtils.validateFilterExpression(
+ "concat(name, 'x') = 'a1x' OR instr(name, 'a') = 1", scalarSchema, spark)
+ assert(unsupported.isLeft)
+ val unsupportedMsg = unsupported.fold(identity, _ => "")
+ assert(unsupportedMsg.contains("Unsupported functions: concat, instr"))
+ assert(unsupportedMsg.contains("Supported functions:"))
+ assert(unsupportedMsg.contains("upper"))
// if() is parsed as a function call and hits the same gap, while the
equivalent CASE WHEN is
Review Comment:
**minor:** Not blocking. `if(a, b, c)` is the case #19638 leads with, and
this change does now reject it, but only the old `evaluateFilter` `Seq.empty`
assertion is kept, so the fix itself is unpinned. The comment also now says the
opposite of the new behavior -- `if()` no longer "hits the same gap" at the
validation layer.
Could we add
`assert(HoodieProcedureFilterUtils.validateFilterExpression("if(name = 'a1',
true, false)", scalarSchema, spark).isLeft)` and reword the comment?
##########
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:
**minor:** Not blocking. `toLowerCase` here uses the JVM default locale.
Under `-Duser.language=tr`, `"ISNULL"` lowercases to a dotless-i form that
misses the set, so a genuinely supported function gets hard-rejected -- 10 of
the 41 names contain an `i`. #19835 fixed this same class in `a7deb61f7426`,
which is this PR's parent commit, and `style/scalastyle.xml:81` has
`RegexChecker` disabled so nothing flags it.
Could we use `Locale.ROOT` here (plus a `java.util.Locale` import) and align
:100 in the same patch?
```suggestion
Set(unresolved.nameParts.head.toLowerCase(Locale.ROOT)) ++
unresolved.children.flatMap(extractFunctionReferences)
```
##########
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:
**nit:** Feel free to ignore. A qualified call reports the wrong token:
`spark_catalog.default.upper(name)` parses with `nameParts = [spark_catalog,
default, upper]`, so `nameParts.head` at :497 makes this read `Unsupported
functions: spark_catalog`.
Could we report `nameParts.mkString(".")` so the message names the call the
user actually wrote?
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HoodieProcedureFilterUtils.scala:
##########
@@ -39,6 +39,13 @@ import scala.util.{Failure, Success, Try}
*/
object HoodieProcedureFilterUtils {
+ private val SupportedFunctionNames: Set[String] = Set(
+ "abs", "array_contains", "array_size", "between", "bigint", "ceil",
"ceiling", "coalesce",
+ "date_format", "datediff", "day", "dayofmonth", "double", "floor", "hour",
"integer", "int",
Review Comment:
**major:** `hour` and `date_format` are listed as supported, but this file's
own test pins that neither can ever evaluate.
`TestHoodieProcedureFilterUtils.scala:196-203` ("evaluateFilter cannot evaluate
time-zone-aware timestamp functions") asserts `Seq.empty` for `hour(t) = 12`
and `date_format(t, 'yyyy') = '2024'`, because the util binds and evals without
the analyzer so the time zone is never resolved. The message therefore tells a
user the function is supported and they then get the silent empty result of
#19638.
Could we drop both names from the set?
```suggestion
"datediff", "day", "dayofmonth", "double", "floor", "integer", "int",
```
##########
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:
**minor:** Not blocking, a placement point. After the rename, 5 of the 8
assertions here still call `keep` (i.e. `evaluateFilter`) -- concat, instr, if,
case-when, upper -- so the name no longer describes the body, and the two
`validateFilterExpression` assertions sit apart from the existing validate test
at :297.
Could we move the validate assertions into "validateFilterExpression accepts
valid references and rejects unknown ones" and leave this one under an
evaluateFilter-shaped name?
##########
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") {
+ // Direct evaluation still treats an unresolved function as a non-match,
but procedure callers
+ // validate first so unsupported functions are reported instead of
silently dropping every row.
assertResult(Seq.empty)(keep(scalarRows, "concat(name, 'x') = 'a1x'",
scalarSchema))
assertResult(Seq.empty)(keep(scalarRows, "instr(name, 'a') = 1",
scalarSchema))
- assertResult(Right(()))(
- HoodieProcedureFilterUtils.validateFilterExpression("concat(name, 'x') =
'a1x'", scalarSchema, spark))
+ val unsupported = HoodieProcedureFilterUtils.validateFilterExpression(
+ "concat(name, 'x') = 'a1x' OR instr(name, 'a') = 1", scalarSchema, spark)
Review Comment:
**major:** Nothing asserts the new message reaches a user through a `call`
statement, which is how #19638 reproduces. The sibling branch already has that
coverage: `TestShowCleansProcedures.scala:634-638` asserts `"Invalid column
references: nonexistent_col"` end to end, in a fixture that needs no data
insert.
Could we add one line beside it?
```scala
checkExceptionContain(s"""call show_clean_plans(table => '$tableName',
filter => "concat(action, 'x') = 'cleanx'")""")(
"Unsupported functions: concat")
```
##########
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") {
+ // Direct evaluation still treats an unresolved function as a non-match,
but procedure callers
+ // validate first so unsupported functions are reported instead of
silently dropping every row.
assertResult(Seq.empty)(keep(scalarRows, "concat(name, 'x') = 'a1x'",
scalarSchema))
assertResult(Seq.empty)(keep(scalarRows, "instr(name, 'a') = 1",
scalarSchema))
- assertResult(Right(()))(
- HoodieProcedureFilterUtils.validateFilterExpression("concat(name, 'x') =
'a1x'", scalarSchema, spark))
+ val unsupported = HoodieProcedureFilterUtils.validateFilterExpression(
Review Comment:
**nit:** Feel free to ignore.
`HoodieProcedureFilterUtils.validateFilterExpression(..., scalarSchema, spark)`
is now spelled out 7 times in this file, while the sibling API already has the
`keep` helper at :37.
Could we add `private def validate(expr: String, schema: StructType =
scalarSchema) = HoodieProcedureFilterUtils.validateFilterExpression(expr,
schema, spark)` next to it?
--
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]
