You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dejankrak-db (via GitHub)" <gi...@apache.org> on 2026/05/31 22:05:48 UTC
[PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
dejankrak-db opened a new pull request, #56238:
URL: https://github.com/apache/spark/pull/56238
### What changes were proposed in this pull request?
Two related fixes for non-foldable DEFAULT expressions (e.g. `current_user()`) on CHAR/VARCHAR columns:
1. **Fix INTERNAL_ERROR at DDL time.** `ResolveDefaultColumns.coerceDefaultValue` called `CharVarcharUtils.stringLengthCheck(ret, dataType).eval(EmptyRow)` to validate CHAR/VARCHAR default lengths at compile time, without checking foldability. For a non-foldable default such as `current_user()` (processed before it is replaced by a literal), `.eval(EmptyRow)` throws "Cannot evaluate expression", surfacing as INTERNAL_ERROR. Added an `&& ret.foldable` guard so the eager check only runs for foldable expressions.
2. **Add runtime CHAR/VARCHAR length enforcement for implicit defaults.** When a column with a DEFAULT is omitted from an INSERT column list, `TableOutputResolver.reorderColumnsByName` filled the default but did not apply `stringLengthCheck`, so oversized non-foldable defaults silently succeeded. It now wraps the default expression with the write-side length check, using `CharVarcharUtils.getRawType(metadata)` to detect CHAR/VARCHAR for both V1 and V2 tables. The explicit `DEFAULT` keyword path already goes through `checkField`.
### Why are the changes needed?
`CREATE TABLE t(s CHAR(100) DEFAULT current_user()) USING parquet` (or the equivalent `ALTER TABLE ... SET DEFAULT`) previously failed with INTERNAL_ERROR. Oversized non-foldable defaults for omitted columns also bypassed length checks.
### Does this PR introduce _any_ user-facing change?
Yes. Non-foldable expressions are now allowed as CHAR/VARCHAR defaults (instead of throwing INTERNAL_ERROR), and oversized non-foldable defaults on omitted INSERT columns now fail with EXCEED_LIMIT_LENGTH at runtime instead of succeeding.
### How was this patch tested?
Added 6 tests to `ResolveDefaultColumnsSuite` (CREATE/ALTER with `current_user()` on CHAR/VARCHAR; foldable oversize fails at DDL; non-foldable oversize fails at INSERT for both implicit and explicit DEFAULT paths). Full suite: 19 tests pass.
### Was this patch authored or co-authored using generative AI tooling?
Yes, co-authored using Claude code.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56238:
URL: https://github.com/apache/spark/pull/56238#issuecomment-4598597237
thanks, merging to master/4.x/4.2 (bug fix)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "dejankrak-db (via GitHub)" <gi...@apache.org>.
dejankrak-db commented on PR #56238:
URL: https://github.com/apache/spark/pull/56238#issuecomment-4598754019
> @dejankrak-db you can address the last minor review comments in your other related PRs.
Added the suggested minor fix as part of https://github.com/apache/spark/pull/56237
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56238:
URL: https://github.com/apache/spark/pull/56238#issuecomment-4598610389
@dejankrak-db you can address the last minor review comments in your other related PRs.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "dejankrak-db (via GitHub)" <gi...@apache.org>.
dejankrak-db commented on code in PR #56238:
URL: https://github.com/apache/spark/pull/56238#discussion_r3337905789
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TableOutputResolver.scala:
##########
@@ -327,7 +327,25 @@ object TableOutputResolver extends SQLConfHelper with Logging {
tableName, newColPath.quoted
)
}
- Some(applyColumnMetadata(defaultExpr.get, expectedCol))
+ // Apply CHAR/VARCHAR length check to the default expression so that non-foldable
+ // defaults (e.g. current_user()) that exceed the column length are caught at runtime.
+ // Uses getRawType so it works for both V1 and V2 tables.
+ val checkedExpr = defaultExpr.map { expr =>
+ val rawType = CharVarcharUtils.getRawType(expectedCol.metadata)
+ .getOrElse(expectedCol.dataType)
+ if (!conf.charVarcharAsString && CharVarcharUtils.hasCharVarchar(rawType)) {
+ expr match {
+ case a: Alias =>
+ a.copy(child = CharVarcharUtils.stringLengthCheck(a.child, rawType))(
Review Comment:
Dead code collapsed - the helper drops the pointless Alias-field preservation (`applyColumnMetadata` strips + re-wraps anyway) and the dead case other branch; it just wraps the default's value with stringLengthCheck
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56238:
URL: https://github.com/apache/spark/pull/56238#discussion_r3338593172
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TableOutputResolver.scala:
##########
@@ -284,6 +284,31 @@ object TableOutputResolver extends SQLConfHelper with Logging {
}
}
+ /**
+ * Builds the [[NamedExpression]] for a missing column filled with its default value, applying a
+ * write-side CHAR/VARCHAR length check so that non-foldable defaults (e.g. `current_user()`)
+ * that exceed the column length are caught at runtime. Uses `getRawType` so it works for both
+ * V1 and V2 tables. Shared by the by-name and by-position default-fill paths.
+ *
+ * `applyColumnMetadata` strips the default's outer alias and re-wraps it with the required
+ * metadata, so the length check is applied to the default value itself (the alias child).
Review Comment:
Minor doc-accuracy nit: this sentence attributes the alias unwrap to `applyColumnMetadata`, but in the CHAR/VARCHAR path the helper itself unwraps the alias (`case a: Alias => a.child` below) *before* the length check, and `applyColumnMetadata` then runs on the `StaticInvoke` (no longer an `Alias`), so it never strips the default's alias here. The unwrap is needed precisely because otherwise `stringLengthCheck` would wrap the `Alias` and `applyColumnMetadata` would *not* strip it. Behavior is correct; only the explanation is backwards. Suggested rewrite:
```suggestion
* We unwrap the default's outer alias before the length check so the check wraps the
* default value itself, not the alias; `applyColumnMetadata` then re-adds the required
* alias and metadata afterward.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #56238: [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns
URL: https://github.com/apache/spark/pull/56238
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "dejankrak-db (via GitHub)" <gi...@apache.org>.
dejankrak-db commented on code in PR #56238:
URL: https://github.com/apache/spark/pull/56238#discussion_r3337896851
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TableOutputResolver.scala:
##########
@@ -327,7 +327,25 @@ object TableOutputResolver extends SQLConfHelper with Logging {
tableName, newColPath.quoted
)
}
- Some(applyColumnMetadata(defaultExpr.get, expectedCol))
+ // Apply CHAR/VARCHAR length check to the default expression so that non-foldable
Review Comment:
Extracted a shared `applyDefaultWithLengthCheck(defaultExpr, expectedCol, conf)` helper, called from both default-fill sites — `reorderColumnsByName` (by-name) and `resolveColumnsByPosition` (by-position, reached under V2 schema-evolution RECURSE). Both paths now apply the CHAR/VARCHAR write-side length check identically. Thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57187][SQL] Fix INTERNAL_ERROR when current_user() is used as DEFAULT for CHAR/VARCHAR columns [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56238:
URL: https://github.com/apache/spark/pull/56238#discussion_r3334592405
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TableOutputResolver.scala:
##########
@@ -327,7 +327,25 @@ object TableOutputResolver extends SQLConfHelper with Logging {
tableName, newColPath.quoted
)
}
- Some(applyColumnMetadata(defaultExpr.get, expectedCol))
+ // Apply CHAR/VARCHAR length check to the default expression so that non-foldable
Review Comment:
This runtime enforcement is only added to the by-name default-fill path. The by-position fill in `resolveColumnsByPosition` (around lines 461-470) is structurally identical — `getDefaultValueExprOrNullLit` + `applyColumnMetadata` with no `stringLengthCheck` — and is reached for a by-position INSERT under V2 schema evolution (RECURSE; see `Analyzer.scala`'s `coerceInsertNestedTypes && schemaEvolutionEnabled` branch) when a trailing CHAR/VARCHAR column is omitted. So an oversized non-foldable default there would still be written without EXCEED_LIMIT_LENGTH — the same bypass this PR closes for by-name. I traced both sites and the matched-column analogues (`checkField`/`checkUpdate` both wrap with `stringLengthCheck`) but didn't run an end-to-end schema-evolution write — can you confirm the by-position path isn't enforced elsewhere? If not, I'd extract a small shared helper (e.g. `applyDefaultWithLengthCheck(expectedCol, conf)`) called from both default-fill sites, and add a t
est covering the by-position/RECURSE path.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TableOutputResolver.scala:
##########
@@ -327,7 +327,25 @@ object TableOutputResolver extends SQLConfHelper with Logging {
tableName, newColPath.quoted
)
}
- Some(applyColumnMetadata(defaultExpr.get, expectedCol))
+ // Apply CHAR/VARCHAR length check to the default expression so that non-foldable
+ // defaults (e.g. current_user()) that exceed the column length are caught at runtime.
+ // Uses getRawType so it works for both V1 and V2 tables.
+ val checkedExpr = defaultExpr.map { expr =>
+ val rawType = CharVarcharUtils.getRawType(expectedCol.metadata)
+ .getOrElse(expectedCol.dataType)
+ if (!conf.charVarcharAsString && CharVarcharUtils.hasCharVarchar(rawType)) {
+ expr match {
+ case a: Alias =>
+ a.copy(child = CharVarcharUtils.stringLengthCheck(a.child, rawType))(
Review Comment:
This branch carefully preserves `exprId`/`qualifier`/`explicitMetadata`/`nonInheritableMetadataKeys`, but `applyColumnMetadata` on the next line strips the Alias down to its child and re-wraps it in a fresh `Alias(stripAlias, name)(explicitMetadata = Some(requiredMetadata))`, so all of that is discarded. The `case other` branch is also dead — `getDefaultValueExprOrNullLit` always returns an `Alias`. The whole block can collapse to wrapping the default's child with `stringLengthCheck`, which folds naturally into the shared helper suggested above.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org