You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2021/07/13 00:07:00 UTC
[Impala-ASF-CR] IMPALA-10626: Add support for Iceberg's Catalogs API
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17466 )
Change subject: IMPALA-10626: Add support for Iceberg's Catalogs API
......................................................................
Patch Set 5: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17466/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:
http://gerrit.cloudera.org:8080/#/c/17466/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@680
PS5, Line 680: putGeneratedKuduProperty
This name could be changed as we are using this function for non Kudu stuff too.
http://gerrit.cloudera.org:8080/#/c/17466/5/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalogs.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalogs.java:
http://gerrit.cloudera.org:8080/#/c/17466/5/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalogs.java@72
PS5, Line 72: equals
Maybe it would be safer to use case insensitive comparison.
http://gerrit.cloudera.org:8080/#/c/17466/5/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCatalogs.java@163
PS5, Line 163: JNDI uses the context class loader
It would be nice to create an Iceberg issue for this, as other users of the API may also hit this. My understanding is that libraries should not rely on contextClassLoader and add a ClassLoader argument to the public functions that need one.
--
To view, visit http://gerrit.cloudera.org:8080/17466
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5dfa150986117fc55b28034c4eda38a736460ead
Gerrit-Change-Number: 17466
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@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 Jul 2021 00:07:00 +0000
Gerrit-HasComments: Yes