Steve Carlin 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 5: Code-Review+1 (2 comments) LGTM, nits are optional 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: valid_transition = valid_transition || old_state == ExecState::INITIALIZED; Optional: IMO, lack of parentheses makes this harder to read, I'm gonna put this as optional since I know some people believe "the less characters we have, the better", and I'll be honest, I haven't dealt with C++ in years. But if it were me coding, I'd put in the parentheses characters. http://gerrit.cloudera.org:8080/#/c/23699/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteParsedStatement.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteParsedStatement.java: http://gerrit.cloudera.org:8080/#/c/23699/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteParsedStatement.java@80 PS5, Line 80: return isNoOp_; Optional: just return false? I'm ok if you want to leave this the way it is. -- 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: 5 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: Wed, 03 Dec 2025 13:31:00 +0000 Gerrit-HasComments: Yes
