This is an automated email from the ASF dual-hosted git repository. michaelsmith pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 9d112dae23870b6729473047da94f1bc0ea89ceb Author: Daniel Vanko <[email protected]> AuthorDate: Wed Nov 19 17:56:06 2025 +0100 IMPALA-14536: Fix CONVERT TO ICEBERG to not throw exception on Iceberg tables Previously, running ALTER TABLE <table> CONVERT TO ICEBERG on an Iceberg table produced an error. This patch fixes that, so the statement will do nothing when called on an Iceberg table and return with 'Table has already been migrated.' message. This is achieved by adding a new flag to StatementBase to signal when a statement ends up NO_OP, if that's true, the new TStmtType::NO_OP will be set as TExecRequest's type and noop_result can be used to set result from Frontend-side. Tests: * extended fe and e2e tests Change-Id: I41ecbfd350d38e4e3fd7b813a4fc27211d828f73 Reviewed-on: http://gerrit.cloudera.org:8080/23699 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Peter Rozsa <[email protected]> --- be/src/service/client-request-state.cc | 13 +++++++++++-- common/thrift/Frontend.thrift | 4 ++++ common/thrift/Types.thrift | 1 + .../apache/impala/analysis/ConvertTableToIcebergStmt.java | 15 +++++++++++++++ .../java/org/apache/impala/analysis/StatementBase.java | 12 ++++++++++++ fe/src/main/java/org/apache/impala/service/Frontend.java | 13 ++++++++++--- .../java/org/apache/impala/analysis/AnalyzeStmtsTest.java | 1 + .../iceberg-migrate-from-external-hdfs-tables.test | 5 +++++ 8 files changed, 59 insertions(+), 5 deletions(-) diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc index ef9147c83..2ef6efb86 100644 --- a/be/src/service/client-request-state.cc +++ b/be/src/service/client-request-state.cc @@ -388,6 +388,12 @@ Status ClientRequestState::Exec() { DCHECK(exec_req.__isset.kill_query_request); LOG_AND_RETURN_IF_ERROR(ExecKillQueryRequest()); break; + case TStmtType::NO_OP: + if (exec_req.__isset.noop_result) { + SetResultSet(exec_req.noop_result); + } + return Status::OK(); + break; default: return Status(Substitute("Unknown exec request stmt type: $0", exec_req.stmt_type)); } @@ -1464,8 +1470,11 @@ void ClientRequestState::UpdateNonErrorExecState(ExecState new_state) { // valid to transition from ERROR to FINISHED, so skip any attempt to do so. if (old_state != ExecState::ERROR) { // A query can transition from PENDING to FINISHED if it is cancelled by the - // client. - DCHECK(old_state == ExecState::PENDING || old_state == ExecState::RUNNING) + // client. NO_OP statements can also transition from INITIALIZED to FINISHED. + bool valid_transition = + old_state == ExecState::PENDING || old_state == ExecState::RUNNING + || (stmt_type() == TStmtType::NO_OP && old_state == ExecState::INITIALIZED); + DCHECK(valid_transition) << Substitute(error_msg, ExecStateToString(old_state), ExecStateToString(new_state), PrintId(query_id())); UpdateExecState(new_state); diff --git a/common/thrift/Frontend.thrift b/common/thrift/Frontend.thrift index 86e5a6bb0..affb317ae 100644 --- a/common/thrift/Frontend.thrift +++ b/common/thrift/Frontend.thrift @@ -750,6 +750,10 @@ struct TExecRequest { // Request for "KILL QUERY" statements. 26: optional TKillQueryReq kill_query_request + + // Result of statements which end up being NO_OP. Set iff stmt_type is NO_OP. E.g., + // "ALTER TABLE ... CONVERT TO ICEBERG" when the table is already an Iceberg table. + 27: optional list<string> noop_result } // Parameters to FeSupport.cacheJar(). diff --git a/common/thrift/Types.thrift b/common/thrift/Types.thrift index 9d25ac818..85686abbf 100644 --- a/common/thrift/Types.thrift +++ b/common/thrift/Types.thrift @@ -115,6 +115,7 @@ enum TStmtType { CONVERT = 8 UNKNOWN = 9 KILL = 10 + NO_OP = 11 } enum TIcebergOperation { diff --git a/fe/src/main/java/org/apache/impala/analysis/ConvertTableToIcebergStmt.java b/fe/src/main/java/org/apache/impala/analysis/ConvertTableToIcebergStmt.java index 6048c9367..e59309758 100644 --- a/fe/src/main/java/org/apache/impala/analysis/ConvertTableToIcebergStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/ConvertTableToIcebergStmt.java @@ -20,6 +20,8 @@ package org.apache.impala.analysis; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.Maps; + +import java.util.Collections; import java.util.List; import java.util.Map; import org.apache.hadoop.hive.metastore.api.StorageDescriptor; @@ -32,6 +34,7 @@ import org.apache.impala.analysis.QueryStringBuilder.Rename; import org.apache.impala.analysis.QueryStringBuilder.SetTblProps; import org.apache.impala.authorization.Privilege; import org.apache.impala.catalog.FeFsTable; +import org.apache.impala.catalog.FeIcebergTable; import org.apache.impala.catalog.FeTable; import org.apache.impala.catalog.IcebergTable; import org.apache.impala.catalog.Table; @@ -88,6 +91,13 @@ public class ConvertTableToIcebergStmt extends StatementBase implements SingleTa // table. Once it's fixed, ALL privileges on the table are enough. analyzer.getDb(tableName_.getDb(), Privilege.ALL); FeTable table = analyzer.getTable(tableName_, Privilege.ALL); + + // Do nothing if the table is already an Iceberg table. + if (table instanceof FeIcebergTable) { + setIsNoOp(); + return; + } + if (!(table instanceof FeFsTable)) { throw new AnalysisException("CONVERT TO ICEBERG is not supported for " + table.getClass().getSimpleName()); @@ -209,6 +219,11 @@ public class ConvertTableToIcebergStmt extends StatementBase implements SingleTa return TableName.parse(tmpTableNameStr); } + @Override + public List<String> getNoopSummary() throws AnalysisException { + return Collections.singletonList("Table has already been migrated."); + } + public TConvertTableRequest toThrift() { Preconditions.checkNotNull(tableName_); Preconditions.checkNotNull(tmpHdfsTableName_); diff --git a/fe/src/main/java/org/apache/impala/analysis/StatementBase.java b/fe/src/main/java/org/apache/impala/analysis/StatementBase.java index 69b5ddba1..703c6d18b 100644 --- a/fe/src/main/java/org/apache/impala/analysis/StatementBase.java +++ b/fe/src/main/java/org/apache/impala/analysis/StatementBase.java @@ -44,6 +44,9 @@ public abstract class StatementBase extends StmtNode { // True if this Stmt is the top level of an explain stmt. protected boolean isExplain_ = false; + // True if the statement is noop. + protected boolean isNoOp_ = false; + ///////////////////////////////////////// // BEGIN: Members that need to be reset() @@ -61,6 +64,7 @@ public abstract class StatementBase extends StmtNode { protected StatementBase(StatementBase other) { analyzer_ = other.analyzer_; isExplain_ = other.isExplain_; + isNoOp_ = other.isNoOp_; } /** @@ -167,6 +171,14 @@ public abstract class StatementBase extends StmtNode { public void setIsExplain() { isExplain_ = true; } public boolean isExplain() { return isExplain_; } + public void setIsNoOp() { isNoOp_ = true; } + public boolean isNoOp() { return isNoOp_; } + + public List<String> getNoopSummary() throws AnalysisException { + throw new IllegalStateException( + "getNoopSummary() not implemented for this stmt: " + getClass().getSimpleName()); + } + /** * Returns a deep copy of this node including its analysis state. Some members such as * tuple and slot descriptors are generally not deep copied to avoid potential diff --git a/fe/src/main/java/org/apache/impala/service/Frontend.java b/fe/src/main/java/org/apache/impala/service/Frontend.java index f601c5e73..b08c6ff72 100644 --- a/fe/src/main/java/org/apache/impala/service/Frontend.java +++ b/fe/src/main/java/org/apache/impala/service/Frontend.java @@ -76,6 +76,7 @@ import org.apache.impala.analysis.AlterDbStmt; import org.apache.impala.analysis.AnalysisContext; import org.apache.impala.analysis.AnalysisContext.AnalysisResult; import org.apache.impala.analysis.CommentOnStmt; +import org.apache.impala.analysis.ConvertTableToIcebergStmt; import org.apache.impala.analysis.CopyTestCaseStmt; import org.apache.impala.analysis.CreateDataSrcStmt; import org.apache.impala.analysis.CreateDropRoleStmt; @@ -3097,11 +3098,17 @@ public class Frontend { result.setAdmin_request(analysisResult.getAdminFnStmt().toThrift()); return result; } else if (analysisResult.isConvertTableToIcebergStmt()) { - result.stmt_type = TStmtType.CONVERT; result.setResult_set_metadata(new TResultSetMetadata( Collections.singletonList(new TColumn("summary", Type.STRING.toThrift())))); - result.setConvert_table_request( - analysisResult.getConvertTableToIcebergStmt().toThrift()); + ConvertTableToIcebergStmt stmt = analysisResult.getConvertTableToIcebergStmt(); + if (stmt.isNoOp()) { + result.setStmt_type(TStmtType.NO_OP); + result.setNoop_result(stmt.getNoopSummary()); + } else { + result.setStmt_type(TStmtType.CONVERT); + result.setConvert_table_request( + analysisResult.getConvertTableToIcebergStmt().toThrift()); + } return result; } else if (analysisResult.isTestCaseStmt()) { CopyTestCaseStmt testCaseStmt = ((CopyTestCaseStmt) analysisResult.getStmt()); diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java index f7df74938..a9b450856 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java @@ -5234,5 +5234,6 @@ public class AnalyzeStmtsTest extends AnalyzerTest { AnalysisError("alter table functional_parquet.tinytable convert to iceberg" + " tblproperties('metadata.generator.threads'='a1')", "CONVERT TO ICEBERG doesn't accept 'metadata.generator.threads' as TBLPROPERTY."); + AnalyzesOk("alter table functional_parquet.iceberg_alltypes_part convert to iceberg"); } } diff --git a/testdata/workloads/functional-query/queries/QueryTest/iceberg-migrate-from-external-hdfs-tables.test b/testdata/workloads/functional-query/queries/QueryTest/iceberg-migrate-from-external-hdfs-tables.test index ed41307eb..671a91355 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/iceberg-migrate-from-external-hdfs-tables.test +++ b/testdata/workloads/functional-query/queries/QueryTest/iceberg-migrate-from-external-hdfs-tables.test @@ -78,6 +78,11 @@ alter table parquet_partitioned convert to iceberg tblproperties('iceberg.catalo 'Table has been migrated.' ==== ---- QUERY +alter table parquet_partitioned convert to iceberg tblproperties('iceberg.catalog' = 'hadoop.tables'); +---- RESULTS +'Table has already been migrated.' +==== +---- QUERY select count(*) from parquet_partitioned; ---- RESULTS 7301
