Skip to content

[SPARK-52236][SQL] Standardize analyze exceptions for default value #50960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
+68 −40
Closed

[SPARK-52236][SQL] Standardize analyze exceptions for default value #50960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ff0712
[SPARK-52236][SQL] Standardize analyze exceptions for default value
szehon-ho May 20, 2025
99ff574
Fix tests
szehon-ho May 21, 2025
ed5ed73
Address comments
szehon-ho May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 5 additions & 26 deletions ...atalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ object ColumnDefinition {
// Do not check anything if the children are not resolved yet.
case _ if !plan.childrenResolved =>

// Wrap errors for default values in a more user-friendly message.
case cmd: V2CreateTablePlan if cmd.columns.exists(_.defaultValue.isDefined) =>
val statement = cmd match {
case _: CreateTable => "CREATE TABLE"
Expand All @@ -161,44 +162,22 @@ object ColumnDefinition {
cmd.columns.foreach { col =>
col.defaultValue.foreach { default =>
checkDefaultColumnConflicts(col)
validateDefaultValueExpr(default, statement, col.name, col.dataType)
if (!default.deterministic) {
throw QueryCompilationErrors.defaultValueNonDeterministicError(
"CREATE TABLE", col.name, default.originalSQL)
}
validateDefaultValueExpr(default, statement, col.name, Some(col.dataType))
}
}

case cmd: AddColumns if cmd.columnsToAdd.exists(_.default.isDefined) =>
// Wrap analysis errors for default values in a more user-friendly message.
cmd.columnsToAdd.foreach { c =>
c.default.foreach { d =>
if (!d.resolved) {
throw QueryCompilationErrors.defaultValuesUnresolvedExprError(
"ALTER TABLE", c.colName, d.originalSQL, null)
}
validateDefaultValueExpr(d, "ALTER TABLE", c.colName, c.dataType)
if (!d.deterministic) {
throw QueryCompilationErrors.defaultValueNonDeterministicError(
"ALTER TABLE", c.colName, d.originalSQL)
}
validateDefaultValueExpr(d, "ALTER TABLE ADD COLUMNS", c.colName, Some(c.dataType))
}
}

case cmd: AlterColumns if cmd.specs.exists(_.newDefaultExpression.isDefined) =>
// Wrap analysis errors for default values in a more user-friendly message.
cmd.specs.foreach { c =>
c.newDefaultExpression.foreach { d =>
if (!d.resolved) {
throw QueryCompilationErrors.defaultValuesUnresolvedExprError(
"ALTER TABLE ALTER COLUMN", c.column.name.quoted, d.originalSQL, null)
}
validateDefaultValueExpr(d, "ALTER TABLE ALTER COLUMN",
c.column.name.quoted, d.dataType)
if (!d.deterministic) {
throw QueryCompilationErrors.defaultValueNonDeterministicError(
"ALTER TABLE ALTER COLUMN", c.column.name.quoted, d.originalSQL)
}
validateDefaultValueExpr(d, "ALTER TABLE ALTER COLUMN", c.column.name.quoted,
None)
}
}

Expand Down
29 changes: 21 additions & 8 deletions ...atalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ResolveDefaultColumnsUtil.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,21 +583,29 @@ object ResolveDefaultColumns extends QueryErrorsBase
default: DefaultValueExpression,
statement: String,
colName: String,
targetType: DataType): Unit = {
targetTypeOption: Option[DataType]): Unit = {
if (default.containsPattern(PLAN_EXPRESSION)) {
throw QueryCompilationErrors.defaultValuesMayNotContainSubQueryExpressions(
statement, colName, default.originalSQL)
} else if (default.resolved) {
val dataType = CharVarcharUtils.replaceCharVarcharWithString(targetType)
if (!Cast.canUpCast(default.child.dataType, dataType) &&
defaultValueFromWiderTypeLiteral(default.child, dataType, colName).isEmpty) {
throw QueryCompilationErrors.defaultValuesDataTypeError(
statement, colName, default.originalSQL, targetType, default.child.dataType)
targetTypeOption match {
case Some(targetType) =>
CharVarcharUtils.replaceCharVarcharWithString(targetType)
if (!Cast.canUpCast(default.child.dataType, targetType) &&
defaultValueFromWiderTypeLiteral(default.child, targetType, colName).isEmpty) {
throw QueryCompilationErrors.defaultValuesDataTypeError(
statement, colName, default.originalSQL, targetType, default.child.dataType)
}
case _ => ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case _ => ()
case _ =>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too minor, you can address it in your next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks!

}
// Our analysis check passes here. We do not further inspect whether the
// expression is `foldable` here, as the plan is not optimized yet.
} else {
throw QueryCompilationErrors.defaultValuesUnresolvedExprError(
statement, colName, default.originalSQL, null)
}

// Our analysis check passes here. We do not further inspect whether the
// expression is `foldable` here, as the plan is not optimized yet.

if (default.references.nonEmpty || default.exists(_.isInstanceOf[VariableReference])) {
// Ideally we should let the rest of `CheckAnalysis` report errors about why the default
// expression is unresolved. But we should report a better error here if the default
Expand All @@ -606,6 +614,11 @@ object ResolveDefaultColumns extends QueryErrorsBase
throw QueryCompilationErrors.defaultValueNotConstantError(
statement, colName, default.originalSQL)
}

if (!default.deterministic) {
throw QueryCompilationErrors.defaultValueNonDeterministicError(
statement, colName, default.originalSQL)
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/connector/AlterTableTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ trait AlterTableTests extends SharedSparkSession with QueryErrorsBase {
},
condition = "INVALID_DEFAULT_VALUE.UNRESOLVED_EXPRESSION",
parameters = Map(
"statement" -> "ALTER TABLE",
"statement" -> "ALTER TABLE ADD COLUMNS",
"colName" -> "`s`",
"defaultValue" -> "badvalue"))

Expand Down
4 changes: 2 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ class DataSourceV2DataFrameSuite
Array(LiteralValue(123, IntegerType), LiteralValue(56, IntegerType)))))

val alterExecCol2 = executeAndKeepPhysicalPlan[AlterTableExec] {
sql(s"ALTER TABLE $tableName ALTER COLUMN salary SET DEFAULT ('r' || 'l')")
sql(s"ALTER TABLE $tableName ALTER COLUMN dep SET DEFAULT ('r' || 'l')")
}
checkDefaultValue(
alterExecCol2.changes.collect {
Expand All @@ -526,7 +526,7 @@ class DataSourceV2DataFrameSuite
LiteralValue(UTF8String.fromString("l"), StringType)))))

