Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16143 )
Change subject: IMPALA-9741: Support querying Iceberg table by impala ...................................................................... Patch Set 15: Code-Review+1 (8 comments) Thank you very much for your quick and great work! Only found a few nits, other than that LGTM! http://gerrit.cloudera.org:8080/#/c/16143/15/fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java File fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java: http://gerrit.cloudera.org:8080/#/c/16143/15/fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java@66 PS15, Line 66: iceberg table not supported truncate. nit: "we also cannot truncate Iceberg tables." http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/data/README@503 PS15, Line 503: Icebreg nit: Iceberg http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/data/README@507 PS15, Line 507: including table metadata nit: "which contains table metadata" http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/data/README@507 PS15, Line 507: managerd managed http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/data/README@508 PS15, Line 508: including data files nit: which contains the data files. http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test: http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test@19 PS15, Line 19: ImpalaRuntimeException: Error making 'createTable' RPC to Hive Metastore: : CAUSED BY: IllegalArgumentException: Cannot find source column: event_time Can we catch this error during analysis, and throw an AnalysisException instead of ImpalaRuntimeException? http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-profile.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-profile.test: http://gerrit.cloudera.org:8080/#/c/16143/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-profile.test@7 PS15, Line 7: row_regex: .* PARQUET/GZIP:4 : row_regex: .* PARQUET/GZIP:2 I'm just wondering how deterministic is it. Do the test consistently pass if you run it multiple times? The alternative is to use SUM(aggregation, RowsRead) http://gerrit.cloudera.org:8080/#/c/16143/13/testdata/workloads/functional-query/queries/QueryTest/iceberg_query.test File testdata/workloads/functional-query/queries/QueryTest/iceberg_query.test: http://gerrit.cloudera.org:8080/#/c/16143/13/testdata/workloads/functional-query/queries/QueryTest/iceberg_query.test@77 PS13, Line 77: > 1. I've already add non-count queries in iceberg-query.test The generated test files usually contain blank lines which needs to be removed. You can remove the blank lines in vim with the following: :g/^$/d Other than that it's up to you if you just copy over the original iceberg-query.test file, or just copy-paste some parts of it. But before committing the file don't forget to check that you only modified/added tests that you intended. -- To view, visit http://gerrit.cloudera.org:8080/16143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I856cfee4f3397d1a89cf17650e8d4fbfe1f2b006 Gerrit-Change-Number: 16143 Gerrit-PatchSet: 15 Gerrit-Owner: wangsheng <sky...@163.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: Mon, 20 Jul 2020 17:44:18 +0000 Gerrit-HasComments: Yes
