Gabor Kaszab 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 9: (4 comments) I went through this patch mostly for my own education. Left some minor questions or comments, nothing serious. Great work Zoli! 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: abstract void substituteResultExprs(ExprSubstitutionMap smap, Analyzer analyzer); nit: could you add a comment for this function 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 strings into partition values (?) if I'm not mistaken. Just mentioning it here to see if any of those could be re-used. 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 on the table, but in fact the issue is that there is column masking on the table. This use case anyway made me think: wouldn't it make sense to allow the user to update these tables if they don't change the masked columns? I can understand that from implementation pow this could be difficult. 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: masked tables nit: I guess these are not masked tables, but tables with row filtering. -- 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: 9 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 07 Dec 2023 13:36:52 +0000 Gerrit-HasComments: Yes
