HIVE-15984: Allow ALTER TABLE DROP COLUMN #5715

ramitg254 · 2025-03-25T06:24:21Z

What changes were proposed in this pull request?

The changes allow dropping column via alter table drop column

Why are the changes needed?

Before, the drop column query was not there as regarding sql coverage.

Does this PR introduce any user-facing change?

Now, user can also drop column via alter table drop column and no need to use replace columns for it

Is the change a dependency upgrade?

No

How was this patch tested?

I tested it on my local machine.

Copilot

Pull Request Overview

This pull request adds support for the ALTER TABLE DROP COLUMN command in Hive. Key changes include the introduction of a new descriptor and analyzer for the drop column syntax, updates to the Iceberg meta hook to handle drop column operations, and adjustments in privilege and error messaging to facilitate the new functionality.

Reviewed Changes

Copilot reviewed 14 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnDesc.java	New descriptor for DROP COLUMN operations
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java	Updated to support the drop column operation and enforce single-column drop semantics
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnAnalyser.java	Analyzer implementation for the DROP COLUMN statement
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnOperation.java	Implementation of the drop column alteration including SerDe handling and column removal logic
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java	New error messages for drop column operations
Others	Updates in enums, privileges, and test cases to accommodate the new drop column behavior

Files not reviewed (9)

iceberg/iceberg-handler/src/test/queries/positive/iceberg_drop_column.q: Language not supported
iceberg/iceberg-handler/src/test/results/positive/iceberg_drop_column.q.out: Language not supported
parser/src/java/org/apache/hadoop/hive/ql/parse/AlterClauseParser.g: Language not supported
parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g: Language not supported
ql/src/test/queries/clientnegative/orc_drop_column.q: Language not supported
ql/src/test/queries/clientnegative/parquet_drop_column.q: Language not supported
ql/src/test/queries/clientpositive/orc_drop_column.q: Language not supported
ql/src/test/queries/clientpositive/parquet_drop_column.q: Language not supported
ql/src/test/results/clientnegative/orc_drop_column.q.out: Language not supported

Comments suppressed due to low confidence (2)

iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:948

The code throws an exception when more than one column is detected for a drop operation; please verify that supporting only single-column drops is the intended behavior.

if (schemaDifference.getMissingFromFirst().size() > 1) {

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnOperation.java:61

[nitpick] Consider defining a constant for the SerDe string 'org.apache.hadoop.hive.serde.thrift.columnsetSerDe' to improve maintainability and reduce potential typos.

if ("org.apache.hadoop.hive.serde.thrift.columnsetSerDe".equals(serializationLib)) {

soumyakanti3578 · 2025-03-28T22:43:54Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnAnalyser.java

+    @Override
+    protected void analyzeCommand(TableName tableName, Map<String, String> partitionSpec, ASTNode command)
+            throws SemanticException {
+        boolean ifExists = (command.getFirstChildWithType(HiveParser.TOK_IFEXISTS)!=null);


These parantheses are probably unnecessary here.

removed parenthesis

soumyakanti3578 · 2025-03-28T22:46:47Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnOperation.java

+            }
+        }
+        if(droppingIndex==-1){
+            if(desc.isIfExists())return;


Using braces for an if block even when the block consists of a single line of code is a good practice to follow :)

added braces

soumyakanti3578 · 2025-03-28T22:47:43Z

ql/src/test/queries/clientnegative/orc_drop_column.q

+SET hive.exec.schema.evolution=true;
+create table src_orc (key int, val string) stored as orc;
+insert into src_orc values (1, "Alice");
+alter table src_orc drop column val;


Please add a newline at EOF for this and other .q files

added in it and others as well

soumyakanti3578

I have reviewed this only partially. Other than specific comments, I found that the spacing is inconsistent in various places. It is a good practice to not deviate from the coding convention for better readability and maintainability. You might find this helpful: https://cwiki.apache.org/confluence/display/Hive/HowToContribute#HowToContribute-CodingConventions

soumyakanti3578 · 2025-03-31T22:39:15Z

iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java

+      throw new MetaException("Unsupported operation to use DROP COLUMN for adding a column, changing a " +
+              "column type, column comment or reordering columns. Only use DROP COLUMN for dropping a column. " +
+              "For the other operations, consider using the ADD COLUMNS or CHANGE COLUMN commands.");
+    }


