You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2020/10/09 17:23:51 UTC
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16575
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling.
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
A fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
8 files changed, 302 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/1
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7435/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 08:55:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7427/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 09:51:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 6: -Code-Review
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/6/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/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@35
PS6, Line 35: hiveexec.com.google.common.base.Preconditions
Should be com.google.common.base.Preconditions?
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 14 Oct 2020 02:01:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6577/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 15 Oct 2020 16:08:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7454/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 14 Oct 2020 16:39:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, wangsheng, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16575
to look at the new patch set (#7).
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs. Until
this patch we let HMS drop the tables and delete the directories. It
worked fine with the filesystem-based catalogs, but might not work well
with other Iceberg catalogs like HiveCatalog.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M tests/query_test/test_iceberg.py
13 files changed, 382 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/7
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, wangsheng, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16575
to look at the new patch set (#6).
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs. Until
this patch we let HMS drop the tables and delete the directories. It
worked fine with the filesystem-based catalogs, but might not work well
with other Iceberg catalogs like HiveCatalog.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M tests/query_test/test_iceberg.py
13 files changed, 383 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/6
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2654
PS1, Line 2654: TableIdentifier identifier = IcebergUtil.getIcebergTableIdentifier(newTable);
line too long (95 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Fri, 09 Oct 2020 17:24:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7428/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 13:35:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 2:
(7 comments)
Thanks for the comments!
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2654
PS1, Line 2654: // We use catalog location and table identifier as location
> line too long (95 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@83
PS1, Line 83: ebergTable.isSynchronizedTable(fe
> We may rename this function, since 'needsCreateInIceberg' is not fitable in
Renamed it to 'isSynchronizedTable' to be consistent with Kudu. Also moved this check to a precondition.
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@90
PS1, Line 90:
> nit: reduce tow spaces
Done
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@99
PS1, Line 99:
> HadoopTablesCatalog could be a singleton.
Done
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@136
PS1, Line 136:
> Do we need to separate HadoopTablesCatalog and HadoopCatalog
Yeah, I just wanted to keep things close to each other. But probably it's better practice to move the implementations to separate files.
I've created a new 'iceberg' directory/package under the 'catalog' directory and created the new files in that directory.
I've also got rid of IcebergCatalogs and moved the utility functions to IcebergUtil.
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@190
PS1, Line 190:
> Maybe 'catalogLocation' or 'warehouseLocation' is better.
Done
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@202
PS1, Line 202:
> Maybe add a comment to explain that Iceberg use a empty location when creat
Done
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 09:35:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6535/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Fri, 09 Oct 2020 17:24:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, wangsheng, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16575
to look at the new patch set (#4).
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs. Until
this patch we let HMS drop the tables and delete the directories. It
worked fine with the filesystem-based catalogs, but might not work well
with other Iceberg catalogs like HiveCatalog.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
12 files changed, 371 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/4
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1907
PS2, Line 1907: IcebergCatalogOpExecutor.dropTable((IcebergTable)existingTbl, params.if_exists);
Dropping synchronised tables isn't tightly related to the Iceberg catalog refactor in my opinion. I'd rather consider it a bug fix. Have you considered moving it to another patch?
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 12:29:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7442/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 16:19:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@99
PS1, Line 99: new HadoopTablesCatalog();
HadoopTablesCatalog could be a singleton.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Fri, 09 Oct 2020 17:41:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 4:
(1 comment)
Hi Zoltan, I found a bug when develop IMPALA-10166 based on this patch. Here is the steps:
1. create HadoopCatalog Iceberg table by Impala;
2. Drop table location on hdfs, which is '$catalog_loc/db/tbl'
3. Drop table by impala, this step failed due to check failed.
http://gerrit.cloudera.org:8080/#/c/16575/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1906
PS4, Line 1906: Preconditions.checkState(existingTbl instanceof IcebergTable);
I found a bug, here is the reproduce steps:
1. CREATE TABLE default.iceberg_alter_test(
level STRING
)
STORED AS ICEBERG
TBLPROPERTIES('iceberg.catalog'='hadoop.catalog',
'iceberg.catalog_location'='/test-warehouse/iceberg_test/hadoop_catalog_test');
2. hadoop fs -rm -r /test-warehouse/iceberg_test/hadoop_catalog_test/default/iceberg_alter_test
3. drop table default.iceberg_alter_test;
failed due to 'existingTbl' is IncompleteTable
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 08:51:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs. Until
this patch we let HMS drop the tables and delete the directories. It
worked fine with the filesystem-based catalogs, but might not work well
with other Iceberg catalogs like HiveCatalog.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Reviewed-on: http://gerrit.cloudera.org:8080/16575
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M tests/query_test/test_iceberg.py
13 files changed, 382 insertions(+), 100 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 15 Oct 2020 21:28:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 4:
(1 comment)
Thanks for the suggestion, WangSheng. I renamed my classes.
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1907
PS2, Line 1907: IcebergCatalogOpExecutor.dropTable((IcebergTable)existingTbl, params.if_exists);
> No strong feelings from my side just wanted to check that this is intention
I've had a short sentence about it in the commit message. Extended that paragraph with some explanation.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 08:39:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 14 Oct 2020 02:01:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 3:
Hi Zoltan, I suddenly found that if we supported HiveCatalog in the future, creating a 'HiveCatalog' class in Impala, this maybe confusing, other people maybe think this class related to HMS. So maybe we should use other name, such as 'IcebergHiveCatalog'/'IcebergHadoopCatalog'/'IcebergHadoopTables'. How do you think?
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 13:57:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@83
PS1, Line 83: IcebergTable.needsCreateInIceberg
We may rename this function, since 'needsCreateInIceberg' is not fitable in this situation.
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@136
PS1, Line 136: HadoopTablesCatalog
Do we need to separate HadoopTablesCatalog and HadoopCatalog
to new files, such as HadoopTablesCatalog.java and HadoopCatalog.java? When supporting HiveCatalog in the near future, we just need to add a new class file, and modify few code in IcebergCatalogs. This kind of design maybe better.
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@190
PS1, Line 190: location
Maybe 'catalogLocation' or 'warehouseLocation' is better.
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogs.java@202
PS1, Line 202: null
Maybe add a comment to explain that Iceberg use a empty location when create table by HadoopCatalog.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Sat, 10 Oct 2020 03:17:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 2:
(6 comments)
Thanks for the comments!
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java@29
PS2, Line 29: contiains
> contains
Done
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java@50
PS2, Line 50: Loads a native Iceberg table based on 'tableId' or 'tableLocation'.
> I'm not sure if it is better to add: "'tableId' is for HadoopCatalog, 'tab
Done
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1907
PS2, Line 1907: IcebergCatalogOpExecutor.dropTable((IcebergTable)existingTbl, params.if_exists);
> Dropping synchronised tables isn't tightly related to the Iceberg catalog r
For the current filesystem-based catalogs (HadoopTables and HadoopCatalog) the current behavior, i.e. let HMS delete the table directories work fine. But it won't work well with HiveCatalog I think.
So I've added dropTable() functionality to the IcebergCatalog interface in this patch, and it seemed reasonable to also do this code change. With it, adding more catalogs should be really simple.
But I can split this change to two if you feel strong about it.
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@80
PS2, Line 80: IcebergTable.isSynchronizedTable(feTable.getMetaStoreTable())
> We have already checked this condition in CatalogOpExecutor.java:1905, why
This precondition ensures that this function cannot be called under the wrong circumstances.
You're right, currently we also have this check in CatalogOpExecutor, but maybe that code will change in the future, or we'll have other places in the codebase that'll call this function.
So it's basically a future-proof check.
http://gerrit.cloudera.org:8080/#/c/16575/2/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/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@51
PS2, Line 51: import org.apache.impala.common.FileSystemUtil;
> It seems that this import is unused
Done
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@86
PS2, Line 86: loadTable
> I see that these two 'loadTable' method only called in IcebergUtil class, m
I will need this function from IcebergCatalogOpExecutor in my INSERT INTO patch (it currently uses IcebergUtil.getBaseTable()).
I'm making the other one private, though probably these stateless utility functions don't cause much harm if they are public as they only invoke public methods.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 13:14:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 6: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16575/6/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/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@90
PS6, Line 90: // TODO: un-comment the following line after Iceberg update.
: // return hadoopTables.dropTable(feTable.getLocation());
What is the story behind this? Can you add more info here or in the commit message? If we are expecting a specific change then it would be also nice to refer a Jira or some link here.
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java:
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@95
PS6, Line 95: dataFileList = IcebergUtil.getIcebergDataFiles(icebergTable_,
nit: fits in one line
http://gerrit.cloudera.org:8080/#/c/16575/6/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/16575/6/tests/query_test/test_iceberg.py@44
PS6, Line 44:
Can you add a comment to explain what we are testing here?
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 18:11:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, wangsheng, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16575
to look at the new patch set (#5).
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs. Until
this patch we let HMS drop the tables and delete the directories. It
worked fine with the filesystem-based catalogs, but might not work well
with other Iceberg catalogs like HiveCatalog.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M tests/query_test/test_iceberg.py
13 files changed, 384 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/5
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 7:
(7 comments)
Thanks for the comments!
http://gerrit.cloudera.org:8080/#/c/16575/6/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/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java@41
PS6, Line 41: cebergHadoopCatalog(String catalogLocat
> Better to import this class, and use 'HadoopCatalog' directly, just like 'H
Done
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java@44
PS6, Line 44:
> Same as below.
Done
http://gerrit.cloudera.org:8080/#/c/16575/6/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/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@29
PS6, Line 29: import org.apache.impala.common.FileSystemUtil;
> Unnecessary import.
Done
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@31
PS6, Line 31:
> Unnecessary import.
Done
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@33
PS6, Line 33:
> Unnecessary import.
Done
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@35
PS6, Line 35: ementation of IcebergCatalog for tables store
> Should be com.google.common.base.Preconditions?
Done
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java:
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@95
PS6, Line 95: dataFileList = IcebergUtil.getIcebergDataFiles(icebergTable_, icebergPredicates_);
> nit: fits in one line
Done
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 14 Oct 2020 16:20:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 4:
(1 comment)
Thanks for catching this. Fortunately, if anything had happened between step 1 and 2, e.g. SHOW FILES, SELECT, etc., then the DROP TABLE would have succeeded.
But still, we should be able to DROP incomplete tables in all cases.
http://gerrit.cloudera.org:8080/#/c/16575/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1906
PS4, Line 1906: Preconditions.checkState(existingTbl instanceof IcebergTable);
> I found a bug, here is the reproduce steps:
Thanks for the notice, I've fixed it and also added a test for it.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 16:30:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, wangsheng, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16575
to look at the new patch set (#2).
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/HadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/HadoopTablesCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
12 files changed, 366 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/2
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 7: Code-Review+2
Thanks for the explanations!
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 14 Oct 2020 20:47:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Gabor Kaszab, wangsheng, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16575
to look at the new patch set (#3).
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
IMPALA-10152: part 1: refactor Iceberg catalog handling
This patch refactors the code a bit to make it easier in the future
to add support for new Iceberg catalogs. We plan to add support for
HiveCatalog in the near future.
Iceberg has two main interfaces to manage tables: Tables and Catalog.
I created a new interface in Impala called 'IcebergCatalog' that
abstracts both Tables and Catalog. Currently there are two
implementations for IcebergCatalog:
* HadoopTablesCatalog for HadoopTables
* HadoopCatalog for HadoopCatalog
This patch also delegates dropTable() to the Iceberg catalogs.
Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/HadoopCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/HadoopTablesCatalog.java
A fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
12 files changed, 371 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/16575/3
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7441/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 16:20:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 2: Code-Review+1
(5 comments)
Thanks for these changes, LGTM. Only few small nits
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java@29
PS2, Line 29: contiains
contains
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalog.java@50
PS2, Line 50: Loads a native Iceberg table based on 'tableId' or 'tableLocation'.
I'm not sure if it is better to add: "'tableId' is for HadoopCatalog, 'tableLocation' is for HadoopTables"
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@80
PS2, Line 80: IcebergTable.isSynchronizedTable(feTable.getMetaStoreTable())
We have already checked this condition in CatalogOpExecutor.java:1905, why do we use Preconditions.checkState to check again? For some where else to call this method?
http://gerrit.cloudera.org:8080/#/c/16575/2/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/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@51
PS2, Line 51: import org.apache.impala.common.FileSystemUtil;
It seems that this import is unused
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@86
PS2, Line 86: loadTable
I see that these two 'loadTable' method only called in IcebergUtil class, maybe we should use 'private' instead.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 12 Oct 2020 12:06:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/5/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/16575/5/tests/query_test/test_iceberg.py@21
PS5, Line 21: from tests.util.shell_util import exec_process
flake8: F401 'tests.util.shell_util.exec_process' imported but unused
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 15:59:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1907
PS2, Line 1907: IcebergCatalogOpExecutor.dropTable((IcebergTable)existingTbl, params.if_exists);
> For the current filesystem-based catalogs (HadoopTables and HadoopCatalog)
No strong feelings from my side just wanted to check that this is intentional. What do you think about adding a small comment about this in the commit msg?
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 13 Oct 2020 07:54:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 15 Oct 2020 16:08:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/16575/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@90
PS1, Line 90:
nit: reduce tow spaces
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Sat, 10 Oct 2020 06:35:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6535/
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Fri, 09 Oct 2020 22:53:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling.
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7399/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Fri, 09 Oct 2020 17:44:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10152: part 1: refactor Iceberg catalog handling
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16575 )
Change subject: IMPALA-10152: part 1: refactor Iceberg catalog handling
......................................................................
Patch Set 6: Code-Review+1
(5 comments)
Thanks for the quick turnaround, LGTM, just few small nits.
http://gerrit.cloudera.org:8080/#/c/16575/6/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/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java@41
PS6, Line 41: org.apache.iceberg.hadoop.HadoopCatalog
Better to import this class, and use 'HadoopCatalog' directly, just like 'HadoopTables' in IcebergHadoopTables.java
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopCatalog.java@44
PS6, Line 44: org.apache.iceberg.hadoop.HadoopCatalog
Same as below.
http://gerrit.cloudera.org:8080/#/c/16575/6/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/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@29
PS6, Line 29: import org.apache.impala.catalog.iceberg.IcebergCatalog;
Unnecessary import.
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@31
PS6, Line 31: import org.apache.impala.common.ImpalaRuntimeException;
Unnecessary import.
http://gerrit.cloudera.org:8080/#/c/16575/6/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergHadoopTables.java@33
PS6, Line 33: import org.apache.impala.util.IcebergUtil;
Unnecessary import.
--
To view, visit http://gerrit.cloudera.org:8080/16575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie69dff6cd6b8b3dc0ba5f7671b8504a936032a85
Gerrit-Change-Number: 16575
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 14 Oct 2020 01:56:43 +0000
Gerrit-HasComments: Yes