Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16545 )
Change subject: IMPALA-10215: Implement INSERT INTO for non-partitioned Iceberg tables (Parquet) ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/16545/3/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/16545/3/be/src/runtime/coordinator.cc@786 PS3, Line 786: is_hive_acid) { > is_transactional became a bit misleading IMO, as Iceberg is also kind of tr I agree, renamed it to is_hive_acid. http://gerrit.cloudera.org:8080/#/c/16545/3/be/src/runtime/coordinator.cc@800 PS3, Line 800: } else { > Won't we incorrectly call this in case of Iceberg tables? Done http://gerrit.cloudera.org:8080/#/c/16545/3/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/16545/3/be/src/service/client-request-state.cc@1213 PS3, Line 1213: if (per_part_map.size() != 1) return ret; > If this is always the case than wouldn't be a DCHECK better? We call this function for every table kind, not just for Iceberg tables. What we know is that if the table has multiple partitions, then it's definitely not an Iceberg table. http://gerrit.cloudera.org:8080/#/c/16545/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java: http://gerrit.cloudera.org:8080/#/c/16545/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java@96 PS3, Line 96: String.format("Failed to load Iceberg table with id: %s", tableId)); > nit: +2 indentation Done http://gerrit.cloudera.org:8080/#/c/16545/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java: http://gerrit.cloudera.org:8080/#/c/16545/3/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@101 PS3, Line 101: String.format("Failed to load Iceberg table at location: %s", tableLocation)); > nit: +2 indentation Done http://gerrit.cloudera.org:8080/#/c/16545/3/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/16545/3/tests/query_test/test_iceberg.py@21 PS3, Line 21: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/16545/3/tests/query_test/test_iceberg.py@50 PS3, Line 50: > flake8: W292 no newline at end of file Done http://gerrit.cloudera.org:8080/#/c/16545/3/tests/stress/test_insert_stress.py File tests/stress/test_insert_stress.py: http://gerrit.cloudera.org:8080/#/c/16545/3/tests/stress/test_insert_stress.py@50 PS3, Line 50: while insert_cnt < num_ > Isn't it a problem that this affect ACID tests too? There could be a parame In this file there are non-ACID inserts, but yeah, it will affect them. It would mean the test become a bit more flaky, but also faster. Anyway, I switched to use a parameter. http://gerrit.cloudera.org:8080/#/c/16545/3/tests/stress/test_insert_stress.py@114 PS3, Line 114: self.client.execute("""create table {0} (wid int, i int) > Is this really needed? We should never use the same unique_database twice. Done http://gerrit.cloudera.org:8080/#/c/16545/3/tests/stress/test_insert_stress.py@124 PS3, Line 124: writers = [Task(self._impala_role_concurrent_writer, tbl_name, i, inserts, counter) > Yeah, it's good to see that the infrastructure for ACID inserts can be reus Yeah, it was a nice surprise how easy it was :) -- To view, visit http://gerrit.cloudera.org:8080/16545 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5690fb6c2cc51f0033fa26caf8597c80a11bcd8e Gerrit-Change-Number: 16545 Gerrit-PatchSet: 4 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: Tue, 20 Oct 2020 14:39:41 +0000 Gerrit-HasComments: Yes
