Skip to content

[SPARK-23525] [SQL] Support ALTER TABLE CHANGE COLUMN COMMENT for external hive table #20696

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
jiangxb1987 wants to merge 3 commits into from
Closed

[SPARK-23525] [SQL] Support ALTER TABLE CHANGE COLUMN COMMENT for external hive table #20696

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92b2f31
support change column comment for external hive table
jiangxb1987 Feb 28, 2018
6035d7e
add test case for change partition column.
jiangxb1987 Mar 1, 2018
48fc338
update error message.
jiangxb1987 Mar 7, 2018
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
9 changes: 4 additions & 5 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,8 +314,8 @@ case class AlterTableChangeColumnCommand(
val resolver = sparkSession.sessionState.conf.resolver
DDLUtils.verifyAlterTableType(catalog, table, isView = false)

// Find the origin column from schema by column name.
val originColumn = findColumnByName(table.schema, columnName, resolver)
// Find the origin column from dataSchema by column name.
val originColumn = findColumnByName(table.dataSchema, columnName, resolver)

@gatorsmile gatorsmile Feb 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only data schema? How about partitioning schema?

@jiangxb1987 jiangxb1987 Mar 1, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As described in the comment, we only support altering non-partition columns:

 A command to change the column for a table, only support changing the comment of a non-partition
 column for now.

@gatorsmile gatorsmile Mar 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have a negative test case to cover that?

// Throw an AnalysisException if the column name/dataType is changed.
if (!columnEqual(originColumn, newColumn, resolver)) {
throw new AnalysisException(
Expand All @@ -324,16 +324,15 @@ case class AlterTableChangeColumnCommand(
s"'${newColumn.name}' with type '${newColumn.dataType}'")
}

val newSchema = table.schema.fields.map { field =>
val newDataSchema = table.dataSchema.fields.map { field =>
if (field.name == originColumn.name) {
// Create a new column from the origin column with the new comment.
addComment(field, newColumn.getComment)
} else {
field
}
}
val newTable = table.copy(schema = StructType(newSchema))
catalog.alterTable(newTable)
catalog.alterTableDataSchema(tableName, StructType(newDataSchema))

Seq.empty[Row]
}
Expand Down
1 change: 1 addition & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,6 +1597,7 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
// Ensure that change column will preserve other metadata fields.
sql("ALTER TABLE dbx.tab1 CHANGE COLUMN col1 col1 INT COMMENT 'this is col1'")
assert(getMetadata("col1").getString("key") == "value")
assert(getMetadata("col1").getString("comment") == "this is col1")

@jiangxb1987 jiangxb1987 Feb 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't verify the comment is correctly set to the column metadata, and that caused the issue. :(

}

test("drop build-in function") {
Expand Down