Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16082 )
Change subject: IMPALA-9859: Full ACID Milestone 4: Part 1 Reading modified tables (primitive types) ...................................................................... Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java File fe/src/main/java/org/apache/impala/catalog/FeFsTable.java: http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@376 PS10, Line 376: > Could you add a TODO in this method for IMPALA-9883? Done http://gerrit.cloudera.org:8080/#/c/16082/10/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/16082/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2010 PS10, Line 2010: getNumFileDescriptors(); > Please change this to getNumFileDescriptors() after we revise its implement Done http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@56 PS10, Line 56: import org.apache.thrift.TException; > nit: unused import Thanks for pointing these out! Done. http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@255 PS10, Line 255: } > nit: 4 spaces indent Done http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@409 PS10, Line 409: public ImmutableList<FileDescriptor> getFileDescriptors() { : if (insertFds_.isEmpty()) return fds_; : List<FileDescriptor> ret = Lists.newArrayListWithCapacity( : insertFds_.size() + deleteFds_.size()); : ret.addAll(insertFds_); : ret.addAll(deleteFds_); : return ImmutableList.copyOf(ret); : } : : @Override : public ImmutableList<FileDescriptor> getInsertFileDescriptors() { : return insertFds_; : } : > Need to update these Done http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java File fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java: http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@133 PS10, Line 133: !fileDescriptors_.isEmpty() || > nit: It's inefficient to construct the list for this. Maybe change to Done http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@138 PS10, Line 138: > nit: It's inefficient to construct the list for this. Maybe change to Instead of the ternary operator, I rather just sum up the sizes to keep it more readable. http://gerrit.cloudera.org:8080/#/c/16082/9/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/16082/9/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@45 PS9, Line 45: import org.apache.impala.analysis.NullLiteral; > Looks like IntelliJ is more intelligent in this :D Yeah :D It's a shame vscode fails on such a trivial task. But apart from this it's a great IDE. http://gerrit.cloudera.org:8080/#/c/16082/9/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1518 PS9, Line 1518: feTable.getMetaStoreTable().getParameters())); > PS10 looks good to me. I think the insert files and delete files are used s I'm OK with this if you think it's good. Yeah, I was also thinking about merging fileDescriptors_ and insertFileDescriptors_. My only concern is that it would be a bit weird for genDeleteDeltaPartition(), because it would put the delete delta descriptors into the regular descriptors. http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1536 PS10, Line 1536: hdfsTblRef.getDesc(), conjuncts, insertDeltaPartitions, hdfsTblRef, > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/test/java/org/apache/impala/util/AcidUtilsTest.java File fe/src/test/java/org/apache/impala/util/AcidUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/test/java/org/apache/impala/util/AcidUtilsTest.java@497 PS10, Line 497: () { > not *Fail* anymore Done http://gerrit.cloudera.org:8080/#/c/16082/10/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/16082/10/tests/custom_cluster/test_local_catalog.py@467 PS10, Line 467: @SkipIfHive2.acid > Could you add a test here for reading functional_orc_def.alltypes_deleted_r Copied the existing full acid scans test here. -- To view, visit http://gerrit.cloudera.org:8080/16082 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15c8feabf40be1658f3dd46883f5a1b2aa5d0659 Gerrit-Change-Number: 16082 Gerrit-PatchSet: 11 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 07 Jul 2020 15:03:05 +0000 Gerrit-HasComments: Yes
