Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16825 )
Change subject: IMPALA-10380: INSERT INTO Iceberg tables with 'IDENTITY' partitions only ...................................................................... Patch Set 12: (7 comments) http://gerrit.cloudera.org:8080/#/c/16825/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16825/11//COMMIT_MSG@35 PS11, Line 35: TODO: : * Current change includes some parts of IMPALA-10384 which needs to : be removed once https://gerrit.cloudera.org/#/c/16850/ is merged https://gerrit.cloudera.org/#/c/16850/ is already merged http://gerrit.cloudera.org:8080/#/c/16825/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16825/12//COMMIT_MSG@12 PS12, Line 12: Identity-partitioned Iceberg tables are similar to regular : partitioned tables Can you mention the difference in INSERT syntax? I mean that there is no PARTITION(x=y ... ) in the statement, instead select list -> partition mapping occurs bases on column order if I understand correctly. http://gerrit.cloudera.org:8080/#/c/16825/11/be/src/exec/hdfs-table-sink.cc File be/src/exec/hdfs-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/16825/11/be/src/exec/hdfs-table-sink.cc@157 PS11, Line 157: // For Iceberg tables we only have a single partition descriptor. : if (IsIceberg()) break; This is the same as skipping the whole for loop, right? I would prefer to put the for loop in an if block, or if we want to avoid additional nesting, the extract the loop to a separate function. http://gerrit.cloudera.org:8080/#/c/16825/11/be/src/exec/hdfs-table-sink.cc@579 PS11, Line 579: it != partition_descriptor_map_.end() It it possible for this to succeed if IsIceberg() is true? If not, then I think it is clearer to handle the IsIceberg() case first. http://gerrit.cloudera.org:8080/#/c/16825/11/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/16825/11/be/src/runtime/descriptors.cc@250 PS11, Line 250: TIcebergPartitionSpec Maybe const ref would be better to avoid copy. http://gerrit.cloudera.org:8080/#/c/16825/11/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert.test: http://gerrit.cloudera.org:8080/#/c/16825/11/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert.test@4 PS11, Line 4: partitioend typo http://gerrit.cloudera.org:8080/#/c/16825/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert.test: http://gerrit.cloudera.org:8080/#/c/16825/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert.test@128 PS12, Line 128: select id, bool_col, int_col, bigint_col, float_col, double_col, Can you add test here or in iceberg-negative.test where the insert is rejected because the types in the select list do not match the types in the column? E.g the columns here could be reordered. -- To view, visit http://gerrit.cloudera.org:8080/16825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If98797a2bfdc038d0467c8f83aadf1a12e1d69d4 Gerrit-Change-Number: 16825 Gerrit-PatchSet: 12 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: wangsheng <sky...@163.com> Gerrit-Comment-Date: Wed, 16 Dec 2020 15:26:32 +0000 Gerrit-HasComments: Yes
