Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16206 )
Change subject: IMPALA-9964: Fill file descriptors properly in setFileMetadataFromFS ...................................................................... Patch Set 5: (9 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3150 PS4, Line 3150: load(); > Ah, it looks we changed this recently in IMPALA-9778. In that case, I think Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3152 PS4, Line 3152: // Let's retrieve the original partition instance from builder because this is > I think it would be more readable if you can you add a comment here which s Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1005 PS4, Line 1005: partitionKeyValues_ = partition.partitionKeyValues_; > Does this need to copy the encoded insert and delete FileDescriptors as wel Yes, thanks. This would probably cause a bug after using ALTER TABLE statements (once we start supporting such statements for ACID tables). http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@115 PS4, Line 115: HdfsPartition. > nit, please change this to HdfsPartition.Builders Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@144 PS4, Line 144: load > may be rename this to loadInternal and loadAndSet to load() Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java File fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java: http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java@563 PS4, Line 563: + " where id % 2 = 0"); : catalog_.reset(); : Table tbl = catalog_.getOrLoadTable(testDbName, testAcidTblName, "test", null); : Assert.assertFalse("Table mus > Its a little weird that we execute hiveSql in the try catch block for Impal Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java@572 PS4, Line 572: + " partition(year=2010,month=10) compact ' > not used Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java@576 PS4, Line 576: .db(testDbName) : .tbl(testAcidTblName) > this insert is not needed Done http://gerrit.cloudera.org:8080/#/c/16206/4/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoWriteIdTest.java@590 PS4, Line 590: Assert.assertEquals(0, part.delete_file_descriptors.size()); > I think it would be useful to add the following asserts here to make sure t Done -- To view, visit http://gerrit.cloudera.org:8080/16206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2927171cf426597c86766fb83d565c5e57025c73 Gerrit-Change-Number: 16206 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 20 Jul 2020 08:07:56 +0000 Gerrit-HasComments: Yes
