dongjoon-hyun commented on PR #58200:
URL: https://github.com/apache/spark/pull/58200#issuecomment-5388229604
Thanks for working on this. The direction looks right to me: `length` is
already forced to be a foldable constant, so validating it during analysis is
the natural thing to do, and there is precedent for it (`RegExpInStr`,
`TimeBucket`). My concern is with the mechanism.
### 1. `checkInputDataTypes()` should return a `TypeCheckResult`, not throw
```scala
if (result == TypeCheckResult.TypeCheckSuccess) {
lengthInteger()
}
result
```
`lengthInteger()` raises
`QueryExecutionErrors.unexpectedValueForLengthInFunctionError`, i.e. a
`SparkRuntimeException` with `INVALID_PARAMETER_VALUE.LENGTH` (sqlState
`22023`, a data exception). Raising that from analysis has a few consequences:
- **It is not an `AnalysisException`.** A compile-time failure surfacing as
a runtime exception breaks the `spark.sql(...)` / DataFrame contract and the
compile-vs-runtime distinction that Connect error mapping and downstream
tooling rely on.
- **No `QueryContext` is attached.** On the normal path,
`TypeCoercionValidation.failOnTypeCheckResult(e, Some(operator))` in
`CheckAnalysis` attaches the origin. Every other `RandStr` input check produces
an error carrying the SQL fragment (see the `fragment = "randstr"` assertions
in `DataFrameFunctionsSuite`), so this one case would be the odd one out.
- **It escapes through `Expression.resolved`.** `Expression.resolved` is
`childrenResolved && checkInputDataTypes().isSuccess`, and
`LogicalPlan.resolved` is `expressions.forall(_.resolved) && childrenResolved`
— expressions are evaluated *before* children. So the throw can fire at
arbitrary points inside the analyzer fixed point, and can preempt other, more
appropriate analysis errors.
I grepped every `checkInputDataTypes()` body in `sql/catalyst` that contains
a `throw`: they all raise either `QueryCompilationErrors.*` (an
`AnalysisException`) or `SparkException.internalError`. None raises a
`SparkRuntimeException`. So I do not think the claim in the PR description that
this "is consistent with how other constant arguments are validated in
`checkInputDataTypes()`" holds — the others *return* a failed `TypeCheckResult`.
The closest precedents are `TimeBucket` (`datetimeExpressions.scala`) and
`RegExpInStr` (`regexpExpressions.scala`): both `eval()` the foldable constant
and return `DataTypeMismatch(VALUE_OUT_OF_RANGE)`. Something like:
```scala
if (result == TypeCheckResult.TypeCheckSuccess) {
val lengthValue = length.eval()
// A null length is treated as 0 (see `randstr(NULL, 0)`), so only
reject negative values.
if (lengthValue != null && lengthValue.asInstanceOf[Int] < 0) {
result = DataTypeMismatch(
errorSubClass = "VALUE_OUT_OF_RANGE",
messageParameters = Map(
"exprName" -> toSQLId("length"),
"valueRange" -> s"[0, ${Int.MaxValue}]",
"currentValue" -> toSQLValue(lengthValue, IntegerType)))
}
}
result
```
The null guard matters: `SELECT randstr(NULL, 0)` is a *passing* case today
(`FunctionArgumentTypeCoercion` casts it to `Cast(null, IntegerType)` and the
query returns an empty string, see `results/random.sql.out`). The current patch
happens to survive it because `null.asInstanceOf[Int]` is `0`, but it is easy
to lose when rewriting.
Note this changes the error condition from `INVALID_PARAMETER_VALUE.LENGTH`
to `DATATYPE_MISMATCH.VALUE_OUT_OF_RANGE`, so the "Does this PR introduce any
user-facing change?" section would need updating, and `results/random.sql.out`
would need regenerating as well — the PR currently only updates
`analyzer-results/random.sql.out`.
### 2. The side-effect-only call is hard to read
Calling `lengthInteger()` and discarding its result purely for the exception
makes the reader stop and wonder why the value is unused. Even if the throwing
approach were kept, an explicit value comparison would read better.
### 3. Tests
The only test change is a regenerated golden file. Since the whole point of
the PR is *when* the error is raised, it would be good to assert that directly.
`DataFrameFunctionsSuite`'s `test("randstr function")` already collects the
`randstr` error cases with `checkError`, so a negative-length case there
(failing on `df.select(...)` alone, without an action) would fit naturally.
### 4. The user-facing impact is a bit broader than described
"only the phase at which it is raised changes" understates it — queries that
never reached execution now fail too:
- `EXPLAIN SELECT randstr(-1, 0)` used to print a plan successfully.
- `spark.sql("SELECT randstr(-1, 0)")` now throws immediately, with no
action, so `df.schema` is unreachable.
- A `randstr(-1, ...)` sitting in a branch the optimizer used to prune away.
Failing fast is the intent of the PR and I think that behavior is fine, but
it is worth spelling out in the description.
---
Summary:
| # | Severity | Item |
|---|---|---|
| 1 | Blocker | Throwing `SparkRuntimeException` during analysis; return
`DataTypeMismatch` instead (plus null guard, plus regenerate
`results/random.sql.out`) |
| 2 | Minor | Side-effect-only call with a discarded result |
| 3 | Minor | No test asserting the analysis-time failure |
| 4 | Minor | PR description overstates consistency with existing checks and
understates user impact |
I reviewed this statically against the codebase conventions and did not
build or run the tests.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
