Support dropping columns as a metadata-only operation #1076

liwensun · 2022-04-15T23:08:16Z

Description

This PR proposes to support ALTER TABLE DROP COLUMN as a metadata-only operation in Delta. No changes needed for the underlying Parquet files. This is only supported for Delta tables that enable column mapping. With column mapping, if a new column with the same name is added later, it won't be confused with the dropped column because the new column will have a different physical name than the one being dropped.

Note that there is no way to physically delete the dropped column from Parquet right now. This will be addressed as future work.

This PR partially addresses issue #1064.

How was this patch tested?

unit tests

Does this PR introduce any user-facing changes?

Add a new SQL command

ALTER TABLE table_name DROP COLUMN col_name
ALTER TABLE table_name DROP COLUMNS (col_name1, col2_name2, ...)

GitOrigin-RevId: 5bac741beced1ec847ebcc07d050234f82ece124

GitOrigin-RevId: 09280364501777e2b0953d56294775457b6adc97

vkorukanti · 2022-04-19T05:26:55Z

core/src/main/scala/org/apache/spark/sql/delta/DeltaErrors.scala

+ private def columnMappingAdviceMessage: String = {
+ s"""
+ |Please upgrade your Delta table to reader version 2 and writer version 5
+ | and change the column mapping mode to name mapping. You can use the following command:


name in quotes?

vkorukanti · 2022-04-19T05:28:48Z

core/src/main/scala/org/apache/spark/sql/delta/DeltaErrors.scala


+ def columnRenameNotSupported: Throwable = {
+ val adviceMsg = columnMappingAdviceMessage
 new AnalysisException(


are we planning to convert the exception to DeltaAnlysisException?

vkorukanti · 2022-04-19T05:31:00Z

core/src/main/scala/org/apache/spark/sql/delta/DeltaErrors.scala

- }
+ def foundInvalidCharsInColumnNames(cause: Throwable): Throwable =
+ new DeltaAnalysisException(
+ errorClass = "UNSUPPORTED_INVALID_CHARACTERS_IN_COLUMN_NAME",


Is it better to include the column name here to make it easy for the user to find the column?

The column name shows up in the child exception, so i think that would be ok. it's a bit involved to surface it to the top level.

vkorukanti · 2022-04-19T05:39:00Z

core/src/main/scala/org/apache/spark/sql/delta/commands/alterDeltaTableCommands.scala

+ override def run(sparkSession: SparkSession): Seq[Row] = {
+ if (!sparkSession.sessionState.conf.getConf(
+ DeltaSQLConf.DELTA_ALTER_TABLE_DROP_COLUMN_ENABLED)) {
+ val errorMessage = "Drop Column is not enabled in Delta."


Drop column is not enabled in current Spark session? May be mention the SQL config to enable it?

not sure if we want to mention this flag right now. this flag is mainly for merging code without the support of physical drop.

vkorukanti · 2022-04-19T05:40:16Z

core/src/main/scala/org/apache/spark/sql/delta/commands/alterDeltaTableCommands.scala

+ val droppedColumnSet = columnsToDrop.map(UnresolvedAttribute(_).name).toSet
+ val droppingPartitionCols = metadata.partitionColumns.exists(droppedColumnSet.contains(_))
+ if (droppingPartitionCols) {
+ throw DeltaErrors.dropPartitionColumnNotSupported


include the partition column name in the error message?

vkorukanti · 2022-04-19T05:45:26Z

core/src/main/scala/org/apache/spark/sql/delta/commands/alterDeltaTableCommands.scala

+ if (sparkSession.sessionState.conf.getConf(
+ DeltaSQLConf.DELTA_ALTER_TABLE_CHANGE_COLUMN_CHECK_EXPRESSIONS)) {
+ columnsToDrop.foreach { columnParts =>
+ // need to check if the column to rename is referenced by check constraints
+ val dependentConstraints =
+ Constraints.findDependentConstraints(sparkSession, columnParts, newMetadata)
+ if (dependentConstraints.nonEmpty) {
+ throw DeltaErrors.foundViolatingConstraintsForColumnChange(
+ "drop", UnresolvedAttribute(columnParts).name, dependentConstraints)
+ }
+
+ // need to check if the renaming the column would affect any generated columns
+ val dependentGenCols = SchemaUtils.findDependentGeneratedColumns(
+ sparkSession, columnParts, txn.protocol, newMetadata.schema)
+ if (dependentGenCols.nonEmpty) {
+ throw DeltaErrors.foundViolatingGeneratedColumnsForColumnChange(
+ "drop", UnresolvedAttribute(columnParts).name, dependentGenCols)
+ }
+ }
+ }


The same code is repeated in RENAME with minimal difference. Convert it to a utility method, so that we don't miss updating one if we add a new check?

vkorukanti · 2022-04-19T05:54:20Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaDropColumnSuite.scala

+ spark.sql(s"alter table t1 rename column map to `${colName("map")}`")
+
+ // try drop map after rename to arbitrary chars now
+ spark.sql(s"alter table t1 drop columns `${colName("map")}`")


Verify it actually removed the column? Same below.

vkorukanti · 2022-04-19T05:56:29Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaDropColumnSuite.scala

+
+ simpleNestedData.write.format("delta").mode("append").saveAsTable("t1")
+
+ assertException("Cannot drop column a") {


check for: Cannot drop column a due to dependency (details on why it can't be dropped)?

vkorukanti · 2022-04-19T05:58:12Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaDropColumnSuite.scala

+class DeltaDropColumnSuite extends QueryTest with DeltaArbitraryColumnNameSuiteBase {
+
+ override protected val sparkConf: SparkConf =
+ super.sparkConf.set(DeltaSQLConf.DELTA_ALTER_TABLE_DROP_COLUMN_ENABLED.key, "true")


Is there a test with DELTA_ALTER_TABLE_DROP_COLUMN_ENABLED set to false and try dropping?

vkorukanti · 2022-04-19T05:59:55Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaDropColumnSuite.scala

+ override protected val sparkConf: SparkConf =
+ super.sparkConf.set(DeltaSQLConf.DELTA_ALTER_TABLE_DROP_COLUMN_ENABLED.key, "true")
+
+ test("drop column") {


There are many test cases within a single test. Is possible to split them into smaller and easy to understand tests?

GitOrigin-RevId: a40fd48ca89428254a1f728703417b9a6359a5fd

GitOrigin-RevId: 9829b0fc55bebbbc970faa0ebec5635057951de9

GitOrigin-RevId: e1d641b61c770f97c0a6b17cf580faeb699de221

vkorukanti

LGTM

Support ALTER TABLE DROP COLUMN as a metadata only operation. This feature is not ready for release yet, so it's currently behind a feature flag. Closes delta-io#1076 Unit tests GitOrigin-RevId: 181d6dc38f5c074eb16710191795118e78fdf972

liwensun added 2 commits April 15, 2022 15:35

drop col works

67407da

GitOrigin-RevId: 5bac741beced1ec847ebcc07d050234f82ece124

add tests

1e6c3c0

GitOrigin-RevId: 09280364501777e2b0953d56294775457b6adc97

liwensun requested review from tdas and vkorukanti April 15, 2022 23:08

liwensun changed the title ~~Support dropping columns as a metadata operation~~ Support dropping columns as a metadata-only operation Apr 15, 2022

fix tests

8330da6

vkorukanti reviewed Apr 19, 2022

View reviewed changes

liwensun added 3 commits April 25, 2022 20:59

blank commit

f1db1b6

GitOrigin-RevId: a40fd48ca89428254a1f728703417b9a6359a5fd

address comments

dcc4e56

GitOrigin-RevId: 9829b0fc55bebbbc970faa0ebec5635057951de9

fix tests

fb6483f

GitOrigin-RevId: e1d641b61c770f97c0a6b17cf580faeb699de221

vkorukanti approved these changes Apr 26, 2022

View reviewed changes

vkorukanti closed this in 58b2510 May 3, 2022

vkorukanti added this to the 2.0 milestone Jun 28, 2022

allisonport-db mentioned this pull request Aug 26, 2022

Support dropping columns from a Delta table #1064

Closed

dennyglee mentioned this pull request Nov 10, 2022

Support column mapping delta-io/delta-rs#930

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dropping columns as a metadata-only operation #1076

Support dropping columns as a metadata-only operation #1076

liwensun commented Apr 15, 2022 •

edited

Loading

vkorukanti Apr 19, 2022

vkorukanti Apr 19, 2022

vkorukanti Apr 19, 2022

liwensun Apr 25, 2022

vkorukanti Apr 19, 2022

liwensun Apr 25, 2022

vkorukanti Apr 19, 2022

vkorukanti Apr 19, 2022

liwensun Apr 25, 2022

vkorukanti Apr 19, 2022

vkorukanti Apr 19, 2022

vkorukanti Apr 19, 2022

vkorukanti Apr 19, 2022

vkorukanti left a comment


		simpleNestedData.write.format("delta").mode("append").saveAsTable("t1")

		assertException("Cannot drop column a") {

Support dropping columns as a metadata-only operation #1076

Support dropping columns as a metadata-only operation #1076

Conversation

liwensun commented Apr 15, 2022 • edited Loading

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkorukanti left a comment

Choose a reason for hiding this comment

liwensun commented Apr 15, 2022 •

edited

Loading