val alterExecCol3 = executeAndKeepPhysicalPlan[AlterTableExec] {
sql(s"ALTER TABLE $tableName ALTER COLUMN salary SET DEFAULT CAST(0 AS BOOLEAN)")
sql(s"ALTER TABLE $tableName ALTER COLUMN active SET DEFAULT CAST(0 AS BOOLEAN)")
}
checkDefaultValue(
alterExecCol3.changes.collect {
Expand Down
40 changes: 38 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3552,6 +3552,42 @@ class DataSourceV2SQLSuiteV1Filter
}
}

test("CREATE TABLE with invalid default value") {
withSQLConf(SQLConf.DEFAULT_COLUMN_ALLOWED_PROVIDERS.key -> s"$v2Format, ") {
withTable("t") {
// The default value fails to analyze.
checkError(
exception = intercept[AnalysisException] {
sql(s"create table t(s int default badvalue) using $v2Format")
},
condition = "INVALID_DEFAULT_VALUE.UNRESOLVED_EXPRESSION",
parameters = Map(
"statement" -> "CREATE TABLE",
"colName" -> "`s`",
"defaultValue" -> "badvalue"))
}
}
}

test("REPLACE TABLE with invalid default value") {
withSQLConf(SQLConf.DEFAULT_COLUMN_ALLOWED_PROVIDERS.key -> s"$v2Format, ") {
withTable("t") {
sql(s"create table t(i boolean) using $v2Format")

// The default value fails to analyze.
checkError(
exception = intercept[AnalysisException] {
sql(s"replace table t(s int default badvalue) using $v2Format")
},
condition = "INVALID_DEFAULT_VALUE.UNRESOLVED_EXPRESSION",
parameters = Map(
"statement" -> "REPLACE TABLE",
"colName" -> "`s`",
"defaultValue" -> "badvalue"))
}
}
}

test("SPARK-52116: Create table with default value which is not deterministic") {
withSQLConf(SQLConf.DEFAULT_COLUMN_ALLOWED_PROVIDERS.key -> v2Source) {
withTable("tab") {
Expand All @@ -3576,7 +3612,7 @@ class DataSourceV2SQLSuiteV1Filter
s"REPLACE TABLE tab (col1 DOUBLE DEFAULT rand()) USING $v2Source")
assert(exception.getSqlState == "42623")
assert(exception.errorClass.get == "INVALID_DEFAULT_VALUE.NON_DETERMINISTIC")
assert(exception.messageParameters("statement") == "CREATE TABLE")
assert(exception.messageParameters("statement") == "REPLACE TABLE")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an error message bug fix, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea , my mistake before :)

assert(exception.messageParameters("colName") == "`col1`")
assert(exception.messageParameters("defaultValue") == "rand()")
}
Expand All @@ -3593,7 +3629,7 @@ class DataSourceV2SQLSuiteV1Filter
s"ALTER TABLE tab ADD COLUMN col2 DOUBLE DEFAULT rand()")
assert(exception.getSqlState == "42623")
assert(exception.errorClass.get == "INVALID_DEFAULT_VALUE.NON_DETERMINISTIC")
assert(exception.messageParameters("statement") == "ALTER TABLE")
assert(exception.messageParameters("statement") == "ALTER TABLE ADD COLUMNS")
assert(exception.messageParameters("colName") == "`col2`")
assert(exception.messageParameters("defaultValue") == "rand()")
}
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/sources/InsertSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,7 @@ class InsertSuite extends DataSourceTest with SharedSparkSession {
exception = intercept[AnalysisException] {
sql("create table t(i boolean, s bigint default badvalue) using parquet")
},
condition = "INVALID_DEFAULT_VALUE.NOT_CONSTANT",
condition = "INVALID_DEFAULT_VALUE.UNRESOLVED_EXPRESSION",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good improvement.

parameters = Map(
"statement" -> "CREATE TABLE",
"colName" -> "`s`",
Expand Down