Quanlong Huang 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 9: (3 comments) 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.Path.PathType; nit: keep the import list sorted in groups (usually the IDE will do this for you). http://gerrit.cloudera.org:8080/#/c/16082/9/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1418 PS9, Line 1418: if (addAcidSlotsIfNeeded(analyzer, hdfsTblRef, partitions)) { nit: what about merging this if-statement with its outer scope so they are if (isPartitionKeyScan && queryOpts.optimize_partition_key_scans) { ... } else if (addAcidSlotsIfNeeded(analyzer, hdfsTblRef, partitions)) { ... } else { ... } http://gerrit.cloudera.org:8080/#/c/16082/9/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1518 PS9, Line 1518: // Let's separate insert delta File Descriptors from delete delta FDs. I think we should separate the file descriptors in catalogd after loading them from HDFS instead of doing it here for each query. We can introduce two fileds in HdfsPartition: encodedInsertDeltaFileDescriptors_ and encodedDeleteDeltaFileDescriptors_ (and related fields in THdfsPartition and TPartialPartitionInfo). If a partition contains delete deltas, we separate them by setting these two fields and leaving encodedFileDescriptors_ null. We can also introduce two methods for FeFsPartition: genInsertDeltaPartition() and genDeleteDeltaPartition() using HdfsPartition.Builder in this way: public HdfsPartition genInsertDeltaPartition() { return new HdfsPartition.Builder(this) .setFileDescriptors(InsertDeltaFileDescriptors_) .build(); } With this we don't need to remove the "final" marker of encodedFileDescriptors_ and add back the setFileDescriptor() method, which violates our goal to make HdfsPartition immutable. The setFileDescriptor() method may encourage future developers to modify HdfsPartitions in-place in catalogd, which will break IMPALA-7533. -- 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: 9 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: Wed, 01 Jul 2020 06:57:52 +0000 Gerrit-HasComments: Yes
