[UT] optimize error message when creating hive/iceberg db/table #45612
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| throw new StarRocksConnectorException("Failed checking path: " + path); | ||
| } | ||
| } | ||
|
|
||
| public static void createDirectory(Path path, Configuration conf) { | ||
| try { | ||
| FileSystem fileSystem = FileSystem.get(path.toUri(), conf); |
There was a problem hiding this comment.
The most risky bug in this code is:
the removal of the check for LOCATION_PROPERTY in the checkLocationProperties method.
You can modify the code like this:
public static void checkLocationProperties(Map<String, String> properties) throws DdlException {
// Add back the check for both EXTERNAL_LOCATION_PROPERTY and LOCATION_PROPERTY
if (properties.containsKey(EXTERNAL_LOCATION_PROPERTY) || properties.containsKey(LOCATION_PROPERTY)) {
throw new DdlException("Can't create non-managed Hive table. " +
"Only supports creating hive table under Database location. " +
"You could execute command without location properties");
}
}This modification considers scenarios where either EXTERNAL_LOCATION_PROPERTY or the previously checked LOCATION_PROPERTY are included in the properties map, ensuring that the code handles both situations appropriately and prevents the creation of non-managed Hive tables with inappropriate location properties.
| throw new StarRocksConnectorException("Failed to find location in database '%s'. Please define the location" + | ||
| " when you create table or recreate another database with location." + | ||
| " You could execute the SQL command like 'CREATE TABLE <table_name> <columns> " + | ||
| "PROPERTIES('location' = '<location>')", dbName); | ||
| } | ||
|
|
||
| String dbLocation = database.getLocation(); |
There was a problem hiding this comment.
The most risky bug in this code is:
Database location validation is removed for GLUE metastore.
You can modify the code like this:
@@ -104,12 +103,6 @@ public void createDb(String dbName, Map<String, String> properties) {
}
}
+ if ((dbLocation == null || dbLocation.isEmpty()) && metastoreType == MetastoreType.GLUE) {
+ throw new StarRocksConnectorException("The database location must be set when using glue. " +
+ "you could execute command like " +
+ "'CREATE DATABASE <db_name> properties('location'='s3://<bucket>/<your_db_path>')'");
+ }
+
metastore.createDb(dbName, properties);
}Reintroducing the check for a database location ensures that when using the GLUE metastore, the database location must be explicitly defined. This is important because the absence of this validation might lead to an inconsistency in the way databases are created between different metastores and potentially cause runtime failures or misconfigurations when relying on GLUE’s behavior.
| "PROPERTIES('location' = '<location>')", dbName); | ||
| } | ||
| } | ||
|
|
||
| Table nativeTable = delegate.buildTable(TableIdentifier.of(dbName, tableName), schema) | ||
| .withLocation(location) | ||
| .withPartitionSpec(partitionSpec) |
There was a problem hiding this comment.
The most risky bug in this code is:
location variable not updated with dbLocation after fetching it when location is null or empty
You can modify the code like this:
if (Strings.isNullOrEmpty(location)) {
String dbLocation = getDB(dbName).getLocation();
if (Strings.isNullOrEmpty(dbLocation)) {
throw new StarRocksConnectorException("Failed to find location in database '%s'. Please define the location" +
" when you create table or recreate another database with location." +
" You could execute the SQL command like 'CREATE TABLE <table_name> <columns> " +
"PROPERTIES('location' = '<location>')", dbName);
}
location = dbLocation; // Update location with dbLocation
}
stephen-shelby
force-pushed
the
optimize_create_table_log
branch
from
0ad606b
to
215906e
Compare
stephen-shelby
changed the title
Signed-off-by: stephen <stephen5217@163.com>
stephen-shelby
force-pushed
the
optimize_create_table_log
branch
from
215906e
to
9023fa5
Compare
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 7 / 30 (23.33%) file detail
|
|
@Mergifyio backport branch-3.3 |
|
@Mergifyio backport branch-3.2 |
✅ Backports have been created
|
✅ Backports have been created
|
Signed-off-by: stephen <stephen5217@163.com> (cherry picked from commit 157a37a)
Signed-off-by: stephen <stephen5217@163.com> (cherry picked from commit 157a37a)
…Rocks#45612) Signed-off-by: stephen <stephen5217@163.com>
Why I'm doing:
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: