[SPARK-58931][SQL] Reject negative randstr length during analysis

[uros-b]

What changes were proposed in this pull request?

randstr(length[, seed]) requires a non-negative length. Today that guard lives only on the execution paths - RandStr.lengthInteger(), called from evalInternal (interpreted) and doGenCode (codegen), so a negative constant length is not rejected until the query executes.

This PR moves the guard into Catalyst analysis. RandStr.checkInputDataTypes() already requires length to be a foldable integer; once those checks pass, it now evaluates the (constant) length and, if it is negative, returns a failed TypeCheckResult - DataTypeMismatch("VALUE_OUT_OF_RANGE") - rather than throwing. A null length is left untouched (randstr(NULL, 0) remains valid and returns an empty string). Because checkInputDataTypes() runs during analysis, randstr with a negative constant length is now rejected at analysis time as an AnalysisException carrying the query context, like the other randstr input checks.

This follows the precedent of other expressions that validate a foldable constant during analysis - TimeBucket (datetimeExpressions.scala) and RegExpReplace (regexpExpressions.scala) - which eval() the constant and return DataTypeMismatch("VALUE_OUT_OF_RANGE").

Why are the changes needed?

randstr's length must be a foldable constant, which checkInputDataTypes() already enforces, so a negative length can be detected during analysis rather than only at execution. Performing the check in checkInputDataTypes():

• rejects an invalid randstr(-1, ...) during analysis (fail-fast), rather than only once the expression is evaluated at execution;
• surfaces the failure as an AnalysisException with a QueryContext, like every other randstr input check, instead of a SparkRuntimeException escaping from analysis;
• keeps the validation with the rest of randstr's input checking.

Does this PR introduce any user-facing change?

Yes. randstr with a negative constant length now fails during analysis instead of at execution, and the error class changes.

• Before: analysis succeeds; the query fails at execution with a SparkRuntimeException, error class INVALID_PARAMETER_VALUE.LENGTH.
• After: the query fails during analysis with an AnalysisException, error class DATATYPE_MISMATCH.VALUE_OUT_OF_RANGE.

Because the failure now happens during analysis, it affects queries that never reach execution:

• EXPLAIN SELECT randstr(-1, 0) previously printed a plan; it now fails.
• spark.sql("SELECT randstr(-1, 0)") now throws immediately (before any action), so the returned DataFrame's schema is unreachable.
• A randstr(-1, ...) in a branch the optimizer would have pruned away now fails rather than being eliminated.

Failing fast on an invalid constant length is the intent of this change. Queries with a valid (non-negative or NULL) length are unaffected.

How was this patch tested?

• Added a negative-length case to DataFrameFunctionsSuite's test("randstr function") asserting that df.select(randstr(lit(-1), lit(0))) fails during analysis (on select alone, without an action) with DATATYPE_MISMATCH.VALUE_OUT_OF_RANGE.
• The golden SQL test random.sql covers SELECT randstr(-1, 0); regenerated both analyzer-results/random.sql.out and results/random.sql.out and reviewed the diff:

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z random.sql"

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

[uros-b]

@cloud-fan Please review.

[uros-b]

cc @vladimirg-db

[dongjoon-hyun]

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

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:

    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.

[cloud-fan]

0 blocking, 1 non-blocking, 0 nits.
The implementation is sound and well-covered; one non-blocking public-documentation update remains.

Suggestions (1)

• Non-blocking: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala:431: Document length as non-negative in randstr's public SQL description so the new analysis-time range is discoverable. -- see inline

Verification

Static verification: traced the iterative CheckAnalysis and single-pass ExpressionResolver paths to the same RandStr.checkInputDataTypes implementation; compared the foldable-range pattern with RegExpReplace; and confirmed the negative and NULL cases in the DataFrame test and SQL goldens. The Spark test suite was not run.

PR metadata suggestions

• Replace the cited RegExpInStr precedent with RegExpReplace; the latter is the expression whose checkInputDataTypes evaluates the foldable integer and returns VALUE_OUT_OF_RANGE.

[uros-b]

Thank you @vladimirg-db @dongjoon-hyun @cloud-fan for review!

[uros-b]

Merge Summary:

• merged into master d01d020

Posted by merge_spark_pr.py

[uros-b]

Merge Summary:

• merged into branch-4.x fb3509c
• merged into branch-4.3 cfa026c

Posted by merge_spark_pr.py