zhangbutao commented on code in PR #6267:
URL: https://github.com/apache/hive/pull/6267#discussion_r3069111332
##########
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestMiniClusters.java:
##########
@@ -554,23 +556,27 @@ private void setFsRelatedProperties(HiveConf conf,
boolean isLocalFs, FileSystem
// Create a fake fs root for local fs
Path localFsRoot = new Path(path, "localfs");
warehousePath = new Path(localFsRoot, "warehouse");
+ warehouseCatPath = new Path(localFsRoot, "catalog");
Review Comment:
> shouldn't the catalog be under the warehousePath?
No. The `warehousePath` is the path of the default catalog, under which the
paths of Hive databases reside. If the catalog path is placed under the
`warehousePath`, it will cause confusion with the path information of Hive
databases under the default catalog.
The path information for the default catalog's databases is controlled by
the property `metastore.warehouse.dir`, which defaults to
`/user/hive/warehouse`. The path for non-default catalogs is controlled by the
new added property`metastore.warehouse.catalog.dir`, which defaults to
`/user/hive/catalog/warehouse`.
```
/user/hive/catalog/warehouse/<catalog_name>/<database_name>.db/<table_name>/
# non default catalog
/user/hive/warehouse/<database_name>.db/<table_name>/
# default catalog
/user/hive/warehouse/default.db/<table_name>/
# default catalog & database
```
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/catalog/create/CreateCatalogOperation.java:
##########
@@ -37,7 +38,11 @@ public CreateCatalogOperation(DDLOperationContext context,
CreateCatalogDesc des
@Override
public int execute() throws Exception {
- Catalog catalog = new Catalog(desc.getName(), desc.getLocationUri());
+ String catLocationUri = Optional.ofNullable(desc.getLocationUri())
Review Comment:
Sorry, i didn't get you here. Could you please describe it in detail? Thank
you.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/database/create/CreateDatabaseOperation.java:
##########
@@ -81,34 +81,62 @@ public int execute() throws HiveException {
}
private void makeLocationQualified(Database database) throws HiveException {
+ String catalogName = database.getCatalogName().toLowerCase();
+ String dbName = database.getName().toLowerCase();
+ boolean isDefaultCatalog =
Warehouse.DEFAULT_CATALOG_NAME.equalsIgnoreCase(catalogName);
Review Comment:
Fixed. Added a util method:
`HiveUtils.isDefaultCatalog(String catName, Configuration conf)`
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:
##########
@@ -132,6 +132,8 @@ public void authorizeDbLevelOperations(Privilege[]
readRequiredPriv, Privilege[]
Path root = null;
try {
initWh();
+ // TODO catalog. Need to determine auth root path based on catalog name.
Review Comment:
Create ticket HIVE-29562
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -86,17 +90,26 @@ public class Warehouse {
public Warehouse(Configuration conf) throws MetaException {
this.conf = conf;
- whRootString = MetastoreConf.getVar(conf, ConfVars.WAREHOUSE);
- if (StringUtils.isBlank(whRootString)) {
- throw new MetaException(ConfVars.WAREHOUSE.getVarname()
- + " is not set in the config or blank");
- }
+
+ whRootString = getRequiredVar(conf, ConfVars.WAREHOUSE);
+ whCatRootString = getRequiredVar(conf, ConfVars.WAREHOUSE_CATALOG);
Review Comment:
Already repied above.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/database/create/CreateDatabaseAnalyzer.java:
##########
@@ -51,10 +52,9 @@ public CreateDatabaseAnalyzer(QueryState queryState) throws
SemanticException {
@Override
public void analyzeInternal(ASTNode root) throws SemanticException {
Pair<String, String> catDbNamePair = DDLUtils.getCatDbNamePair((ASTNode)
root.getChild(0));
Review Comment:
I think using a `Pair `here is already simple and clear enough, there's no
need to use the `record CatalogDb`.
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -79,6 +79,7 @@ private Catalog createCatalog() {
if (configUri != null && !configUri.isEmpty()) {
properties.put("uri", configUri);
}
+ // TODO catalog. Consider adding new created native catalog
warehouse(MetastoreConf.ConfVars.WAREHOUSE_CATALOG) later.
Review Comment:
Fixed. This just maybe a question. I created a ticket HIVE-28879 for this.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/catalog/create/CreateCatalogAnalyzer.java:
##########
@@ -67,10 +73,31 @@ public void analyzeInternal(ASTNode root) throws
SemanticException {
}
}
+ // Initialize props if null, and set default type to native if not
specified
+ if (props == null) {
+ props = new HashMap<>();
+ }
+ checkCatalogType(props);
+
CreateCatalogDesc desc = new CreateCatalogDesc(catalogName, comment,
locationUrl, ifNotExists, props);
Catalog catalog = new Catalog(catalogName, locationUrl);
rootTasks.add(TaskFactory.get(new DDLWork(getInputs(), getOutputs(),
desc)));
outputs.add(new WriteEntity(catalog, WriteEntity.WriteType.DDL_NO_LOCK));
}
+
+ private static void checkCatalogType(Map<String, String> props) throws
SemanticException {
+ String catalogType = props.get("type");
Review Comment:
Sure. Create constan `TYPE` in CatalogUtil
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java:
##########
@@ -388,14 +389,34 @@ public static void createDbExportDump(FileSystem fs, Path
metadataPath, Database
private static void updateIfCustomDbLocations(Database database,
Configuration conf) throws SemanticException {
try {
- String whLocatoion = MetastoreConf.getVar(conf,
MetastoreConf.ConfVars.WAREHOUSE_EXTERNAL,
- MetastoreConf.getVar(conf, MetastoreConf.ConfVars.WAREHOUSE));
- Path dbDerivedLoc = new Path(whLocatoion,
database.getName().toLowerCase() + DATABASE_PATH_SUFFIX);
+ String catName = database.getCatalogName();
+ String dbName = database.getName().toLowerCase();
+ boolean isDefaultCatalog =
Warehouse.DEFAULT_CATALOG_NAME.equals(catName);
+
+ // external warehouse root
+ String whLocation = MetastoreConf.getVar(conf,
+ isDefaultCatalog ? MetastoreConf.ConfVars.WAREHOUSE_EXTERNAL :
MetastoreConf.ConfVars.WAREHOUSE_CATALOG_EXTERNAL,
+ MetastoreConf.getVar(conf,
+ isDefaultCatalog ? MetastoreConf.ConfVars.WAREHOUSE :
MetastoreConf.ConfVars.WAREHOUSE_CATALOG));
+
+ if (!isDefaultCatalog) {
+ whLocation = new Path(whLocation, catName).toString();
+ }
+
+ Path dbDerivedLoc = new Path(whLocation, dbName + DATABASE_PATH_SUFFIX);
String defaultDbLoc = Utilities.getQualifiedPath((HiveConf) conf,
dbDerivedLoc);
database.putToParameters(ReplConst.REPL_IS_CUSTOM_DB_LOC,
Boolean.toString(!defaultDbLoc.equals(database.getLocationUri())));
- String whManagedLocatoion = MetastoreConf.getVar(conf,
MetastoreConf.ConfVars.WAREHOUSE);
- Path dbDerivedManagedLoc = new Path(whManagedLocatoion,
database.getName().toLowerCase()
+
+ // managed warehouse root
+ String whManagedLocatoion = MetastoreConf.getVar(conf,
+ isDefaultCatalog ? MetastoreConf.ConfVars.WAREHOUSE
Review Comment:
Replied in https://github.com/apache/hive/pull/6267#discussion_r3068019699
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/catalog/create/CreateCatalogAnalyzer.java:
##########
@@ -67,10 +73,31 @@ public void analyzeInternal(ASTNode root) throws
SemanticException {
}
}
+ // Initialize props if null, and set default type to native if not
specified
+ if (props == null) {
+ props = new HashMap<>();
Review Comment:
Sure. Changed.
##########
ql/src/java/org/apache/hadoop/hive/ql/queryhistory/repository/AbstractRepository.java:
##########
@@ -82,7 +85,11 @@ protected Database initDatabase(Hive hive) {
db = hive.getDatabase(QUERY_HISTORY_DB_NAME);
if (db == null) {
LOG.warn("Database ({}) for query history table hasn't been found,
auto-creating one", QUERY_HISTORY_DB_NAME);
- String location = getDatabaseLocation(QUERY_HISTORY_DB_NAME);
+ db = new Database();
Review Comment:
Fixed.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/database/create/CreateDatabaseOperation.java:
##########
@@ -81,34 +81,62 @@ public int execute() throws HiveException {
}
private void makeLocationQualified(Database database) throws HiveException {
+ String catalogName = database.getCatalogName().toLowerCase();
Review Comment:
At this stage, the database is being created, and the prerequisite is that a
valid catalog already exists — there is no need to double validate the catalog
here.
##########
ql/src/test/queries/clientnegative/lockneg_try_lock_cat_db_in_use.q:
##########
@@ -1,7 +1,7 @@
set hive.lock.numretries=0;
set hive.support.concurrency=true;
-CREATE CATALOG testcat LOCATION '/tmp/testcat' COMMENT 'Hive test catalog';
+CREATE CATALOG testcat LOCATION '/tmp/testcat' COMMENT 'Hive test catalog'
PROPERTIES('type'='native');
Review Comment:
Removed.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/catalog/create/CreateCatalogAnalyzer.java:
##########
@@ -67,10 +73,31 @@ public void analyzeInternal(ASTNode root) throws
SemanticException {
}
}
+ // Initialize props if null, and set default type to native if not
specified
+ if (props == null) {
+ props = new HashMap<>();
+ }
+ checkCatalogType(props);
+
CreateCatalogDesc desc = new CreateCatalogDesc(catalogName, comment,
locationUrl, ifNotExists, props);
Catalog catalog = new Catalog(catalogName, locationUrl);
rootTasks.add(TaskFactory.get(new DDLWork(getInputs(), getOutputs(),
desc)));
outputs.add(new WriteEntity(catalog, WriteEntity.WriteType.DDL_NO_LOCK));
}
+
+ private static void checkCatalogType(Map<String, String> props) throws
SemanticException {
+ String catalogType = props.get("type");
+ // Set default type to native if not specified
+ if (Strings.isNullOrEmpty(catalogType)) {
+ props.put("type", CatalogUtil.NATIVE);
+ return;
+ }
+ // Normalize and validate catalog type (fail fast on invalid input)
+ try {
+ props.put("type", CatalogUtil.normalizeCatalogType(catalogType));
Review Comment:
Sure. fixed.
##########
ql/src/test/queries/clientpositive/catalog.q:
##########
@@ -7,22 +7,22 @@ dfs -mkdir -p hdfs:///tmp/test_cat;
-- SORT_QUERY_RESULTS
SHOW CATALOGS;
--- CREATE with comment
-CREATE CATALOG test_cat LOCATION 'hdfs:///tmp/test_cat' COMMENT 'Hive test
catalog';
+-- CREATE with comment and default location
+CREATE CATALOG test_cat COMMENT 'Hive test catalog'
PROPERTIES('type'='NATIVE');
Review Comment:
Removed.
I also left a few DDL statements with 'type' = 'hive' to indicate that this
parameter can be omitted for the Hive internal catalog.
##########
ql/src/test/queries/clientpositive/catalog.q:
##########
@@ -7,22 +7,22 @@ dfs -mkdir -p hdfs:///tmp/test_cat;
-- SORT_QUERY_RESULTS
SHOW CATALOGS;
--- CREATE with comment
-CREATE CATALOG test_cat LOCATION 'hdfs:///tmp/test_cat' COMMENT 'Hive test
catalog';
+-- CREATE with comment and default location
+CREATE CATALOG test_cat COMMENT 'Hive test catalog'
PROPERTIES('type'='NATIVE');
-- DESCRIBE
DESC CATALOG test_cat;
-- CREATE INE already exists
-CREATE CATALOG IF NOT EXISTS test_cat LOCATION 'hdfs:///tmp/test_cat';
+CREATE CATALOG IF NOT EXISTS test_cat LOCATION 'hdfs:///tmp/test_cat'
PROPERTIES('type'='native');
Review Comment:
Currently, this PR is specifically targeting the internal/native catalog. If
we were to implement a new external catalog, such as a JDBC catalog, I believe
it would still require a lot of additional preliminary work(such as
https://github.com/apache/hive/pull/6252), so we can consider doing it later.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/CatalogUtil.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.hadoop.hive.metastore;
+
+public class CatalogUtil {
+ // Catalog type constants
+ public static final String NATIVE = "native";
Review Comment:
Fixed.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java:
##########
@@ -631,6 +631,8 @@ private static void setCmRootPermissions(Path cmroot)
throws IOException{
FileSystem cmFs = cmroot.getFileSystem(conf);
cmFs.setPermission(cmroot, new FsPermission("770"));
try {
+ // TODO catalog. The Repl function is currently only available for the
default catalog path ConfVars.WAREHOUSE.
Review Comment:
Sure. See HIVE-29278
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -279,27 +330,56 @@ public Path getDefaultDatabasePath(String dbName) throws
MetaException {
return getDefaultDatabasePath(dbName, false);
}
+ public Path getDefaultDatabasePath(Database db) throws MetaException {
+ return getDefaultDatabasePath(db, false);
+ }
+
+ /**
+ * @deprecated use {@link #getDefaultExternalDatabasePath(Database)}
+ */
public Path getDefaultExternalDatabasePath(String dbName) throws
MetaException {
return getDefaultDatabasePath(dbName, true);
}
+ public Path getDefaultExternalDatabasePath(Database db) throws MetaException
{
+ return getDefaultDatabasePath(db, true);
+ }
+
+ /**
+ * @deprecated use {@link #getDefaultDatabasePath(Database, boolean)}
+ */
// should only be used to determine paths before the creation of databases
public Path getDefaultDatabasePath(String dbName, boolean inExternalWH)
throws MetaException {
- if (inExternalWH) {
- if (dbName.equalsIgnoreCase(DEFAULT_DATABASE_NAME)) {
- return getWhRootExternal();
- }
- return new Path(getWhRootExternal(), dbName.toLowerCase() +
DATABASE_WAREHOUSE_SUFFIX);
- } else {
- if (dbName.equalsIgnoreCase(DEFAULT_DATABASE_NAME)) {
- return getWhRoot();
- }
- return new Path(getWhRoot(), dbName.toLowerCase() +
DATABASE_WAREHOUSE_SUFFIX);
+ Database db = new Database();
Review Comment:
I think this should work well. Regarding the changes made here, could you
help me point out the more prominent advantages of using `record CatalogDb`?
Thank you.
--
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]