It looks like this Exception is handling too many scenarios. It might be alright but this could be broken into more specific scenarios. What do you think?

I'll separate them

soumyakanti3578 · 2025-03-31T22:49:16Z

ql/src/test/results/clientpositive/llap/orc_drop_column.q.out

+POSTHOOK: type: QUERY
+POSTHOOK: Input: default@src_orc
+#### A masked pattern was here ####
+1


nit: In these tests it might be useful to set hive.cli.print.header=true to get the column names in the q.out files.

okay I'll add that

soumyakanti3578 · 2025-03-31T22:50:03Z

ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java

@@ -45,6 +45,8 @@ public enum HiveOperation {
  MSCK("MSCK", HiveParser.TOK_MSCK, null, null),
  ALTERTABLE_ADDCOLS("ALTERTABLE_ADDCOLS", HiveParser.TOK_ALTERTABLE_ADDCOLS, new Privilege[]{Privilege.ALTER_METADATA},
      null),
+  ALTERTABLE_DROPCOL("ALTERTABLE_DROPCOL",HiveParser.TOK_ALTERTABLE_DROPCOL,new Privilege[]{Privilege.ALTER_METADATA},


soumyakanti3578

Consider adding more tests as this is an important new feature:

Negative: Dropping a column which is not present
Negative: Dropping a column twice
Have 2-3 partitions in the partitioned tables
Test drop column with show commands like show columns, show create table
Test with describe [formatted] as they list columns too

Also any other aspect which you can think of :)

ramitg254 · 2025-04-13T07:01:09Z

Consider adding more tests as this is an important new feature:

Negative: Dropping a column which is not present

Negative: Dropping a column twice

Have 2-3 partitions in the partitioned tables

Test drop column with show commands like show columns, show create table

Test with describe [formatted] as they list columns too

Also any other aspect which you can think of :)

tests added

deniskuzZ · 2025-04-14T08:52:52Z

common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java

