Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19776 )
Change subject: IMPALA-11877: (part 1) Add support for DELETE statements for UNPARTITIONED Iceberg tables ...................................................................... Patch Set 11: (10 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java File fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java: http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@467 PS10, Line 467: Set<SlotId> uniqueSlots, Set<SlotId> keySlots, Map<String, Integer> colIndexMap) : throws AnalysisException { : String[] deleteCols; : Preconditions.checkState(!icePosDelTable_.isPartitioned()); : deleteCols = new String[] {"INPUT__FILE__NAME", "FILE__POSITION"}; : // Add the key columns as slot refs : for (String k : deleteCols) { > For me it seems that the body of the for loop is identical between the Iceb Done http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@340 PS10, Line 340: get > You can use the getOrDefault(..) method on properties() to simplify this fu Done http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@349 PS10, Line 349: return IcebergUtil.getIcebergFileFormat(deleteFormat); > You can use getOrDefault() here as well, where the default value is getWrit getWriteFileFormat() returns TIcebergFileFormat, so we cannot use it in getOrDefault() which expects the two argument with the same type. http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java File fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java: http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java@28 PS10, Line 28: import org.apache.impala.thrift.TExplainLevel; > Logger and LoggerFactory aren't used. Done http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java@61 PS10, Line 61: PlanNode inputNode = fragment_.getPlanRoot(); > These variables can be final, if I'm not mistaken. Done http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/service/Frontend.java@2548 PS10, Line 2548: private static void addFinalizationParamsForDelete( > nit: I think the indentation is off in this function. Done http://gerrit.cloudera.org:8080/#/c/19776/10/fe/src/main/java/org/apache/impala/service/Frontend.java@2555 PS10, Line 2555: queryCtx, targetTable, false); : TIcebergDmlFinalizeParams iceFinalizeParams = new TIcebergDmlFinalizeParams(); : iceFinalizeParams.operation = TIcebergOperation.DELETE; : FeIcebergTable iceTable = (FeIcebergTable)targetTable; : iceFinalizeParams.setSpec_id(iceTable.getDefaultPartitionSpecId()); : iceFinalizeParams.setInitial_snapshot_id(iceTable.snapshotId()); : finalizeParams.setIceberg_params(iceFinalizeParams); > This part is kind of the same as in addFinalizatonParamsForInsert(). Would Done http://gerrit.cloudera.org:8080/#/c/19776/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-complex.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-complex.test: http://gerrit.cloudera.org:8080/#/c/19776/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-complex.test@3 PS10, Line 3: SELECT * FROM ice_complex_delete; > I think this comment is not relevant here. Done http://gerrit.cloudera.org:8080/#/c/19776/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-complex.test@90 PS10, Line 90: ---- QUERY > Could you give it a try to include an UNNEST() fn call into the select list I added UNNEST to int_array and int_array_array. I tried to add UNNEST to nested_struct.c.d, but apparently it is not supported by Impala. http://gerrit.cloudera.org:8080/#/c/19776/10/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/19776/10/tests/query_test/test_iceberg.py@1163 PS10, Line 1163: hive_output = self.run_stmt_in_hive("SELECT * FROM {} ORDER BY i".format(ice_delete)) > You run this 2 SELECTs twice. I think this line can be dropped. Done -- To view, visit http://gerrit.cloudera.org:8080/19776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic933b2295abe54b46d2a736961219988ff42915b Gerrit-Change-Number: 19776 Gerrit-PatchSet: 11 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 07 Jun 2023 17:24:13 +0000 Gerrit-HasComments: Yes
