Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20677 )
Change subject: IMPALA-12313: (part 2) Limited UPDATE support for Iceberg tables ...................................................................... Patch Set 10: (15 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/exec/iceberg-delete-sink.h File be/src/exec/iceberg-delete-sink.h: http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/exec/iceberg-delete-sink.h@81 PS9, Line 81: /// Verifies that the row batch does not contain duplicated rows. > Could you add an explanation of intent, it is not clear for me why this is Added explanation. http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/exec/iceberg-delete-sink.h@82 PS9, Line 82: the context of > Nit: For readability could you rename to something like: VerifyRowsNotDupli Done http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/exec/multi-table-sink.cc File be/src/exec/multi-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/exec/multi-table-sink.cc@52 PS9, Line 52: tsink_bas > nit: tsink_base Done http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/runtime/dml-exec-state.h File be/src/runtime/dml-exec-state.h: http://gerrit.cloudera.org:8080/#/c/20677/9/be/src/runtime/dml-exec-state.h@72 PS9, Line 72: void UpdatePartition(const std::string& partition_name, int64_t num_rows_delta, : const DmlStatsPB* insert_stats, bool is_delete = false); : > nit: should fit into 2 lines Done http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java@120 PS9, Line 120: id > nit: ++nextTableId_ and return nextTableId_ We want to use the original value (before the increment) in the put() and return stmt. http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java@87 PS9, Line 87: public List<Expr> getDeletePartitionExprs(Analyzer analyzer) throws AnalysisException { : if (!originalTargetTable_.is > nit: should fit into 1 line Done http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/IcebergModifyImpl.java@94 PS9, Line 94: return getSlotRefs(analyzer, Lists.newArrayList( : "INPUT__FILE__NAME", "FI > nit: should fit into 1 line Done http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@54 PS9, Line 54: /** > nit: could you add a comment for this function Sure, added comment. http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java File fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@63 PS9, Line 63: // Currently Kudu and Iceberg tables are sup > Not anymore :) Done :) http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/planner/MultiDataSink.java File fe/src/main/java/org/apache/impala/planner/MultiDataSink.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/planner/MultiDataSink.java@137 PS9, Line 137: > This comment is a bit hard to understand. It seems to me that the two parts Sorry about the confusion. This was copy-pasted from HdfsTableSink, and unfortunately not even being used by the planner. Filed IMPALA-12587 to fix that, until that I think it's best to remove these unused methods to avoid further confusion. http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/planner/MultiDataSink.java@138 PS9, Line 138: > nit: on Done http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/planner/Planner.java@185 PS9, Line 185: root > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/20677/9/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1214 PS9, Line 1214: private static Expr getIcebergPartitionTransformExpr(IcebergPartitionField partField, > Peter's DROP PARTITION patch added some new functionality for converting st Thanks, AFAICT those functions implement a different functionality. Here we need partition Expr objects that can be used in the planner. http://gerrit.cloudera.org:8080/#/c/20677/9/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test File testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test: http://gerrit.cloudera.org:8080/#/c/20677/9/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test@753 PS9, Line 753: AuthorizationException: User '$USER' does not have privileges to access: functional_parquet.iceberg_v2_delete_positional > Isn't the error msg misleading. It says the user doesn't have permissions o We raise the exception from BaseAuthorizationChecker.java because we are requiring ALL privileges from the user. Ranger also have UPDATE privileges, so we could use that instead, and this way we could also improve the error message. Filed IMPALA-12613. About allowing UPDATE on masked tables: yeah, in some cases it would probably make sense to allow UPDATEs, but let's start with a strict policy, and later we can relax the conditions if needed. http://gerrit.cloudera.org:8080/#/c/20677/9/testdata/workloads/functional-query/queries/QueryTest/ranger_row_filtering.test File testdata/workloads/functional-query/queries/QueryTest/ranger_row_filtering.test: http://gerrit.cloudera.org:8080/#/c/20677/9/testdata/workloads/functional-query/queries/QueryTest/ranger_row_filtering.test@647 PS9, Line 647: uld be blocke > nit: I guess these are not masked tables, but tables with row filtering. Done -- To view, visit http://gerrit.cloudera.org:8080/20677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff0ef6075a2b6ebe130d15daa389ac1a505a7a08 Gerrit-Change-Number: 20677 Gerrit-PatchSet: 10 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 08 Dec 2023 16:42:34 +0000 Gerrit-HasComments: Yes