@@ -362,6 +363,8 @@ public enum ErrorMsg {
  DBTXNMGR_REQUIRES_CONCURRENCY(10264,
      "To use DbTxnManager you must set hive.support.concurrency=true"),
  TXNMGR_NOT_ACID(10265, "This command is not allowed on an ACID table {0}.{1} with a non-ACID transaction manager", true),
+  COLUMN_NOT_FOUND(10266, "Invalid Column Name"),


Use INVALID_COLUMN instead

I'll change that

done, used already present INVALID_COLUMN_NAME in place of it

deniskuzZ · 2025-04-14T08:59:31Z

common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java

@@ -362,6 +363,8 @@ public enum ErrorMsg {
  DBTXNMGR_REQUIRES_CONCURRENCY(10264,
      "To use DbTxnManager you must set hive.support.concurrency=true"),
  TXNMGR_NOT_ACID(10265, "This command is not allowed on an ACID table {0}.{1} with a non-ACID transaction manager", true),
+  COLUMN_NOT_FOUND(10266, "Invalid Column Name"),
+  DROP_COLUMN_UNCASCADED(10267, "Drop column is not supported without cascade for partitioned table. SerDe may be incompatible.", true),


I don't see an equivalent for REPLACE_COLUMNS, maybe stick with CANNOT_DROP_COLUMN?

this one is added for more explainatory exception in case of dropping columns without cascade in case of parquet:

if (ParquetHiveSerDe.isParquetTable(table) && AlterTableUtils.isSchemaEvolutionEnabled(table, context.getConf()) && !desc.isCascade() && table.isPartitioned()) { LOG.warn("Cannot drop column from a partitioned parquet table without the CASCADE option"); throw new HiveException(ErrorMsg.DROP_COLUMN_UNCASCADED, desc.getDbTableName()); }

deniskuzZ · 2025-04-14T09:04:44Z

iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java

+    HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsCols, icebergCols, true);
+
+    if (schemaDifference.getMissingFromFirst().size() > 1) {
+      throw new MetaException("Cannot drop more than one column");


Shouldn't the Parser validate this?

Yeah parser will validate this I added it to be on safer side, I'll change that

Done, removed

deniskuzZ · 2025-04-14T09:08:16Z

iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java

+      throw new MetaException("Unsupported operation to use DROP COLUMN for changing a column comment");
+    }
+
+    if (outOfOrder != null) {


why is that needed?

this and other 3 if are handling the below exception which is present in handleReplaceColumns in an elaborative manner:

if (!schemaDifference.getMissingFromSecond().isEmpty() || !schemaDifference.getTypeChanged().isEmpty() || !schemaDifference.getCommentChanged().isEmpty() || outOfOrder != null) { throw new MetaException("Unsupported operation to use REPLACE COLUMNS for adding a column, changing a " + "column type, column comment or reordering columns. Only use REPLACE COLUMNS for dropping columns. " + "For the other operations, consider using the ADD COLUMNS or CHANGE COLUMN commands."); }

deniskuzZ · 2025-04-14T09:18:34Z

iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java

@@ -927,6 +931,52 @@ private void handleAddColumns(org.apache.hadoop.hive.metastore.api.Table hmsTabl
    updateSchema.commit();
  }

+  private void handleDropColumn(org.apache.hadoop.hive.metastore.api.Table hmsTable) throws MetaException {


Why not simply reuse handleReplaceColumns?

Because handleReplaceColumns is also handling addition of columns and changing a column

deniskuzZ · 2025-04-14T09:20:24Z

parser/src/java/org/apache/hadoop/hive/ql/parse/AlterClauseParser.g

+@init { gParent.pushMsg("drop column statement", state); }
+@after { gParent.popMsg(state); }
+    : KW_DROP KW_COLUMN ifExists? columnName restrictOrCascade?
+    -> ^(TOK_ALTERTABLE_DROPCOL ifExists? columnName restrictOrCascade?)


Do we have tests for restrictOrCascade?

there is test for cascade added when using desc formatted table and desc formatted table partition (something) yield different results for columns present when a column is dropped without cascade as in case of specific partition it also contains dropped column in case normal hive tables;

there is no test for restrict , i added it because it was also present in case of parsing grammer of replace columns although for no use

deniskuzZ · 2025-04-14T09:24:23Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AlterTableType.java

@@ -30,6 +30,7 @@
 public enum AlterTableType {
  // column
  ADDCOLS("add columns"),
+  DROPCOL("drop column"),


Please use full name DROP_COLUMN

I'll change that

Done, changed

deniskuzZ · 2025-04-14T09:30:14Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnAnalyser.java

+    protected void analyzeCommand(TableName tableName, Map<String, String> partitionSpec, ASTNode command)
+            throws SemanticException {
+        boolean ifExists = command.getFirstChildWithType(HiveParser.TOK_IFEXISTS) != null;
+        String colName = ifExists ? command.getChild(1).getText().toLowerCase() : command.getChild(0).getText().toLowerCase();


String colName = command.getChild((ifExists) ? 1 : 0).getText().toLowerCase();

I'll change that

deniskuzZ · 2025-04-14T09:31:41Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnAnalyser.java

+        boolean ifExists = command.getFirstChildWithType(HiveParser.TOK_IFEXISTS) != null;
+        String colName = ifExists ? command.getChild(1).getText().toLowerCase() : command.getChild(0).getText().toLowerCase();
+        boolean isCascade = false;
+        if (null != command.getFirstChildWithType(HiveParser.TOK_CASCADE)) {


why do you need if, just assign the result from bool condition

I'll change that

deniskuzZ · 2025-04-14T09:35:10Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnOperation.java

+        super(context, desc);
+    }
+
+    private static final Set<String> VALID_SERIALIZATION_LIBS = ImmutableSet.of(


where is this used?

It can be removed it came by mistake from AlterTableReplaceColumnsOperation

done, removed

deniskuzZ · 2025-04-14T09:38:20Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnOperation.java

+/**
+ * Operation process of dropping column.
+ */
+public class AlterTableDropColumnOperation extends AbstractAlterTableOperation<AlterTableDropColumnDesc> {


should it be different from AlterTableReplaceColumnsOperation?

AlterTableReplaceColumnsOperation is replacing the current columnNameTypeList with provided columnNameTypeList in the query whereas AlterTableDropColumnOperation is dropping that particular column from list of columns whose name is specified in the query.

deniskuzZ · 2025-04-14T10:40:37Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/drop/AlterTableDropColumnDesc.java

+/**
+ * DDL task description for ALTER TABLE ... DROP COLUMN ... command.
+ */
+@Explain(displayName = "Drop Column", explainLevels = {Level.USER, Level.DEFAULT, Level.EXTENDED})


i can't find any tests that validate 'EXPLAIN'

I'll modify the tests for explain

done , added explain statement in tests

sonarqubecloud · 2025-04-15T08:28:42Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

asf-ci-hive added tests pending tests failed and removed tests pending tests failed labels Mar 25, 2025

deniskuzZ requested a review from Copilot March 26, 2025 18:56

Copilot AI reviewed Mar 26, 2025

View reviewed changes

ramitg254 force-pushed the HIVE-15984 branch from 054d634 to c195344 Compare March 28, 2025 15:54

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending labels Mar 28, 2025

ramitg254 force-pushed the HIVE-15984 branch from c195344 to 18358f1 Compare March 28, 2025 21:49

asf-ci-hive added tests pending and removed tests unstable labels Mar 28, 2025

ramitg254 force-pushed the HIVE-15984 branch from 18358f1 to 8f2948e Compare March 28, 2025 21:55

asf-ci-hive added tests failed and removed tests pending labels Mar 28, 2025

soumyakanti3578 reviewed Mar 28, 2025

View reviewed changes

asf-ci-hive added tests pending and removed tests failed labels Mar 29, 2025

soumyakanti3578 reviewed Mar 31, 2025

View reviewed changes

ramitg254 force-pushed the HIVE-15984 branch from 29b8f4e to b4af83b Compare April 12, 2025 20:24

asf-ci-hive added tests pending tests passed and removed tests passed tests pending labels Apr 12, 2025

deniskuzZ reviewed Apr 14, 2025

View reviewed changes

HIVE-15984: Allow ALTER TABLE DROP COLUMN

20a3c77

ramitg254 force-pushed the HIVE-15984 branch from b4af83b to 20a3c77 Compare April 15, 2025 07:33

asf-ci-hive added tests pending and removed tests passed labels Apr 15, 2025

ramitg254 requested a review from deniskuzZ April 15, 2025 08:20

asf-ci-hive added tests passed and removed tests pending labels Apr 15, 2025

HIVE-15984: Allow ALTER TABLE DROP COLUMN #5715

Are you sure you want to change the base?

HIVE-15984: Allow ALTER TABLE DROP COLUMN #5715

Conversation

ramitg254 commented Mar 25, 2025 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed 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

soumyakanti3578 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyakanti3578 Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyakanti3578 left a comment

Choose a reason for hiding this comment

ramitg254 commented Apr 13, 2025

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

ramitg254 Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramitg254 Apr 14, 2025 • edited Loading

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

ramitg254 Apr 14, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramitg254 Apr 14, 2025 • edited Loading

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

sonarqubecloud bot commented Apr 15, 2025

Quality Gate passed

ramitg254 commented Mar 25, 2025 •

edited

Loading

soumyakanti3578 Mar 31, 2025 •

edited

Loading

ramitg254 Apr 14, 2025 •

edited

Loading

ramitg254 Apr 14, 2025 •

edited

Loading

ramitg254 Apr 14, 2025 •

edited

Loading

ramitg254 Apr 14, 2025 •

edited

Loading