kasakrisz commented on code in PR #6449:
URL: https://github.com/apache/hive/pull/6449#discussion_r3273183833
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -183,6 +185,32 @@ public void
rollbackCreateTable(org.apache.hadoop.hive.metastore.api.Table hmsTa
@Override
public void commitCreateTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
+ if (BaseHiveIcebergMetaHook.isNativeIcebergLogicalView(hmsTable)) {
Review Comment:
nit.: this doesn't have to be qualified because isNativeIcebergLogicalView
is defined in the super class.
##########
iceberg/iceberg-catalog/src/test/java/org/apache/iceberg/hive/TestIcebergNativeLogicalViewSupport.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.hive.conf.Constants;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.FieldSchema;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.CatalogUtil;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.view.BaseView;
+import org.apache.iceberg.view.View;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE;
+import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE;
+import static org.assertj.core.api.Assertions.assertThat;
+
+class TestNativeIcebergViewSupport {
+
+ private static final String DB = "native_vw_db";
+ private static final String VIEW = "native_vw";
+
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ HiveMetastoreExtension.builder().withDatabase(DB).build();
+
+ @AfterEach
+ void dropView() {
+ HiveCatalog cat = verifyCatalog();
+ TableIdentifier id = TableIdentifier.of(DB, VIEW);
+ cat.dropView(id);
+ }
+
+ private HiveConf nativeViewConf() {
+ HiveConf conf = new HiveConf(HIVE_METASTORE_EXTENSION.hiveConf());
+ MetastoreConf.setVar(conf, MetastoreConf.ConfVars.CATALOG_DEFAULT, "hive");
+ conf.set(
+ IcebergCatalogProperties.catalogPropertyConfigKey("hive",
ICEBERG_CATALOG_TYPE),
+ ICEBERG_CATALOG_TYPE_HIVE);
+ return conf;
+ }
+
+ private HiveCatalog verifyCatalog() {
+ return (HiveCatalog)
+ CatalogUtil.loadCatalog(
+ HiveCatalog.class.getName(),
+ ICEBERG_CATALOG_TYPE_HIVE,
+ ImmutableMap.of(
+ CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
+ String.valueOf(TimeUnit.SECONDS.toMillis(10))),
+ HIVE_METASTORE_EXTENSION.hiveConf());
+ }
+
+ @Test
+ void testCreateCommitsNativeViewWithMarkerProperty() throws Exception {
+ HiveConf conf = nativeViewConf();
+ List<FieldSchema> cols =
+ Arrays.asList(new FieldSchema("id", "int", null), new
FieldSchema("name", "string", null));
+ String sql = String.format("select id, name from %s.src_tbl", DB);
+ Map<String, String> props = Collections.singletonMap("k1", "v1");
+
+ boolean created =
+ NativeIcebergViewSupport.createOrReplaceNativeView(
+ conf, DB, VIEW, cols, sql, props, "hello-view", false, false);
+ assertThat(created).isTrue();
+
+ HiveCatalog cat = verifyCatalog();
+ TableIdentifier id = TableIdentifier.of(DB, VIEW);
+ assertThat(cat.viewExists(id)).isTrue();
+ View view = cat.loadView(id);
+ assertThat(view.properties())
+ .containsEntry(
+ Constants.NATIVE_VIEW_STORAGE_HANDLER_CLASS_PARAM,
+ NativeIcebergViewSupport.NATIVE_ICEBERG_VIEW_HANDLER_FQCN)
+ .containsEntry("comment", "hello-view")
+ .containsEntry("k1", "v1");
+ HiveViewOperations ops = (HiveViewOperations) ((BaseView)
view).operations();
+ assertThat(ops.current().currentVersion().representations()).isNotEmpty();
+ }
+
+ @Test
+ void
testCreateOrReplaceNativeViewSkipsWhenViewExistsAndIfNotExistsFlagTrue() throws
Exception {
+ HiveConf conf = nativeViewConf();
+ List<FieldSchema> cols = Collections.singletonList(new FieldSchema("id",
"int", null));
+
+ assertThat(
+ NativeIcebergViewSupport.createOrReplaceNativeView(
+ conf, DB, VIEW, cols, "select 1 as id", null, null, false,
false))
+ .isTrue();
+ assertThat(
+ NativeIcebergViewSupport.createOrReplaceNativeView(
+ conf, DB, VIEW, cols, "select 2 as id", null, null, false,
true))
+ .isFalse();
Review Comment:
Could you assert that the definition query hasn't changed?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java:
##########
@@ -82,6 +84,10 @@ public int execute() throws HiveException {
if (desc.getProperties() != null) {
oldview.getTTable().getParameters().putAll(desc.getProperties());
}
+ if (!desc.usesStorageHandler()) {
+ // External logical view is replaced with a native Hive view
+ clearStorageHandlerProp(oldview);
+ }
Review Comment:
Could you please share some repro steps for this use-case?
##########
iceberg/iceberg-handler/src/test/queries/positive/iceberg_rest_catalog_gravitino.q:
##########
@@ -74,6 +72,54 @@ VALUES ('fn1','ln1', 1, 10), ('fn2','ln2', 2, 20),
('fn3','ln3', 3, 30);
describe formatted ice_orc2;
select * from ice_orc2;
+---------------------------------------------------------------------------------------------------------------------
+--! Iceberg Native View tests
+---------------------------------------------------------------------------------------------------------------------
+
+-- Enable once CBO supports Iceberg native views on partitioned tables
+set hive.cbo.enable=false;
+
+-----------------------------------------------------------------------------------------------
+--! Iceberg native view via TBLPROPERTIES ('view-format'='iceberg') on a REST
catalog table
+--! without a catalog name in table properties
+-----------------------------------------------------------------------------------------------
+
+create view ice_v1 tblproperties ('view-format'='iceberg') as select * from
ice_orc1;
+select * from ice_v1;
+desc formatted ice_v1;
+drop view ice_v1;
+
+-----------------------------------------------------------------------------------------------
+--! Iceberg native view via TBLPROPERTIES ('view-format'='iceberg') on a REST
catalog table
+--! with a catalog name in table properties
+-----------------------------------------------------------------------------------------------
+
+create view ice_v2 tblproperties ('view-format'='iceberg') as select * from
ice_orc2;
+select * from ice_v2;
+desc formatted ice_v2;
+drop view ice_v2;
+
+-----------------------------------------------------------------------------------------------
+--! Native Iceberg catalog view: STORED BY omitted,
'hive.default.storage.handler.class' set to
+--! HiveIcebergStorageHandler on a REST catalog table without catalog name in
table properties
+-----------------------------------------------------------------------------------------------
+
+set
hive.default.storage.handler.class=org.apache.iceberg.mr.hive.HiveIcebergStorageHandler;
+
+create view ice_v3 as select * from ice_orc1;
+select * from ice_v3;
+desc formatted ice_v3;
+drop view ice_v3;
+
+-----------------------------------------------------------------------------------------------
+--! Native Iceberg catalog view with default Iceberg storage handler (REST
table with catalog in props)
+-----------------------------------------------------------------------------------------------
+
+create view ice_v4 as select * from ice_orc2;
+select * from ice_v4;
+desc formatted ice_v4;
+drop view ice_v4;
Review Comment:
Thanks for the clarification. Is it mean that the view `ice_v4` is created
in the `ice01` catalog too ?
Does creating views in one catalog referencing base tables from another
catalog?
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -183,6 +185,32 @@ public void
rollbackCreateTable(org.apache.hadoop.hive.metastore.api.Table hmsTa
@Override
public void commitCreateTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
+ if (BaseHiveIcebergMetaHook.isNativeIcebergLogicalView(hmsTable)) {
+ tableProperties = IcebergTableProperties.getTableProperties(hmsTable,
conf);
+ if (Catalogs.hiveCatalog(conf, tableProperties)) {
+ boolean replace =
+ Boolean.parseBoolean(
+ SessionStateUtil.getProperty(conf,
Constants.EXTERNAL_LOGICAL_VIEW_DDL_REPLACE).orElse("false"));
+ boolean ifNotExists =
+ Boolean.parseBoolean(
+ SessionStateUtil.getProperty(conf,
Constants.EXTERNAL_LOGICAL_VIEW_CREATE_IF_NOT_EXISTS)
+ .orElse("false"));
Review Comment:
nit.: `.orElse("false")` goes to new line or not. Both are good but please
be consistent.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java:
##########
@@ -105,6 +114,25 @@ public int execute() throws HiveException {
return 0;
}
+ private void pushExternalLogicalViewSessionHints(boolean replace, boolean
ifNotExists) {
+ SessionStateUtil.addResource(context.getConf(),
Constants.EXTERNAL_LOGICAL_VIEW_DDL_REPLACE,
+ Boolean.toString(replace));
+ SessionStateUtil.addResource(context.getConf(),
Constants.EXTERNAL_LOGICAL_VIEW_CREATE_IF_NOT_EXISTS,
+ Boolean.toString(ifNotExists));
+ }
Review Comment:
Why does these has to be added to session state?
IIUC `replace` and `ifNotExists` is handled here in `CreateViewOperation`
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/MetastoreUtil.java:
##########
@@ -148,6 +153,74 @@ public static Table toHiveTable(org.apache.iceberg.Table
table, Configuration co
return result;
}
+ /**
+ * Builds a Hive metastore {@link Table} representation for an Iceberg
{@link View}, for clients
+ * (e.g. {@code HiveRESTCatalogClient}) that bridge Iceberg catalog metadata
into the HMS API.
+ */
+ public static Table toHiveView(View view, Configuration conf) {
+ Table result = new Table();
+ TableName tableName =
+ TableName.fromString(
+ view.name(), MetaStoreUtils.getDefaultCatalog(conf),
Warehouse.DEFAULT_DATABASE_NAME);
+ result.setCatName(tableName.getCat());
+ result.setDbName(tableName.getDb());
Review Comment:
Sorry, my question was inspired by the fact that the database name is
hardcoded, which makes it impossible to handle views in databases other than
'default'.
> this method is only called when reading from a view using a REST Catalog -
it is called from `HiveRESTCatalogClient.getTable(GetTableRequest tableRequest)`
Does this imply that I am unable to reference views from any database other
than `default` ?
```
select * from my_db.my_view;
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
