kasakrisz commented on code in PR #6449:
URL: https://github.com/apache/hive/pull/6449#discussion_r3323219130
##########
itests/hive-iceberg/src/test/java/org/apache/hive/TestHiveRESTCatalogClientITBase.java:
##########
@@ -195,6 +207,50 @@ public void testIceberg() throws Exception {
Assertions.assertFalse(msClient.getAllDatabases(CATALOG_NAME).contains(DB_NAME));
}
+ @Test
+ public void testNativeIcebergLogicalView() 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", ""));
+ IcebergNativeLogicalViewSupport.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());
Review Comment:
Can `view` be null?
##########
itests/hive-iceberg/src/test/java/org/apache/hive/TestHiveRESTCatalogClientITBase.java:
##########
@@ -195,6 +207,50 @@ public void testIceberg() throws Exception {
Assertions.assertFalse(msClient.getAllDatabases(CATALOG_NAME).contains(DB_NAME));
}
+ @Test
+ public void testNativeIcebergLogicalView() 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", ""));
+ IcebergNativeLogicalViewSupport.createOrReplaceNativeView(
Review Comment:
Why don't we use `msClient.createTable` ?
##########
itests/hive-iceberg/src/test/java/org/apache/hive/TestHiveRESTCatalogClientITBase.java:
##########
@@ -195,6 +207,50 @@ public void testIceberg() throws Exception {
Assertions.assertFalse(msClient.getAllDatabases(CATALOG_NAME).contains(DB_NAME));
}
+ @Test
+ public void testNativeIcebergLogicalView() 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", ""));
+ IcebergNativeLogicalViewSupport.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));
Review Comment:
It might be better to move this before `GetTableRequest getTableRequest =
new GetTableRequest();`
##########
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:
These are not used in case of `HiveIcebergMetaHook`:
1. When view does not exist it always goes to
`HiveIcebergMetaHook.commitCreateTable`. The flags' values `replace` and
`ifNotExists` doesn't matter the view has to be created.
2. When view exists and `ifNotExists` is `true` the operation is aborted
here:
https://github.com/kasakrisz/hive/blob/1ba1712e9e5a32534aedcc70a2b66843b6d71421/ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java#L65-L68
3. When view exists and `replace` is `true` alter table and
`HiveIcebergMetaHook.commitAlterTable` is called
4. When view exists and both `replace` and `ifNotExists` are `false` then we
throw View already exists here
https://github.com/kasakrisz/hive/blob/1ba1712e9e5a32534aedcc70a2b66843b6d71421/ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java#L71
5. When view does not exist and both `replace` and `ifNotExists` are `true`
we throw exception because only one of them is allowed at a time
https://github.com/kasakrisz/hive/blob/1ba1712e9e5a32534aedcc70a2b66843b6d71421/ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewAnalyzer.java#L80-L81
I haven't found any tests for replace and if-not-exists in case of
`HiveRESTCatalogClient`. Please add some. I expect that these flags are handled
by `CreateViewOperation` in this case too and don't need to be added to the
session state.
##########
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:
Maybe I'm missing something, but I can't find this test. Could you please
point it out?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewAnalyzer.java:
##########
@@ -191,6 +206,56 @@ private List<FieldSchema> getPartitionColumns(List<String>
partitionColumnNames)
return partitionColumnsCopy;
}
+ /**
+ * Returns the FQCN of the storage handler that should own external logical
view metadata, or {@code null} for a
+ * classic HMS virtual view. Uses {@code storageHandlerClassFromTableProps}
when non-null (from the
+ * {@code view-format} table property); otherwise the Hive config {@code
hive.default.storage.handler.class}.
+ */
+ private String resolveViewStorageHandlerClass(String
storageHandlerClassFromTableProps)
+ throws SemanticException {
+
+ String storageHandlerClassFromConfig =
+ StringUtils.trimToNull(HiveConf.getVar(conf,
HiveConf.ConfVars.HIVE_DEFAULT_STORAGE_HANDLER));
+
+ String storageHandlerClass =
+ storageHandlerClassFromTableProps != null ?
storageHandlerClassFromTableProps : storageHandlerClassFromConfig;
+
+ if (StringUtils.isBlank(storageHandlerClass)) {
+ return null;
+ }
+
+ boolean explicitViewFormat = storageHandlerClassFromTableProps != null;
+ try {
+ HiveStorageHandler storageHandler = HiveUtils.getStorageHandler(conf,
storageHandlerClass);
+
+ if (storageHandler != null &&
storageHandler.supportsExternalLogicalViewCatalog()) {
+ return storageHandlerClass;
+ }
+ } catch (HiveException e) {
+
+ if (explicitViewFormat) {
+ throw new
SemanticException(ErrorMsg.VIEW_STORAGE_HANDLER_UNSUPPORTED.format(storageHandlerClass),
e);
+ }
+ }
Review Comment:
Why does it matter where `storageHandlerClass` is defined? If there is an
issue while creating an instance, the statement fails anyway. IMHO, the error
message is misleading because `HiveException` is thrown by
`HiveUtils.getStorageHandler`, which is responsible only for creating the
instance.
--
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]
