Daniel Vanko has posted comments on this change. ( http://gerrit.cloudera.org:8080/23699 )
Change subject: IMPALA-14536: Fix CONVERT TO ICEBERG to not throw exception on Iceberg tables ...................................................................... Patch Set 8: (4 comments) Thank you everyone for taking a look. Just a minor change and rebase compared to Patch Set 6 and answering your comments. http://gerrit.cloudera.org:8080/#/c/23699/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/23699/5/be/src/service/client-request-state.cc@1478 PS5, Line 1478: << Substitute(error_msg, ExecStateToString(old_state), > Optional: IMO, lack of parentheses makes this harder to read, Based on Noémi's idea I merged the if statement to the assignment above. http://gerrit.cloudera.org:8080/#/c/23699/5/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/23699/5/common/thrift/Frontend.thrift@752 PS5, Line 752: end up being > nit: "end up being NO_OP" Done http://gerrit.cloudera.org:8080/#/c/23699/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/23699/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@254 PS5, Line 254: public AlterTableStmt getAlterTableStmt() { > Oh, I see now. You're calling parsedStmt_.isNoOp() Based on your ideas, I could remove AnalysisResult.isNoOp() and use StatementBase.isNoOp() instead. http://gerrit.cloudera.org:8080/#/c/23699/5/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/23699/5/fe/src/main/java/org/apache/impala/service/Frontend.java@3034 PS5, Line 3034: if (analysisResult.isCatalogOp()) { : result.stmt_type = TStmtType.DDL; : createCatalogOpRequest(analysisResult, result); : TLineageGraph thriftLineageGraph = analysisResult.getThriftLineageGraph(); : if (thriftLineageGraph != null && thriftLineageGraph.isSetQuery_text()) { : > Instead of having this if stmt here, we could have a method in StatementBas Done, also moved the NO_OP check into the existing analysisResult.isConvertTableToIcebergStmt() branch. -- To view, visit http://gerrit.cloudera.org:8080/23699 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41ecbfd350d38e4e3fd7b813a4fc27211d828f73 Gerrit-Change-Number: 23699 Gerrit-PatchSet: 8 Gerrit-Owner: Daniel Vanko <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 08 Dec 2025 14:05:44 +0000 Gerrit-HasComments: Yes
