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 <borokna...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Jul 2020 06:57:52 +0000
Gerrit-HasComments: Yes

Reply via email to