yuqi1129 commented on code in PR #13787:
URL: https://github.com/apache/gravitino/pull/13787#discussion_r4255530667
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -212,6 +229,8 @@ public void createSchema(GravitinoSchema schema) {
} catch (TableAlreadyExistsException e) {
throw new TrinoException(
GravitinoErrorCode.GRAVITINO_SCHEMA_ALREADY_EXISTS, "Schema already
exists", e);
+ } catch (RuntimeException e) {
+ throw GravitinoErrorCode.toTrinoException(e);
Review Comment:
[Important] `createSchema` still reports expected failures as
`GENERIC_INTERNAL_ERROR` because the preceding catches use the wrong exception
types.
`SupportsSchemas.createSchema` declares `NoSuchCatalogException` and
`SchemaAlreadyExistsException`, and `SchemaErrorHandler` produces those types
for missing catalogs and duplicate schemas. The current catches use
`NoSuchSchemaException` and `TableAlreadyExistsException`, respectively, so
neither handles the actual failure; both reach this fallback.
Please replace the two catch types with `NoSuchCatalogException` and
`SchemaAlreadyExistsException`, retaining the existing
`GRAVITINO_CATALOG_NOT_EXISTS` and `GRAVITINO_SCHEMA_ALREADY_EXISTS` mappings,
and add regression tests for both cases.
I independently verified this on head `469634589`: all 407 module non-Docker
tests passed (one skipped), but two temporary tests exercising these failures
both failed because the returned code was `GENERIC_INTERNAL_ERROR`. The
temporary tests and mutation checks were reverted after verification. This
confirms the earlier review finding remains unresolved.
--
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]
