You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/05/18 04:17:21 UTC

[GitHub] [incubator-doris] ConeyLiu opened a new pull request, #9658: Iceberg custom catalog

ConeyLiu opened a new pull request, #9658:
URL: https://github.com/apache/incubator-doris/pull/9658

   # Proposed changes
   
   Issue Number: close #9657
   
   ## Problem Summary:
   
   This PR refactors part of the Iceberg code to support more Iceberg Catalogs. And also it supports set more catalog specified properties.
   
   Describe the overview of changes.
   
   ## Checklist(Required)
   
   1. Does it affect the original behavior: (No)
   2. Has unit tests been added: (Yes)
   3. Has document been added or modified: (Yes)
   4. Does it need to update dependencies: (No)
   5. Are there any changes that cannot be rolled back: (No)
   
   ## Further comments
   
   If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] qidaye commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
qidaye commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r895050605


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,14 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`). Iceberg built-in supports two catalogs with a simple name: `HIVE` and `HADOOP`. You should use the full class name for other implementations.

Review Comment:
   How about add more built-in catalogs, such as JDBC, Glue catalog, for user convenience ?
   
   User only need set full class name when using a custom catalog.



##########
fe/fe-core/src/main/java/org/apache/doris/catalog/IcebergProperty.java:
##########
@@ -17,33 +17,73 @@
 
 package org.apache.doris.catalog;
 
+import com.google.common.base.Joiner;
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
+
+import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_HADOOP;
+import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_HIVE;

Review Comment:
   Do not import static member.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r884787351


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).

Review Comment:
   Updated



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r876780913


##########
fe/fe-core/src/main/java/org/apache/doris/external/iceberg/IcebergCatalogImpl.java:
##########
@@ -14,54 +14,48 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
-
 package org.apache.doris.external.iceberg;
 
+import com.google.common.collect.Maps;
 import org.apache.doris.catalog.IcebergProperty;
-
 import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogUtil;
 import org.apache.iceberg.Table;
+import org.apache.iceberg.catalog.Catalog;
 import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.SupportsNamespaces;
 import org.apache.iceberg.catalog.TableIdentifier;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-/**
- * HiveCatalog of Iceberg
- */
-public class HiveCatalog implements IcebergCatalog {
-    private static final Logger LOG = LogManager.getLogger(HiveCatalog.class);
+public class IcebergCatalogImpl implements IcebergCatalog {
+    private static final Logger LOG = LogManager.getLogger(IcebergCatalogImpl.class);
 
-    private org.apache.iceberg.hive.HiveCatalog hiveCatalog;
-
-    public HiveCatalog() {
-        hiveCatalog = new org.apache.iceberg.hive.HiveCatalog();
-    }
+    private String catalogType;
+    private Catalog icebergCatalog;
 
     @Override
     public void initialize(IcebergProperty icebergProperty) {
-        // set hadoop conf
+        this.catalogType = icebergProperty.getCatalogType();
+        Map<String, String> properties = Maps.newHashMap(icebergProperty.getExtraProperties());
+        properties.put(CatalogUtil.ICEBERG_CATALOG_TYPE, icebergProperty.getCatalogType());
+        // Hadoop configuration
         Configuration conf = new Configuration();
-        hiveCatalog.setConf(conf);
-        // initialize hive catalog
-        Map<String, String> catalogProperties = new HashMap<>();
-        catalogProperties.put("uri", icebergProperty.getHiveMetastoreUris());
-        hiveCatalog.initialize("hive", catalogProperties);
+        this.icebergCatalog = CatalogUtil.buildIcebergCatalog(icebergProperty.getCatalogType(), properties, conf);

Review Comment:
   We could use the `iceberg.catalog.catalog-impl` property to load other catalogs with the full catalog name.



##########
fe/fe-core/src/main/java/org/apache/doris/planner/IcebergScanNode.java:
##########
@@ -99,9 +98,6 @@ public String getNodeExplainString(String prefix, TExplainLevel detailLevel) {
         StringBuilder output = new StringBuilder();
         if (!isLoad()) {
             output.append(prefix).append("TABLE: ").append(icebergTable.getName()).append("\n");
-            output.append(prefix).append("PATH: ")

Review Comment:
   Added



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] qidaye commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
qidaye commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r875717698


##########
fe/fe-core/src/main/java/org/apache/doris/external/iceberg/IcebergCatalogMgr.java:
##########
@@ -50,83 +49,62 @@ public class IcebergCatalogMgr {
 
     // hive metastore uri -> iceberg catalog
     // used to cache iceberg catalogs
-    private static final ConcurrentHashMap<String, IcebergCatalog> metastoreUriToCatalog = new ConcurrentHashMap();
+    private static final ConcurrentHashMap<IcebergProperty, IcebergCatalog> propertiesToCatalog = new ConcurrentHashMap();

Review Comment:
   It's better to use `IcebergProperty.extraProperties`, such as `uri`/`warehouse`, as the cache key. 
   `IcebergProperty` is bound to the database and table, not a generic cache.



##########
fe/fe-core/src/main/java/org/apache/doris/planner/IcebergScanNode.java:
##########
@@ -99,9 +98,6 @@ public String getNodeExplainString(String prefix, TExplainLevel detailLevel) {
         StringBuilder output = new StringBuilder();
         if (!isLoad()) {
             output.append(prefix).append("TABLE: ").append(icebergTable.getName()).append("\n");
-            output.append(prefix).append("PATH: ")

Review Comment:
   Uri or warehouse information should be added here.



##########
fe/fe-core/src/main/java/org/apache/doris/external/iceberg/IcebergCatalogImpl.java:
##########
@@ -14,54 +14,48 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
-
 package org.apache.doris.external.iceberg;
 
+import com.google.common.collect.Maps;
 import org.apache.doris.catalog.IcebergProperty;
-
 import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogUtil;
 import org.apache.iceberg.Table;
+import org.apache.iceberg.catalog.Catalog;
 import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.SupportsNamespaces;
 import org.apache.iceberg.catalog.TableIdentifier;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-/**
- * HiveCatalog of Iceberg
- */
-public class HiveCatalog implements IcebergCatalog {
-    private static final Logger LOG = LogManager.getLogger(HiveCatalog.class);
+public class IcebergCatalogImpl implements IcebergCatalog {
+    private static final Logger LOG = LogManager.getLogger(IcebergCatalogImpl.class);
 
-    private org.apache.iceberg.hive.HiveCatalog hiveCatalog;
-
-    public HiveCatalog() {
-        hiveCatalog = new org.apache.iceberg.hive.HiveCatalog();
-    }
+    private String catalogType;
+    private Catalog icebergCatalog;
 
     @Override
     public void initialize(IcebergProperty icebergProperty) {
-        // set hadoop conf
+        this.catalogType = icebergProperty.getCatalogType();
+        Map<String, String> properties = Maps.newHashMap(icebergProperty.getExtraProperties());
+        properties.put(CatalogUtil.ICEBERG_CATALOG_TYPE, icebergProperty.getCatalogType());
+        // Hadoop configuration
         Configuration conf = new Configuration();
-        hiveCatalog.setConf(conf);
-        // initialize hive catalog
-        Map<String, String> catalogProperties = new HashMap<>();
-        catalogProperties.put("uri", icebergProperty.getHiveMetastoreUris());
-        hiveCatalog.initialize("hive", catalogProperties);
+        this.icebergCatalog = CatalogUtil.buildIcebergCatalog(icebergProperty.getCatalogType(), properties, conf);

Review Comment:
   **Note**: `CatalogUtil.buildIcebergCatalog` only supports `HiveCatalog` and `HadoopCatalog` for now.
   How do you plan to support other catalog types?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] qidaye commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
qidaye commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r880003376


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).
+    - `iceberg.catalog.catalog-impl`: you could also use the full catalog name with this property. You should not use both `iceberg.catalog.type` and `iceberg.catalog.catalog-impl`.

Review Comment:
   > Do you mean always set the full class name? Would it be inconvenient for users who use the `HIVE` or `Hadoop`?
   
   What I mean is that the catalog types already supported by iceberg can be set without setting the full class name, just set the type.
   For custom catalog types, only the full class name needs to be set



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r879433707


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).

Review Comment:
   OK, will update it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r879433250


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).
+    - `iceberg.catalog.catalog-impl`: you could also use the full catalog name with this property. You should not use both `iceberg.catalog.type` and `iceberg.catalog.catalog-impl`.

Review Comment:
   Do you mean always set the full class name?  Would it be inconvenient for users who use the `HIVE` or `Hadoop`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [doris] github-actions[bot] closed pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs
URL: https://github.com/apache/doris/pull/9658


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r876779129


##########
fe/fe-core/src/main/java/org/apache/doris/external/iceberg/IcebergCatalogMgr.java:
##########
@@ -50,83 +49,62 @@ public class IcebergCatalogMgr {
 
     // hive metastore uri -> iceberg catalog
     // used to cache iceberg catalogs
-    private static final ConcurrentHashMap<String, IcebergCatalog> metastoreUriToCatalog = new ConcurrentHashMap();
+    private static final ConcurrentHashMap<IcebergProperty, IcebergCatalog> propertiesToCatalog = new ConcurrentHashMap();

Review Comment:
   Updated



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r884805646


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).
+    - `iceberg.catalog.catalog-impl`: you could also use the full catalog name with this property. You should not use both `iceberg.catalog.type` and `iceberg.catalog.catalog-impl`.

Review Comment:
   Updated



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] ConeyLiu commented on pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#issuecomment-1131411701

   @qidaye, thanks for the review. Comments have been addressed. Please take another look when you are free. Thanks again.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [incubator-doris] qidaye commented on a diff in pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
qidaye commented on code in PR #9658:
URL: https://github.com/apache/incubator-doris/pull/9658#discussion_r879153705


##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).

Review Comment:
   `type` only supports `HIVE` and `HADOOP`.
   Other types of catalog should use `catalog-impl` config. We should make it clear for users.



##########
docs/en/ecosystem/external-table/iceberg-of-doris.md:
##########
@@ -135,10 +167,15 @@ You can also create an Iceberg table by explicitly specifying the column definit
     - The order of the columns needs to be consistent with the Iceberg table
 - ENGINE needs to be specified as ICEBERG
 - PROPERTIES property.
-    - `iceberg.hive.metastore.uris`: Hive Metastore service address
     - `iceberg.database`: the name of the database to which Iceberg is mounted
     - `iceberg.table`: the name of the table to which Iceberg is mounted, not required when mounting Iceberg database.
-    - `iceberg.catalog.type`: the catalog method used in Iceberg, the default is `HIVE_CATALOG`, currently only this method is supported, more Iceberg catalog access methods will be supported in the future.
+    - `iceberg.catalog.type`: the catalog type used in Iceberg, the default is `HIVE` (simple name of `org.apache.iceberg.hive.HiveCatalog`).
+    - `iceberg.catalog.catalog-impl`: you could also use the full catalog name with this property. You should not use both `iceberg.catalog.type` and `iceberg.catalog.catalog-impl`.

Review Comment:
   The user does not need to care about the specific implementation classes of the catalog already supported in iceberg, which would make it more difficult to use. Such as `JDBC`/`Glue` and so on.
   
   User only needs to set the catalog type and the specific implementation class Doris should encapsulate it.
   
   Wouldn't it be better if the user only needs to set the implementation class information when using a custom catalog?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org


[GitHub] [doris] github-actions[bot] commented on pull request #9658: Refactor part of Iceberg code to support more Iceberg Catalogs

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #9658:
URL: https://github.com/apache/doris/pull/9658#issuecomment-1343593748

   We're closing this PR because it hasn't been updated in a while.
   This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
   If you'd like to revive this PR, please reopen it and feel free a maintainer to remove the Stale tag!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org