Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17313 )
Change subject: IMPALA-10656: Fire insert events before commit ...................................................................... Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4672 PS8, Line 4672: an > nit, s/and/an Done http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4868 PS8, Line 4868: * insert. : * @param table The target table. : * @param updatedPartitions All affected partitions with the list of new files : * inserted. : * @param addedPartitionNames List of new partitions created during the insert. : * @param isInsertOverwrite Indicates if the operation was an insert overwrite. : * @param tblTxn Contains the transactionId and the writeId for the insert. > can you update the comment with the new params? Done http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4902 PS8, Line 4902: if (!isTransactional) { : // add_partitions() RPC have already fired events for new partitions in the : // non-transactional case. : partSet = new HashSet<String>(partSet); : partSet.removeAll(addedPartitionNames); : } > It is not clear to me why we do this for non-transactional tables. IIUC add It is also not clear to me, but we worked like this before this patch. This is both mentioned in comments and code works this way if I understood correctly. See line 4843 in the original commit - filesBeforeInsert only contains partitions which are not new as we only load affectedExistingPartitions. Then again in line 4901 we only load partitions that have already existed to partsPostInsert, and then generate the events based on partsPostInsert, and later fire events for the new partitions only in the transactional case. http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4912 PS8, Line 4912: ition w > I think it is worth a comment here to mention that in case of unpartitioned Done. This is the way we "name" them in general, not just in BE. http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4916 PS8, Line 4916: Preconditions > nit, move the preconditions above line 4914. Done http://gerrit.cloudera.org:8080/#/c/17313/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4926 PS8, Line 4926: Preconditions.checkState(!newFiles.isEmpty() || isInsertOverwrite); : Preconditions.checkState(!partVals.isEmpty()); > nit, move the preconditions check before line 4924 Done -- To view, visit http://gerrit.cloudera.org:8080/17313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ed812dbcb5f55efff3a910a3daeeb76cd3295b9 Gerrit-Change-Number: 17313 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 20:48:28 +0000 Gerrit-HasComments: Yes
