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 10: (9 comments) Thanks for making the change! I'm still looking into the join related logic and tests. 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: getFilesSample Could you add a TODO in this method for IMPALA-9883? 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: getFileDescriptors().size() Please change this to getNumFileDescriptors() after we revise its implementation. 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.orc.FileMetadata; nit: unused import 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 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: !getFileDescriptors().isEmpty() nit: It's inefficient to construct the list for this. Maybe change to return !fileDescriptors_.isEmpty() || !insertFileDescriptors_.isEmpty() || !deleteFileDescriptors_.isEmpty(). http://gerrit.cloudera.org:8080/#/c/16082/10/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@138 PS10, Line 138: getFileDescriptors().size() nit: It's inefficient to construct the list for this. Maybe change to return fileDescriptors_.isEmpty()? insertFileDescriptors_.size() + deleteFileDescriptors_.size() : fileDescriptors_.size(); 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; > Yeah, I use my IDE to do that for me. Interestingly, even if I delete this Looks like IntelliJ is more intelligent in this :D 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())); > I implemented it in PS10, but I'm afraid that the change became a bit too i PS10 looks good to me. I think the insert files and delete files are used separately so it makes sense to store them in different fileds. However, It'd be good to ask more feedbacks on this decision. Maybe we can merge fileDescriptors_ and insertFileDescriptors_ to one field (since only one of them is valid at a time) to reduce the complexity. 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: def test_full_acid_support(self): Could you add a test here for reading functional_orc_def.alltypes_deleted_rows in LocalCatalog mode? -- 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: 10 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 09:59:38 +0000 Gerrit-HasComments: Yes
