Copilot commented on code in PR #6267:
URL: https://github.com/apache/hive/pull/6267#discussion_r2689761412
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -56,6 +56,7 @@
import org.apache.hadoop.hive.metastore.api.Partition;
import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
import org.apache.hadoop.hive.metastore.api.Table;
+import org.stringtemplate.v4.ST;
Review Comment:
The import `org.stringtemplate.v4.ST` is added but never used in the visible
code changes. This appears to be an unused import that should be removed.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/catalog/create/CreateCatalogAnalyzer.java:
##########
@@ -67,10 +72,23 @@ public void analyzeInternal(ASTNode root) throws
SemanticException {
}
}
+ assert props != null;
Review Comment:
The assertion `assert props != null;` on line 75 will not throw a
user-friendly error if assertions are disabled (which is common in production).
Since the grammar now requires PROPERTIES (line 1136 in HiveParser.g), `props`
should never be null. However, if it is somehow null, this should throw a
proper SemanticException instead of relying on an assertion. Consider replacing
with a proper null check and exception.
```suggestion
if (props == null) {
throw new SemanticException("PROPERTIES clause is required for CREATE
CATALOG");
}
```
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -156,33 +170,67 @@ public Path getDnsPath(Path path) throws MetaException {
* This involves opening the FileSystem corresponding to the warehouse root
* dir (but that should be ok given that this is only called during DDL
* statements for non-external tables).
+ *
+ * @deprecated use {@link #getWhRoot(String)}
*/
public Path getWhRoot() throws MetaException {
- if (whRoot != null) {
- return whRoot;
+ return getWhRoot(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRoot(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName.trim());
+
+ Path rootDir = isDefault ? whRoot : whCatRoot;
+ if (rootDir == null) {
+ rootDir = getDnsPath(new Path(isDefault ? whRootString :
whCatRootString));
+ if (isDefault) {
+ whRoot = rootDir;
+ } else {
+ whCatRoot = rootDir;
+ }
}
- whRoot = getDnsPath(new Path(whRootString));
- return whRoot;
+
+ return isDefault ? rootDir : new Path(rootDir, catalogName);
Review Comment:
The caching logic for `whCatRoot` is incorrect. When a non-default catalog
is requested, the code caches the base path (from `whCatRootString`) in
`whCatRoot`, but then returns `new Path(rootDir, catalogName)` which includes
the catalog name. On subsequent calls, it retrieves the cached path and adds
the catalog name again, resulting in paths like `/base/catalog1/catalog1`. The
cache should store either the base path or the full path with catalog name, but
not mix the two approaches.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -73,10 +74,14 @@ public class Warehouse {
private static final String CAT_DB_TABLE_SEPARATOR = ".";
private Path whRoot;
+ private Path whCatRoot;
private Path whRootExternal;
+ private Path whCatRootExternal;
Review Comment:
The fields `whRoot`, `whCatRoot`, `whRootExternal`, and `whCatRootExternal`
are not thread-safe. Multiple threads could simultaneously check if these
fields are null and attempt to initialize them, leading to race conditions.
These fields should be marked as `volatile` or the getter methods should be
synchronized to ensure thread-safe lazy initialization.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/CatalogUtil.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;
+
+import org.apache.commons.lang3.EnumUtils;
+
+import java.util.EnumSet;
+import java.util.Set;
+
+public class CatalogUtil {
+ public enum CatalogType {
+ NATIVE,
+ ICEBERG
+ }
+
+ public static final Set<CatalogType> VALID_CATALOG_TYPES =
EnumSet.of(CatalogType.NATIVE, CatalogType.ICEBERG);
+
+ public static boolean isValidCatalogType(String name) {
+ CatalogType type = EnumUtils.getEnumIgnoreCase(CatalogType.class,
name);
+ return type != null && VALID_CATALOG_TYPES.contains(type);
Review Comment:
The constant `VALID_CATALOG_TYPES` on line 32 is redundant because it
contains exactly the same values as the `CatalogType` enum. The
`isValidCatalogType` method could simply check if `type != null` after parsing
with `EnumUtils.getEnumIgnoreCase`, making this constant unnecessary. This
reduces maintainability as future catalog types need to be added in two places.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -156,33 +170,67 @@ public Path getDnsPath(Path path) throws MetaException {
* This involves opening the FileSystem corresponding to the warehouse root
* dir (but that should be ok given that this is only called during DDL
* statements for non-external tables).
+ *
+ * @deprecated use {@link #getWhRoot(String)}
*/
public Path getWhRoot() throws MetaException {
- if (whRoot != null) {
- return whRoot;
+ return getWhRoot(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRoot(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName.trim());
+
+ Path rootDir = isDefault ? whRoot : whCatRoot;
+ if (rootDir == null) {
+ rootDir = getDnsPath(new Path(isDefault ? whRootString :
whCatRootString));
+ if (isDefault) {
+ whRoot = rootDir;
+ } else {
+ whCatRoot = rootDir;
+ }
}
- whRoot = getDnsPath(new Path(whRootString));
- return whRoot;
+
+ return isDefault ? rootDir : new Path(rootDir, catalogName);
}
+
+
+ /**
+ * @deprecated use {@link #getWhRootExternal(String)}
+ */
public Path getWhRootExternal() throws MetaException {
- if (whRootExternal != null) {
- return whRootExternal;
+ return getWhRootExternal(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRootExternal(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName);
+
+ Path rootExDir = isDefault ? whRootExternal : whCatRootExternal;
+ if (rootExDir != null) {
+ return isDefault ? rootExDir : new Path(rootExDir, catalogName);
}
- if (!hasExternalWarehouseRoot()) {
- whRootExternal = getWhRoot();
- } else {
- whRootExternal = getDnsPath(new Path(whRootExternalString));
+
+ if (isDefault) {
+ rootExDir = hasExternalWarehouseRoot()
+ ? getDnsPath(new Path(whRootExternalString))
+ : getWhRoot(catalogName);
+ whRootExternal = rootExDir;
+ return rootExDir;
}
- return whRootExternal;
+
+ rootExDir = hasExternalWarehouseRoot(catalogName)
+ ? getDnsPath(new Path(whCatRootExternalString, catalogName))
+ : getWhRoot(catalogName);
+ whCatRootExternal = rootExDir;
+ return rootExDir;
}
Review Comment:
The new methods `getWhRoot(String catalogName)` and
`getWhRootExternal(String catalogName)` lack proper JavaDoc documentation. They
should have JavaDoc comments explaining their purpose, parameters, return
values, and exceptions. This is especially important since the old no-arg
versions are now deprecated in favor of these.
##########
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);
+
+ // -------- External location --------
if (database.isSetLocationUri()) {
database.setLocationUri(Utilities.getQualifiedPath(context.getConf(),
new Path(database.getLocationUri())));
} else {
- // Location is not set we utilize WAREHOUSE_EXTERNAL together with
database name
- String rootDir = MetastoreConf.getVar(context.getConf(),
MetastoreConf.ConfVars.WAREHOUSE_EXTERNAL);
- if (rootDir == null || rootDir.trim().isEmpty()) {
- // Fallback plan
- LOG.warn(String.format(
- "%s is not set, falling back to %s. This could cause external
tables to use to managed tablespace.",
- MetastoreConf.ConfVars.WAREHOUSE_EXTERNAL.getVarname(),
MetastoreConf.ConfVars.WAREHOUSE.getVarname()));
- rootDir = MetastoreConf.getVar(context.getConf(),
MetastoreConf.ConfVars.WAREHOUSE);
- }
- Path path = new Path(rootDir, database.getName().toLowerCase() +
DATABASE_PATH_SUFFIX);
- String qualifiedPath = Utilities.getQualifiedPath(context.getConf(),
path);
- database.setLocationUri(qualifiedPath);
+ String rootDir = getExternalRootDir(isDefaultCatalog);
+ Path path = buildDbPath(rootDir, catalogName, dbName, isDefaultCatalog);
+ database.setLocationUri(Utilities.getQualifiedPath(context.getConf(),
path));
}
+ // -------- Managed location --------
if (database.isSetManagedLocationUri()) {
database.setManagedLocationUri(Utilities.getQualifiedPath(context.getConf(),
new Path(database.getManagedLocationUri())));
} else {
- // ManagedLocation is not set we utilize WAREHOUSE together with
database name
- String rootDir = MetastoreConf.getVar(context.getConf(),
MetastoreConf.ConfVars.WAREHOUSE);
- Path path = new Path(rootDir, database.getName().toLowerCase() +
DATABASE_PATH_SUFFIX);
+ String rootDir = MetastoreConf.getVar(
+ context.getConf(),
+ isDefaultCatalog
+ ? MetastoreConf.ConfVars.WAREHOUSE
+ : MetastoreConf.ConfVars.WAREHOUSE_CATALOG
+ );
+
+ Path path = buildDbPath(rootDir, catalogName, dbName, isDefaultCatalog);
String qualifiedPath = Utilities.getQualifiedPath(context.getConf(),
path);
if (!qualifiedPath.equals(database.getLocationUri())) {
database.setManagedLocationUri(qualifiedPath);
}
}
}
+
+ private Path buildDbPath(String rootDir, String catalogName, String dbName,
boolean isDefaultCatalog) {
+ return isDefaultCatalog
+ ? new Path(rootDir, dbName + DATABASE_PATH_SUFFIX)
+ : new Path(rootDir + "/" + catalogName, dbName +
DATABASE_PATH_SUFFIX);
Review Comment:
The path concatenation on line 120 uses string concatenation (`rootDir + "/"
+ catalogName`) instead of using the Path constructor. This is problematic
because it doesn't account for cases where `rootDir` might already end with a
separator, potentially creating paths with double slashes. Use `new Path(new
Path(rootDir), catalogName)` or similar Path-based construction for proper path
handling.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/EximUtil.java:
##########
@@ -388,9 +389,15 @@ 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);
+ boolean isDefaultCatalog =
Warehouse.DEFAULT_CATALOG_NAME.equals(database.getCatalogName());
+ 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
+ )
+ );
+
+ Path dbDerivedLoc = new Path(whLocation,
database.getName().toLowerCase() + DATABASE_PATH_SUFFIX);
Review Comment:
The path construction on line 400 doesn't account for non-default catalogs.
When determining if a database has a custom location, the code builds
`dbDerivedLoc` from `whLocation` and database name, but for non-default
catalogs, the catalog name should also be included in the path (similar to how
it's done in `CreateDatabaseOperation.buildDbPath()`). This could lead to
incorrect detection of custom database locations for non-default catalogs.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1783,6 +1783,15 @@ public enum ConfVars {
"hive.metastore.warehouse.external.dir", "",
"Default location for external tables created in the warehouse. " +
"If not set or null, then the normal warehouse location will be used
as the default location."),
+ WAREHOUSE_CATALOG("metastore.warehouse.catalog.dir",
+ "hive.metastore.warehouse.catalog.dir",
"/user/hive/catalog/warehouse",
+ "location of default database for the warehouse. If the name of
the catalog to which the db belongs is testcat," +
+ "then the default path prefix for the db is
/user/hive/catalog/warehouse/testcat."),
+ WAREHOUSE_CATALOG_EXTERNAL("metastore.warehouse.catalog.external.dir",
+ "hive.metastore.warehouse.catalog.external.dir", "",
+ "Default location for external tables created in the warehouse. " +
+ "If not set or null, then the normal warehouse
location(MetastoreConf.WAREHOUSE_CATALOG) " +
+ "will be used as the default location."),
Review Comment:
Missing space before the opening parenthesis in the description, similar to
the issue in HiveConf. The text should read "...warehouse location
(MetastoreConf.WAREHOUSE_CATALOG)" instead of "...warehouse
location(MetastoreConf.WAREHOUSE_CATALOG)".
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -156,33 +170,67 @@ public Path getDnsPath(Path path) throws MetaException {
* This involves opening the FileSystem corresponding to the warehouse root
* dir (but that should be ok given that this is only called during DDL
* statements for non-external tables).
+ *
+ * @deprecated use {@link #getWhRoot(String)}
*/
public Path getWhRoot() throws MetaException {
- if (whRoot != null) {
- return whRoot;
+ return getWhRoot(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRoot(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName.trim());
+
+ Path rootDir = isDefault ? whRoot : whCatRoot;
+ if (rootDir == null) {
+ rootDir = getDnsPath(new Path(isDefault ? whRootString :
whCatRootString));
+ if (isDefault) {
+ whRoot = rootDir;
+ } else {
+ whCatRoot = rootDir;
+ }
}
- whRoot = getDnsPath(new Path(whRootString));
- return whRoot;
+
+ return isDefault ? rootDir : new Path(rootDir, catalogName);
}
+
+
+ /**
+ * @deprecated use {@link #getWhRootExternal(String)}
+ */
public Path getWhRootExternal() throws MetaException {
- if (whRootExternal != null) {
- return whRootExternal;
+ return getWhRootExternal(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRootExternal(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName);
Review Comment:
There's an inconsistency in how catalog names are compared. In `getWhRoot()`
on line 181, the code uses `catalogName.trim()` to handle whitespace, but in
`getWhRootExternal()` on line 206, it compares directly without trimming. This
inconsistency could lead to different behavior when catalog names have leading
or trailing whitespace. Both methods should use the same comparison approach.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/database/create/CreateDatabaseAnalyzer.java:
##########
@@ -51,10 +54,16 @@ public CreateDatabaseAnalyzer(QueryState queryState) throws
SemanticException {
@Override
public void analyzeInternal(ASTNode root) throws SemanticException {
Pair<String, String> catDbNamePair = DDLUtils.getCatDbNamePair((ASTNode)
root.getChild(0));
- String catalogName = catDbNamePair.getLeft();
- if (catalogName != null && getCatalog(catalogName) == null) {
+ String catalogName = Optional.ofNullable(catDbNamePair.getLeft())
+ .orElse(HiveUtils.getCurrentCatalogOrDefault(conf));
+
+ Catalog catalog = getCatalog(catalogName);
+ if (catalog == null) {
throw new SemanticException(ErrorMsg.CATALOG_NOT_EXISTS, catalogName);
}
+ if (!DDLUtils.doesCatalogSupportCreateDB(catalog)) {
+ throw new SemanticException("Catalog %s does not support creating
database".formatted(catalogName));
Review Comment:
The error message uses String.formatted() which was introduced in Java 15,
but Hive typically supports earlier Java versions. This could cause
compatibility issues. Consider using String.format() instead for broader Java
version compatibility.
##########
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())
+ .orElse(MetastoreConf.getVar(context.getConf(),
+ MetastoreConf.ConfVars.WAREHOUSE_CATALOG) + "/" +
desc.getName());
Review Comment:
The path construction on line 43 uses string concatenation (`+ "/" +`)
instead of proper Path construction. This is the same issue as in
CreateDatabaseOperation - it doesn't handle cases where the warehouse catalog
directory might already end with a separator, potentially creating paths with
double slashes. Use `new Path(...)` constructor for proper path handling.
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -922,6 +922,15 @@ public static enum ConfVars {
"Default location for external tables created in the warehouse. " +
"If not set or null, then the normal warehouse location will be used
as the default location."),
+ METASTORE_CATALOG_WAREHOUSE("hive.metastore.warehouse.catalog.dir",
"/user/hive/catalog/warehouse",
+ "location of default database for the warehouse. If the name of
the catalog to which the db belongs is testcat," +
+ "then the default path prefix for the db is
/user/hive/catalog/warehouse/testcat"),
Review Comment:
The description for `METASTORE_CATALOG_WAREHOUSE` on lines 926-927 has the
same missing space after the comma issue as in MetastoreConf. It should read
"...is testcat, then the default..." instead of "...is testcat,then the
default..."
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -156,33 +170,67 @@ public Path getDnsPath(Path path) throws MetaException {
* This involves opening the FileSystem corresponding to the warehouse root
* dir (but that should be ok given that this is only called during DDL
* statements for non-external tables).
+ *
+ * @deprecated use {@link #getWhRoot(String)}
*/
public Path getWhRoot() throws MetaException {
- if (whRoot != null) {
- return whRoot;
+ return getWhRoot(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRoot(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName.trim());
+
+ Path rootDir = isDefault ? whRoot : whCatRoot;
+ if (rootDir == null) {
+ rootDir = getDnsPath(new Path(isDefault ? whRootString :
whCatRootString));
+ if (isDefault) {
+ whRoot = rootDir;
+ } else {
+ whCatRoot = rootDir;
+ }
}
- whRoot = getDnsPath(new Path(whRootString));
- return whRoot;
+
+ return isDefault ? rootDir : new Path(rootDir, catalogName);
}
+
+
+ /**
+ * @deprecated use {@link #getWhRootExternal(String)}
+ */
public Path getWhRootExternal() throws MetaException {
- if (whRootExternal != null) {
- return whRootExternal;
+ return getWhRootExternal(DEFAULT_CATALOG_NAME);
+ }
+
+ public Path getWhRootExternal(String catalogName) throws MetaException {
+ boolean isDefault = DEFAULT_CATALOG_NAME.equals(catalogName);
+
+ Path rootExDir = isDefault ? whRootExternal : whCatRootExternal;
+ if (rootExDir != null) {
+ return isDefault ? rootExDir : new Path(rootExDir, catalogName);
}
- if (!hasExternalWarehouseRoot()) {
- whRootExternal = getWhRoot();
- } else {
- whRootExternal = getDnsPath(new Path(whRootExternalString));
+
+ if (isDefault) {
+ rootExDir = hasExternalWarehouseRoot()
+ ? getDnsPath(new Path(whRootExternalString))
+ : getWhRoot(catalogName);
+ whRootExternal = rootExDir;
+ return rootExDir;
}
- return whRootExternal;
+
+ rootExDir = hasExternalWarehouseRoot(catalogName)
+ ? getDnsPath(new Path(whCatRootExternalString, catalogName))
+ : getWhRoot(catalogName);
+ whCatRootExternal = rootExDir;
+ return rootExDir;
Review Comment:
The same caching issue exists in `getWhRootExternal()`. When the cached
value exists and is non-null (line 209), the method returns `new
Path(rootExDir, catalogName)` for non-default catalogs. However, the cached
value `whCatRootExternal` is set on line 224 to already include the catalog
name (from line 222: `new Path(whCatRootExternalString, catalogName)`). This
means subsequent calls will incorrectly append the catalog name again.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLUtils.java:
##########
@@ -292,4 +295,11 @@ public static Pair<String, String>
getCatDbNamePair(ASTNode dbNameNode) throws S
return Pair.of(catName, dbName);
}
+
+ public static boolean doesCatalogSupportCreateDB(Catalog catalog) {
+ if (catalog.getName().equals(Warehouse.DEFAULT_CATALOG_NAME)) {
+ return true;
+ }
+ return
CatalogUtil.CatalogType.NATIVE.toString().toLowerCase().equals(catalog.getParameters().get("type"));
Review Comment:
The method `doesCatalogSupportCreateDB` could throw a NullPointerException
if `catalog.getParameters()` returns null or if the catalog type is stored with
different casing. The code should handle the case where parameters might be
null and should perform a case-insensitive comparison since catalog type
validation in `CatalogUtil.isValidCatalogType` uses
`EnumUtils.getEnumIgnoreCase`. Add null-safety checks for
`catalog.getParameters()` and use case-insensitive comparison.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -279,27 +326,59 @@ 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 {
+ Database db = new Database();
+ db.setName(dbName);
+ db.setCatalogName(DEFAULT_CATALOG_NAME);
+ return getDefaultDatabasePath(db, inExternalWH);
+ }
+
+ public Path getDefaultDatabasePath(Database db, boolean inExternalWH) throws
MetaException {
+ String catalogName = db.getCatalogName();
+ String dbName = db.getName();
if (inExternalWH) {
if (dbName.equalsIgnoreCase(DEFAULT_DATABASE_NAME)) {
- return getWhRootExternal();
+ return getWhRootExternal(catalogName);
}
- return new Path(getWhRootExternal(), dbName.toLowerCase() +
DATABASE_WAREHOUSE_SUFFIX);
+ return new Path(getWhRootExternal(catalogName), dbName.toLowerCase() +
DATABASE_WAREHOUSE_SUFFIX);
} else {
if (dbName.equalsIgnoreCase(DEFAULT_DATABASE_NAME)) {
- return getWhRoot();
+ return getWhRoot(catalogName);
}
- return new Path(getWhRoot(), dbName.toLowerCase() +
DATABASE_WAREHOUSE_SUFFIX);
+ return new Path(getWhRoot(catalogName), dbName.toLowerCase() +
DATABASE_WAREHOUSE_SUFFIX);
}
}
Review Comment:
The new method `getDefaultDatabasePath(Database db, boolean inExternalWH)`
lacks proper JavaDoc documentation. It should have JavaDoc comments explaining
its purpose, parameters, return values, and exceptions, following the pattern
of other public methods in this class.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -279,27 +326,59 @@ 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);
+ }
Review Comment:
The new methods `getDefaultDatabasePath(Database db)` and
`getDefaultExternalDatabasePath(Database db)` lack proper JavaDoc
documentation. They should have JavaDoc comments explaining their purpose,
parameters, return values, and exceptions, following the pattern of other
public methods in this class.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1783,6 +1783,15 @@ public enum ConfVars {
"hive.metastore.warehouse.external.dir", "",
"Default location for external tables created in the warehouse. " +
"If not set or null, then the normal warehouse location will be used
as the default location."),
+ WAREHOUSE_CATALOG("metastore.warehouse.catalog.dir",
+ "hive.metastore.warehouse.catalog.dir",
"/user/hive/catalog/warehouse",
+ "location of default database for the warehouse. If the name of
the catalog to which the db belongs is testcat," +
+ "then the default path prefix for the db is
/user/hive/catalog/warehouse/testcat."),
Review Comment:
The description for `WAREHOUSE_CATALOG` on lines 1788-1789 has a missing
space after the comma. It should read "...is testcat, then the default..."
instead of "...is testcat,then the default..."
##########
ql/src/java/org/apache/hadoop/hive/ql/queryhistory/repository/AbstractRepository.java:
##########
@@ -82,7 +82,7 @@ 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);
+ String location = getDatabaseLocation(db);
Review Comment:
The variable `db` is being used before it's initialized. On line 82, `db` is
retrieved from `hive.getDatabase()`, but on line 85, if `db` is null, the code
calls `getDatabaseLocation(db)` passing the null reference. This will cause a
NullPointerException when the method tries to call `db.getCatalogName()` inside
`getDefaultExternalDatabasePath(db)`. The database object should be created
before calling `getDatabaseLocation()`.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/CatalogUtil.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;
+
+import org.apache.commons.lang3.EnumUtils;
+
+import java.util.EnumSet;
+import java.util.Set;
+
+public class CatalogUtil {
+ public enum CatalogType {
+ NATIVE,
+ ICEBERG
+ }
+
+ public static final Set<CatalogType> VALID_CATALOG_TYPES =
EnumSet.of(CatalogType.NATIVE, CatalogType.ICEBERG);
+
+ public static boolean isValidCatalogType(String name) {
+ CatalogType type = EnumUtils.getEnumIgnoreCase(CatalogType.class,
name);
+ return type != null && VALID_CATALOG_TYPES.contains(type);
+ }
+}
Review Comment:
The class `CatalogUtil` and its public members lack JavaDoc documentation.
Public classes and methods should have JavaDoc comments explaining their
purpose, especially since this is a new API being introduced. The `CatalogType`
enum should also document what each type represents and when to use it.
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -922,6 +922,15 @@ public static enum ConfVars {
"Default location for external tables created in the warehouse. " +
"If not set or null, then the normal warehouse location will be used
as the default location."),
+ METASTORE_CATALOG_WAREHOUSE("hive.metastore.warehouse.catalog.dir",
"/user/hive/catalog/warehouse",
+ "location of default database for the warehouse. If the name of
the catalog to which the db belongs is testcat," +
+ "then the default path prefix for the db is
/user/hive/catalog/warehouse/testcat"),
+
+
HIVE_METASTORE_CATALOG_WAREHOUSE_EXTERNAL("hive.metastore.warehouse.catalog.external.dir",
null,
+ "Default location for external tables created in the warehouse. " +
+ "If not set or null, then the normal warehouse
location(MetastoreConf.METASTORE_CATALOG_WAREHOUSE)" +
+ "will be used as the default location."),
Review Comment:
Missing space before the opening parenthesis. The text should read
"...warehouse location (MetastoreConf.METASTORE_CATALOG_WAREHOUSE)" instead of
"...warehouse location(MetastoreConf.METASTORE_CATALOG_WAREHOUSE)".
--
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]
