[SPARK-58001] [SQL] Improve UNSUPPORTED_EXPR_FOR_OPERATOR error message#57081
[SPARK-58001] [SQL] Improve UNSUPPORTED_EXPR_FOR_OPERATOR error message#57081mihailoale-db wants to merge 1 commit into
UNSUPPORTED_EXPR_FOR_OPERATOR error message#57081Conversation
|
@MaxGekk @cloud-fan Could you PTAL at this PR when you find time? Thanks! |
| "message" : [ | ||
| "A query operator contains one or more unsupported expressions.", | ||
| "Consider to rewrite it to avoid window functions, aggregate functions, and generator functions in the WHERE clause.", | ||
| "The query operator <operator> contains one or more unsupported expressions.", |
There was a problem hiding this comment.
I wonder whether operator is the best term. Here it seems it applies to a mix of different scenarios. Maybe just diluting it to "operation" works better. More generic.
There was a problem hiding this comment.
I partially agree. Although it would be better to have operations instead of operators but it's already like that all over the place in Spark:
SELECT * FROM VALUES (1, 10, 20) AS t(id, a, b) UNPIVOT (val FOR name IN (a, b)) ORDER BY t.athrow[MISSING_ATTRIBUTES.RESOLVED_ATTRIBUTE_MISSING_FROM_INPUT] Resolved attribute(s) "a" missing from "id", "name", "val" in operator SortSELECT * FROM VALUES(1) GROUP BY *throw[INVALID_USAGE_OF_STAR_OR_REGEX] Invalid usage of '*' in Aggregate- etc
Also it would require more work since we would have to map operators to operations which seem like a unnecessary overhead since we already have same stuff in other exceptions. I would leave it as it is for now if you agree.
There was a problem hiding this comment.
I presume the goal of this PR is to improve an error message. If an error message improvement needs a follow up PR then this sounds more like a check-box PR to me.
We should fix others that expose non-user facing gibberish as well. THAT coudl be a follow up. RIght now let's stop the bleed.
There was a problem hiding this comment.
It would be possible to create a map if this was an allowlist but it's not:
def specialExpressionsInUnsupportedOperator(plan: LogicalPlan): Seq[Expression] = {
val exprs = plan.expressions
val invalidExpressions = exprs.flatMap { root =>
root.collect {
case e: WindowExpression
if !plan.isInstanceOf[Window] => e
case e: AggregateExpression
if !(plan.isInstanceOf[Aggregate] ||
plan.isInstanceOf[Window] ||
plan.isInstanceOf[CollectMetrics] ||
onlyInLateralSubquery(plan)) => e
case e: Generator
if !(plan.isInstanceOf[Generate] ||
plan.isInstanceOf[BaseEvalPythonUDTF]) => e
}
}
invalidExpressions
}
It doesn't make sense to map every single operator to its operation. I would still keep it as it is.
There was a problem hiding this comment.
OK, then use a nested error condition to "roll them up". And you can always have "OTHER" catch all where you expose the the node.
There was a problem hiding this comment.
I don't see how it's different. Instead of operator -> operation you propose operator -> sub error class. It would be an overkill. Although I agree that it's the right way to do in majority of cases I would say that this one should use operators.
There was a problem hiding this comment.
@srielau Should we proceed with merging this? Thanks!
| throw new AnalysisException( | ||
| errorClass = "UNSUPPORTED_EXPR_FOR_OPERATOR", | ||
| messageParameters = Map( | ||
| "operator" -> operator.nodeName, |
There was a problem hiding this comment.
These are internal names. When you look at the test results some of them are hard to map back to teh external name.
| expectedErrorCondition = "UNSUPPORTED_EXPR_FOR_OPERATOR", | ||
| expectedMessageParameters = Map("invalidExprSqls" -> "\"sum(a)\", \"sum(c)\"") | ||
| expectedMessageParameters = Map( | ||
| "operator" -> "LateralJoin", |
There was a problem hiding this comment.
This should come out as LATERAL (the keyword representing the lateral join)
| "messageParameters" : { | ||
| "invalidExprSqls" : "\"row_number() OVER (ORDER BY salary ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)\"" | ||
| "invalidExprSqls" : "\"row_number() OVER (ORDER BY salary ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)\"", | ||
| "operator" : "Join" |
There was a problem hiding this comment.
| "operator" : "Join" | |
| "operator" : "ON" |
| "messageParameters" : { | ||
| "invalidExprSqls" : "\"RANK() OVER (ORDER BY 1 ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)\"" | ||
| "invalidExprSqls" : "\"RANK() OVER (ORDER BY 1 ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)\"", | ||
| "operator" : "Aggregate" |
There was a problem hiding this comment.
| "operator" : "Aggregate" | |
| "operator" : "GROUP BY" |
| "messageParameters" : { | ||
| "invalidExprSqls" : "\"max(col1)\"" | ||
| "invalidExprSqls" : "\"max(col1)\"", | ||
| "operator" : "Sort" |
There was a problem hiding this comment.
| "operator" : "Sort" | |
| "operator" : "ORDER BY" |
2 participants
What changes were proposed in this pull request?
To improve message for
UNSUPPORTED_EXPR_FOR_OPERATORexception.Why are the changes needed?
In order to improve error message.
Does this PR introduce any user-facing change?
Better error message.
How was this patch tested?
Testing change.
Was this patch authored or co-authored using generative AI tooling?
Yes.