Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/18847 )
Change subject: IMPALA-11484: Create SCAN plan for Iceberg V2 position delete tables ...................................................................... Patch Set 4: (8 comments) I know it's easier said than done, but separating measurable amount of refactor and new feature helps the reviewing process. I wish gerrit had some feature to support it outside the codebase, e.g. creating a virtual diff of a.java:100-250 and b.java:250-400 to show the diff and mark it in another color in the original files. http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java@48 PS4, Line 48: * Iceberg position delete table created on the fly during planning. It belongs to an nit: "table is created" http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java@50 PS4, Line 50: if nit: "of" http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/catalog/VirtualTable.java File fe/src/main/java/org/apache/impala/catalog/VirtualTable.java: http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/catalog/VirtualTable.java@41 PS4, Line 41: taht nit: "that" http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@204 PS4, Line 204: .add(HdfsFileFormat.ICEBERG) Doesn't iceberg tables returns the underlying file format? https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java#L202 http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java: http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@125 PS4, Line 125: createIcebergScanNode nit: I think this name is a bit confusing, as we do have an IcebergScanNode class, but not necessarily returning that type. http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@197 PS4, Line 197: 1000000 magic number http://gerrit.cloudera.org:8080/#/c/18847/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@202 PS4, Line 202: JoinNode acidJoin = new HashJoinNode(dataScanNode, deleteScanNode, : /*straight_join=*/true, distributionMode, JoinOperator.LEFT_ANTI_JOIN, : positionJoinConjuncts, /*otherJoinConjuncts=*/Collections.emptyList()); : acidJoin.setId(ctx_.getNextNodeId()); : acidJoin.init(analyzer_); : acidJoin.setIsAcidJoin(); As I understand this is (similar but) unrelated to ACID, we should not let the names creep up. http://gerrit.cloudera.org:8080/#/c/18847/4/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/18847/4/tests/query_test/test_iceberg.py@765 PS4, Line 765: @SkipIfDockerizedCluster.internal_hostname This could be fixed with the custom operator in the future, if we add support for relative URI-s right? -- To view, visit http://gerrit.cloudera.org:8080/18847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I672cfee18d8e131772d90378d5b12ad4d0f7dd48 Gerrit-Change-Number: 18847 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Comment-Date: Wed, 24 Aug 2022 12:06:20 +0000 Gerrit-HasComments: Yes
