kasakrisz commented on code in PR #6449:
URL: https://github.com/apache/hive/pull/6449#discussion_r3218914237
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -445,6 +445,8 @@ public enum ErrorMsg {
@Deprecated // kept for backwards reference
REPLACE_VIEW_WITH_MATERIALIZED(10400, "Attempt to replace view {0} with
materialized view", true),
REPLACE_MATERIALIZED_WITH_VIEW(10401, "Attempt to replace materialized view
{0} with view", true),
+ VIEW_STORAGE_HANDLER_UNSUPPORTED(10448, "CREATE VIEW only supports STORED BY
ICEBERG for native "
+ + "Iceberg views; unsupported storage clause: {0}", true),
Review Comment:
Please rephrase this error message. Remove `STORED BY` and let `Iceberg` be
a parameter.
##########
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:
What happens when a customer db is specified?
```
create view my_db.myview as...
```
##########
iceberg/iceberg-catalog/src/test/java/org/apache/iceberg/hive/TestNativeIcebergViewSupport.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();
+ }
+
+ @Test
+ void testCreateOrReplaceNativeViewReplacesExistingWhenReplaceFlagTrue()
throws Exception {
+ HiveConf conf = nativeViewConf();
+ List<FieldSchema> cols = Collections.singletonList(new FieldSchema("id",
"int", null));
+
+ NativeIcebergViewSupport.createOrReplaceNativeView(
+ conf, DB, VIEW, cols, "select 1 as id", null, null, false, false);
+ assertThat(
+ NativeIcebergViewSupport.createOrReplaceNativeView(
+ conf, DB, VIEW, cols, "select 2 as id", null, null, true,
false))
+ .isTrue();
+
+ assertThat(verifyCatalog().viewExists(TableIdentifier.of(DB,
VIEW))).isTrue();
Review Comment:
I think it worth checking if the view definition is actually altered.
##########
iceberg/iceberg-handler/src/test/queries/positive/iceberg_native_view.q:
##########
@@ -0,0 +1,53 @@
+-- SORT_QUERY_RESULTS
+-- Mask random uuid
+--! qt:replace:/(\s+uuid\s+)\S+(\s*)/$1#Masked#$2/
+
+create database ice_native_view_db;
+use ice_native_view_db;
+
+create table src_ice (
+ first_name string,
+ last_name string
+ )
+partitioned by (dept_id bigint)
+stored by iceberg stored as orc;
+
+INSERT INTO src_ice VALUES
+ ('fn1','ln1', 1),
+ ('fn2','ln2', 1),
+ ('fn3','ln3', 1),
+ ('fn4','ln4', 1),
+ ('fn5','ln5', 2),
+ ('fn6','ln6', 2),
+ ('fn7','ln7', 2);
+
+----------------------------------------------------------------
+-- Iceberg native view via TBLPROPERTIES before AS
+----------------------------------------------------------------
+
+create view v_ice tblproperties ('view-format'='iceberg') as select * from
src_ice;
+select * from v_ice;
+desc formatted v_ice;
+drop view v_ice;
+
+----------------------------------------------------------------
+-- Native Iceberg view when default storage handler is Iceberg (no
TBLPROPERTIES view-format)
+----------------------------------------------------------------
+
+set
hive.default.storage.handler.class=org.apache.iceberg.mr.hive.HiveIcebergStorageHandler;
+
+create view v_def as select * from src_ice;
+select first_name, dept_id from v_def;
+desc formatted v_def;
+drop view v_def;
+
+----------------------------------------------------------------
+-- Classic Hive view when default storage handler is unset
+----------------------------------------------------------------
+
+set hive.default.storage.handler.class=;
+
+create view v_hive as select * from src_ice;
+select * from v_hive;
+desc formatted v_hive;
+drop view v_hive;
Review Comment:
AFAIK we already have coverage for Hive views. Please mention in the comment
that the base table is iceberg.
##########
iceberg/iceberg-catalog/src/test/java/org/apache/iceberg/hive/TestNativeIcebergViewSupport.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() {
Review Comment:
Why is this called `verify`? Based on the implementation of this method it
is more like loading a catalog.
Could you please share some background of the use case of this method.
##########
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
Review Comment:
`STORED BY` is not supported by the grammar in create view
##########
itests/hive-iceberg/src/test/java/org/apache/hive/TestHiveRESTCatalogClientITBase.java:
##########
@@ -195,6 +201,50 @@ public void testIceberg() throws Exception {
Assertions.assertFalse(msClient.getAllDatabases(CATALOG_NAME).contains(DB_NAME));
}
+ @Test
+ public void testNativeIcebergView() throws Exception {
+ Database db = new Database();
+ db.setCatalogName(CATALOG_NAME);
+ db.setName(VIEW_DB_NAME);
+ db.setOwnerType(PrincipalType.USER);
+ db.setOwnerName(System.getProperty("user.name"));
+ String warehouseDir = MetastoreConf.get(conf,
MetastoreConf.ConfVars.WAREHOUSE_EXTERNAL.getVarname());
+ db.setLocationUri(warehouseDir + "/" + VIEW_DB_NAME + ".db");
+ hive.createDatabase(db, true);
+
+ List<FieldSchema> cols = Collections.singletonList(new FieldSchema("x",
"int", ""));
+ NativeIcebergViewSupport.createOrReplaceNativeView(
+ hiveConf,
+ VIEW_DB_NAME,
+ NATIVE_VIEW_NAME,
+ cols,
+ "select 1 as x",
+ null,
+ "rest-native-view",
+ false,
+ false);
+
+ GetTableRequest getTableRequest = new GetTableRequest();
+ getTableRequest.setCatName(CATALOG_NAME);
+ getTableRequest.setDbName(VIEW_DB_NAME);
+ getTableRequest.setTblName(NATIVE_VIEW_NAME);
+ Table view = msClient.getTable(getTableRequest);
+ Assertions.assertEquals(TableType.VIRTUAL_VIEW.name(),
view.getTableType());
+ String tableTypeProp =
view.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP);
+ Assertions.assertNotNull(tableTypeProp);
+ Assertions.assertEquals(ICEBERG_VIEW_TYPE_VALUE,
tableTypeProp.toLowerCase());
+
+ Assertions.assertTrue(msClient.tableExists(CATALOG_NAME, VIEW_DB_NAME,
NATIVE_VIEW_NAME));
+
+ List<String> names = msClient.getTables(CATALOG_NAME, VIEW_DB_NAME, "*");
Review Comment:
What is the difference between `msClient.getTables` and `msClient.getTable` ?
Aren't they call the same API ?
##########
itests/src/test/resources/testconfiguration.properties:
##########
@@ -429,11 +429,11 @@ iceberg.llap.query.files=\
iceberg_bucket_map_join_7.q,\
iceberg_bucket_map_join_8.q,\
iceberg_clustered.q,\
- iceberg_clustered_by.q,\
iceberg_create_locally_ordered_table.q,\
iceberg_create_locally_zordered_table.q,\
iceberg_merge_delete_files.q,\
iceberg_merge_files.q,\
+ iceberg_native_view.q,\
Review Comment:
Is there any LLAP specific in view DDLs ? If not; can this test run by the
default Iceberg driver?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -89,6 +89,23 @@ public String outputFormat() {
}
}
+ /**
+ * Resolves {@code STORED BY identifier} for CREATE VIEW (short names such
as {@code ICEBERG} or an FQCN).
Review Comment:
Was `STORED BY` left here intentionally?
##########
iceberg/iceberg-handler/src/test/queries/positive/iceberg_native_view.q:
##########
@@ -0,0 +1,38 @@
+-- SORT_QUERY_RESULTS
+
+create database ice_native_view_db;
+use ice_native_view_db;
+
+create table src_ice (
+ first_name string,
+ last_name string
+ )
+partitioned by (dept_id bigint)
+stored by iceberg stored as orc;
+
+insert into src_ice values ('fn1','ln1', 1);
+insert into src_ice values ('fn2','ln2', 1);
+insert into src_ice values ('fn3','ln3', 1);
+insert into src_ice values ('fn4','ln4', 1);
+insert into src_ice values ('fn5','ln5', 2);
+insert into src_ice values ('fn6','ln6', 2);
+insert into src_ice values ('fn7','ln7', 2);
+
+update src_ice set last_name = 'ln1a' where first_name='fn1';
+update src_ice set last_name = 'ln2a' where first_name='fn2';
+update src_ice set last_name = 'ln3a' where first_name='fn3';
+update src_ice set last_name = 'ln4a' where first_name='fn4';
+update src_ice set last_name = 'ln5a' where first_name='fn5';
+update src_ice set last_name = 'ln6a' where first_name='fn6';
+update src_ice set last_name = 'ln7a' where first_name='fn7';
+
+delete from src_ice where last_name in ('ln1a', 'ln2a', 'ln7a');
+
+create view v_ice as select * from src_ice stored by iceberg;
+
+select * from v_ice;
+
Review Comment:
> logical view which does some transformation on it's base table
Sorry I mean something like
```
select first_name || last_name from ... where <some filter condition>
```
because
```
select * from table;
```
as a view definition is a kind of edge case. It is ok for testing but not a
typical 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;
Review Comment:
CBO off is not supported and there is a plan to remove this setting
https://issues.apache.org/jira/browse/HIVE-27830
##########
iceberg/iceberg-handler/src/test/queries/positive/iceberg_rest_catalog_hms.q:
##########
@@ -69,6 +67,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;
Review Comment:
CBO off is not supported and there is a plan to remove this setting
https://issues.apache.org/jira/browse/HIVE-27830
What is blocking the Iceberg view support?
##########
iceberg/iceberg-handler/src/test/queries/positive/iceberg_native_view.q:
##########
@@ -0,0 +1,53 @@
+-- SORT_QUERY_RESULTS
+-- Mask random uuid
+--! qt:replace:/(\s+uuid\s+)\S+(\s*)/$1#Masked#$2/
+
+create database ice_native_view_db;
+use ice_native_view_db;
+
+create table src_ice (
+ first_name string,
+ last_name string
+ )
+partitioned by (dept_id bigint)
+stored by iceberg stored as orc;
+
+INSERT INTO src_ice VALUES
+ ('fn1','ln1', 1),
+ ('fn2','ln2', 1),
+ ('fn3','ln3', 1),
+ ('fn4','ln4', 1),
+ ('fn5','ln5', 2),
+ ('fn6','ln6', 2),
+ ('fn7','ln7', 2);
+
+----------------------------------------------------------------
+-- Iceberg native view via TBLPROPERTIES before AS
Review Comment:
Does `before AS` adds any value in this comment? AFAIK the grammar allows
this way only.
##########
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:
Where is the catalog specified?
##########
iceberg/iceberg-handler/src/test/queries/positive/iceberg_rest_catalog_hms.q:
##########
@@ -69,6 +67,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
Review Comment:
`STORED BY` is not supported by the grammar in create view
##########
itests/src/test/resources/testconfiguration.properties:
##########
@@ -481,11 +481,11 @@ iceberg.llap.only.query.files=\
iceberg_bucket_map_join_7.q,\
iceberg_bucket_map_join_8.q,\
iceberg_clustered.q,\
- iceberg_clustered_by.q,\
iceberg_create_locally_ordered_table.q,\
iceberg_create_locally_zordered_table.q,\
iceberg_merge_delete_files.q,\
iceberg_merge_files.q,\
+ iceberg_native_view.q,\
Review Comment:
Is there any LLAP specific in view DDLs ? If not; can this test run by the
default Iceberg driver?
##########
parser/src/test/org/apache/hadoop/hive/ql/parse/TestParseDefault.java:
##########
@@ -90,6 +90,16 @@ public void testParseStructNamedDefault() throws Exception {
assertFalse(tree.dump(),
tree.toStringTree().contains("tok_default_value"));
}
+ @Test
+ public void testParseCreateViewTblpropertiesViewFormatIceberg() throws
Exception {
+ ASTNode tree = parseDriver.parse(
+ "create view v1 tblproperties ('view-format'='iceberg') as select *
from t", null).getTree();
+ assertTrue(tree.dump(), tree.toStringTree().contains("tok_createview"));
+ assertTrue(tree.dump(),
tree.toStringTree().contains("tok_tableproperties"));
+ assertTrue(tree.dump(), tree.toStringTree().contains("view-format"));
+ assertTrue(tree.dump(), tree.toStringTree().contains("iceberg"));
+ }
+
Review Comment:
This test class is about the `default` keyword testing. Could you please
move this test to the one which is about view creation. Please create a new one
of not exists. Since this grammar is not Iceberg specific it can be a generic
one.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewAnalyzer.java:
##########
@@ -191,6 +200,58 @@ private List<FieldSchema> getPartitionColumns(List<String>
partitionColumnNames)
return partitionColumnsCopy;
}
+ /**
+ * Returns the FQCN of the storage handler that should own native-catalog
view metadata, or {@code null} for a
+ * classic HMS virtual view. If {@code TBLPROPERTIES} contains {@code
view-format}, its value is resolved the same
+ * way as a {@code STORED BY} handler identifier (short name or FQCN) and
used when it
+ * {@link HiveStorageHandler#supportsNativeViewCatalog()}. Otherwise, when
{@code view-format} is absent,
+ * {@link HiveConf.ConfVars#HIVE_DEFAULT_STORAGE_HANDLER} is used if set and
the handler supports native views.
+ */
+ private String resolveNativeViewStorageHandlerClass(Map<String, String>
properties) throws SemanticException {
+ String fromViewFormat = findViewFormatHandlerClass(properties);
+ String fromDefaultConfig =
+ StringUtils.trimToNull(HiveConf.getVar(conf,
HiveConf.ConfVars.HIVE_DEFAULT_STORAGE_HANDLER));
+ String handlerClass = fromViewFormat != null ? fromViewFormat :
fromDefaultConfig;
+ if (StringUtils.isBlank(handlerClass)) {
+ return null;
+ }
+ return resolveNativeHandlerClass(handlerClass, fromViewFormat != null);
+ }
+
+ private static String findViewFormatHandlerClass(Map<String, String>
properties) throws SemanticException {
+ if (properties == null) {
+ return null;
+ }
+ for (Map.Entry<String, String> e : properties.entrySet()) {
+ if (e.getKey() != null
+ && VIEW_FORMAT_TABLE_PROPERTY.equalsIgnoreCase(e.getKey())
+ && StringUtils.isNotBlank(e.getValue())) {
+ return
StorageFormat.resolveStorageHandlerClassNameForView(e.getValue().trim());
+ }
+ }
+ return null;
+ }
+
+ private String resolveNativeHandlerClass(String handlerClass, boolean
explicitViewFormat)
+ throws SemanticException {
+ try {
+ HiveStorageHandler handler = HiveUtils.getStorageHandler(conf,
handlerClass);
+ if (handler != null && handler.supportsNativeViewCatalog()) {
+ return handlerClass;
+ }
+ } catch (HiveException e) {
+ if (explicitViewFormat) {
+ throw new
SemanticException(ErrorMsg.VIEW_STORAGE_HANDLER_UNSUPPORTED.getMsg(e.getMessage()),
e);
+ }
+ return null;
+ }
+ if (explicitViewFormat) {
+ throw new
SemanticException(ErrorMsg.VIEW_STORAGE_HANDLER_UNSUPPORTED.getMsg(
+ "Native view metadata is not supported for storage handler: " +
handlerClass));
+ }
Review Comment:
We have the knowledge about explicit view format at the caller.
Could you please move this check to the caller method.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java:
##########
@@ -46,6 +51,11 @@ public CreateViewOperation(DDLOperationContext context,
CreateViewDesc desc) {
@Override
public int execute() throws HiveException {
+ if (desc.usesNativeViewCatalog()) {
+ executeNativeCatalogView();
+ return 0;
+ }
Review Comment:
I think this decision should be moved to `BaseHiveIcebergMetaHook` or
`HiveIcebergMetaHook.preCreateTable` like we do in case of Iceberg tables.
Please set the `StorageHandlerClass` in
`CreateViewOperation.createViewObject` to the newly created `ql.metadata.Table`
object because later this object is passed to HMS and the meta hook.
```
if (desc.usesNativeViewCatalog()) {
storageFormat.setStorageHandler(desc.getNativeViewStorageHandlerClass());
view.setProperty(
org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.META_TABLE_STORAGE,
desc.getNativeViewStorageHandlerClass());
}
```
I'm not a big fan of this meta hook solution but this is whet we have in
case of Iceberg tables and IMHO it is better to be consistent.
Probably `create or replace` and `if not exists` doesn't have to be handled
separately in case of Iceberg views of you make this change.
Please add some tests for `create or replace` and `if not exists` to the q
test `iceberg_native_view.q`
--
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]
