Skip to content

Commit

Permalink
[SPARK-51010][SQL] Fix AlterColumnSpec not reporting resolved status …
Browse files Browse the repository at this point in the history
…correctly

### What changes were proposed in this pull request?

#49559 introduces `AlterColumnSpec`, which has the field `newPosition` that should have been accounted for when calling `resolved`. Because it is currently not accounted for, `AlterColumnSpec` reports `resolved` to be true even when `newPosition` is not resolved.

### Why are the changes needed?

While this is currently not a problem with Spark, this might affect extensions or become a surprise in the future.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

New unit test

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

No

Closes #49705 from ctring/SPARK-51010.

Authored-by: Cuong Nguyen <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
  • Loading branch information
ctring authored and cloud-fan committed Feb 6, 2025
1 parent 88485a3 commit 84e924e
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ case class AlterColumnSpec(
newPosition: Option[FieldPosition],
newDefaultExpression: Option[String]) extends Expression with Unevaluable {

override def children: Seq[Expression] = Seq(column)
override def children: Seq[Expression] = Seq(column) ++ newPosition.toSeq
override def nullable: Boolean = false
override def dataType: DataType = throw new UnresolvedException("dataType")
override protected def withNewChildrenInternal(newChildren: IndexedSeq[Expression]): Expression =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import org.mockito.invocation.InvocationOnMock
import org.apache.spark.SparkUnsupportedOperationException
import org.apache.spark.sql.{AnalysisException, SaveMode}
import org.apache.spark.sql.catalyst.{AliasIdentifier, TableIdentifier}
import org.apache.spark.sql.catalyst.analysis.{AnalysisContext, AnalysisTest, Analyzer, EmptyFunctionRegistry, NoSuchTableException, ResolvedFieldName, ResolvedIdentifier, ResolvedTable, ResolveSessionCatalog, UnresolvedAttribute, UnresolvedInlineTable, UnresolvedRelation, UnresolvedSubqueryColumnAliases, UnresolvedTable}
import org.apache.spark.sql.catalyst.analysis.{AnalysisContext, AnalysisTest, Analyzer, EmptyFunctionRegistry, NoSuchTableException, ResolvedFieldName, ResolvedFieldPosition, ResolvedIdentifier, ResolvedTable, ResolveSessionCatalog, UnresolvedAttribute, UnresolvedFieldPosition, UnresolvedInlineTable, UnresolvedRelation, UnresolvedSubqueryColumnAliases, UnresolvedTable}
import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogStorageFormat, CatalogTable, CatalogTableType, InMemoryCatalog, SessionCatalog, TempVariableManager}
import org.apache.spark.sql.catalyst.expressions.{AttributeReference, Cast, EqualTo, Expression, InSubquery, IntegerLiteral, ListQuery, Literal, StringLiteral}
import org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke
Expand All @@ -36,7 +36,7 @@ import org.apache.spark.sql.catalyst.plans.logical.{AlterColumns, AlterColumnSpe
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.catalyst.util.TypeUtils.toSQLId
import org.apache.spark.sql.connector.FakeV2Provider
import org.apache.spark.sql.connector.catalog.{CatalogManager, Column, ColumnDefaultValue, Identifier, SupportsDelete, Table, TableCapability, TableCatalog, TableWritePrivilege, V1Table}
import org.apache.spark.sql.connector.catalog.{CatalogManager, Column, ColumnDefaultValue, Identifier, SupportsDelete, Table, TableCapability, TableCatalog, TableChange, TableWritePrivilege, V1Table}
import org.apache.spark.sql.connector.catalog.CatalogManager.SESSION_CATALOG_NAME
import org.apache.spark.sql.connector.expressions.{LiteralValue, Transform}
import org.apache.spark.sql.errors.QueryExecutionErrors
Expand Down Expand Up @@ -1389,7 +1389,7 @@ class PlanResolutionSuite extends SharedSparkSession with AnalysisTest {
None,
None))) =>
assert(column.name == Seq("i"))
case _ => fail("expect AlterTableAlterColumn")
case _ => fail("expect AlterColumns")
}

parsed2 match {
Expand All @@ -1403,7 +1403,7 @@ class PlanResolutionSuite extends SharedSparkSession with AnalysisTest {
None,
None))) =>
assert(column.name == Seq("i"))
case _ => fail("expect AlterTableAlterColumn")
case _ => fail("expect AlterColumns")
}

val parsed3 = parseAndResolve(sql3)
Expand All @@ -1427,7 +1427,7 @@ class PlanResolutionSuite extends SharedSparkSession with AnalysisTest {
Some("'value'")))) =>
assert(column1.name == Seq("i"))
assert(column2.name == Seq("s"))
case _ => fail("expect AlterTableAlterColumn")
case _ => fail("expect AlterColumns")
}
}
}
Expand All @@ -1441,6 +1441,25 @@ class PlanResolutionSuite extends SharedSparkSession with AnalysisTest {
comparePlans(parsed, expected)
}

test("SPARK-51010: AlterColumnSpec should correctly report resolved status") {
val unresolvedSpec = AlterColumnSpec(
ResolvedFieldName(Seq("test"), StructField("i", IntegerType)),
newDataType = None,
newNullability = None,
newComment = None,
newPosition = Some(UnresolvedFieldPosition(TableChange.ColumnPosition.first)),
newDefaultExpression = None)
assert(!unresolvedSpec.resolved)

val partiallyResolvedSpec = unresolvedSpec.copy(
column = ResolvedFieldName(Seq("test"), StructField("i", IntegerType)))
assert(!partiallyResolvedSpec.resolved)

val resolvedSpec = partiallyResolvedSpec.copy(
newPosition = Some(ResolvedFieldPosition(TableChange.ColumnPosition.first)))
assert(resolvedSpec.resolved)
}

test("alter table: alter column action is not specified") {
val sql = "ALTER TABLE v1Table ALTER COLUMN i"
checkError(
Expand Down

0 comments on commit 84e924e

Please sign in to comment.