Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13215 )
Change subject: IMPALA-8438: Store WriteId and ValidWriteId list for table and partition ...................................................................... Patch Set 8: (13 comments) Will submit patch 9 for the issues. http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@255 PS8, Line 255: return -1L; > agree with Sudanshu's earlier comments -- I think it's better to throw Unsu Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java File fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java@2 PS8, Line 2: // // or more contributor license agreements. See the NOTICE file > nit: double // here Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-2/java/org/apache/impala/compat/ValidWriteIdList.java@18 PS8, Line 18: > Doesn't this need to be in the same package as the Hive ValidWriteIdClass s Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@387 PS8, Line 387: public static String fetchValidWriteIdsString(IMetaStoreClient client, > why do we need this class if we already have the above one, and the shim cl Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@399 PS8, Line 399: if (validWriteIds != null) { > Why check against null here instead of Preconditions.checkNotNull()? Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@412 PS8, Line 412: return partition == null ? -1L : partition.getWriteId(); > Same -- we shouldn't be checking for null and papering over it. It should b Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@238 PS8, Line 238: System.lineSeparator > nit: we just use \n elsewhere Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@243 PS8, Line 243: validIdsBuf.append(" "); > why this extra whitespace? To indent. If not, the output looks ugly. http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java File fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java@172 PS8, Line 172: * @return the writeId stored in hms for the partition > Per comments elsewhere, I think we should either change these to Long (to a Add common. http://gerrit.cloudera.org:8080/#/c/13215/8/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/13215/8/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@618 PS8, Line 618: //-1 means writeId_ is irrelevant(not supported). > nit: space after // Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@1022 PS8, Line 1022: : if (thriftPartition.isSetWrite_id()) { : partition.writeId_ = thriftPartition.getWrite_id(); : } else { : partition.writeId_ = -1l; : } > Use a ternary? Done http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@924 PS8, Line 924: //TODO writeIDs may also be loaded in other code paths. > I'm still not quite sure I understand this TODO Will remove it. http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java File fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java: http://gerrit.cloudera.org:8080/#/c/13215/8/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java@235 PS8, Line 235: testLoadAcidTables("select * from acid.insert_only_no_partitions"); > We need to get this into the data loading setup before we can commit this c Will @Ignore the test -- To view, visit http://gerrit.cloudera.org:8080/13215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6edbd64424edf0ba88af110ab8b958a1966b8b54 Gerrit-Change-Number: 13215 Gerrit-PatchSet: 8 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sudhanshu Arora <sudhan...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 14 May 2019 03:45:29 +0000 Gerrit-HasComments: Yes
