Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20866 )
Change subject: IMPALA-12412: Support partition evolution in OPTIMIZE statement ...................................................................... Patch Set 10: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/20866/8/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/20866/8/fe/src/main/java/org/apache/impala/service/Frontend.java@2613 PS8, Line 2613: tMt_dop(0); > I can't see too much need for that INSERT check. The original 'addFinalizat Yes, the naming is a tough one :) I could only think of 'addFinalizationParamsForIcebergDelUpdOpt()' or 'addFinalizationParamsForNonInsertIcebergDml()'. The first is only a bit longer than 'addFinalizationParamsForIcebergModify()'. I understand that these names are very long but I think clarity of function names is more important than avoiding extra long function names. In any case, we should state in a function comment that INSERT is not a valid argument for this function. http://gerrit.cloudera.org:8080/#/c/20866/8/fe/src/main/java/org/apache/impala/service/Frontend.java@2613 PS8, Line 2613: tMt_dop(0); > This could also be TIcebergOperation.INSERT, we should add a DCHECK that it Before this change the functions took separate types for DELETE (DeleteStmt) and UPDATE (UpdateStmt), so accidentally passing an INSERT was not possible. Now the type is indicated by the TIcebergOperation param, so it's possible to pass an InsertStmt, and based on the signature one may think that that would be valid. I think it's worth to add a Preconditions check to prevent possible future errors. -- To view, visit http://gerrit.cloudera.org:8080/20866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a0c8529d274afff38ccd582f1b8a857716b1b5 Gerrit-Change-Number: 20866 Gerrit-PatchSet: 10 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 28 Feb 2024 12:49:29 +0000 Gerrit-HasComments: Yes
