lasdf1234 commented on code in PR #14556:
URL: https://github.com/apache/gravitino/pull/14556#discussion_r4453134220
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java:
##########
@@ -95,6 +102,52 @@ public Response toResponse(Throwable ex) {
return toRESTResponse("", ex);
}
+ private static Response handleWebApplicationException(
+ String instance, WebApplicationException exception) {
+ int status = exception.getResponse().getStatus();
+ String message = exception.getMessage();
+ if (exception instanceof ParamException parameterException
+ && !(exception instanceof ParamException.PathParamException)) {
Review Comment:
**[P2]** Query `ParamException` is remapped to 400, but `PathParamException`
is excluded and stays on Jersey's 404 path, which then becomes
`NamespaceNotFoundException` (code 1) below.
A malformed path id is invalid input, not a missing namespace. Iceberg maps
all `ParamException` to 400. Please treat path conversion failures as
`InvalidInputException` / 400 as well, or document why path params should keep
namespace-not-found semantics.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java:
##########
@@ -95,6 +102,52 @@ public Response toResponse(Throwable ex) {
return toRESTResponse("", ex);
}
+ private static Response handleWebApplicationException(
+ String instance, WebApplicationException exception) {
+ int status = exception.getResponse().getStatus();
+ String message = exception.getMessage();
+ if (exception instanceof ParamException parameterException
+ && !(exception instanceof ParamException.PathParamException)) {
+ // Jersey uses 404 for query conversion failures; the route exists, but
the input is invalid.
+ status = Response.Status.BAD_REQUEST.getStatusCode();
+ message = "Invalid value for parameter '" +
parameterException.getParameterName() + "'";
+ }
+ LanceNamespaceException error;
+ if (status >= 400 && status < 500) {
+ error =
+ switch (status) {
+ case 401 -> new UnauthenticatedException(message, "", instance);
+ case 403 -> new PermissionDeniedException(message, "", instance);
+ case 404 -> new NamespaceNotFoundException(message, "", instance);
Review Comment:
**[P2]** Every framework HTTP 404 becomes `NamespaceNotFoundException`. That
is reasonable for a missing namespace route, but it also covers unmatched table
routes / path-param failures (see above), so Lance clients that key off body
`code` will read **namespace not found (1)** for non-namespace misses.
Consider branching on the request path (or exception subtype) so table/route
404s map to `TableNotFoundException` / `InvalidInputException` when appropriate.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java:
##########
@@ -95,6 +102,52 @@ public Response toResponse(Throwable ex) {
return toRESTResponse("", ex);
}
+ private static Response handleWebApplicationException(
+ String instance, WebApplicationException exception) {
+ int status = exception.getResponse().getStatus();
+ String message = exception.getMessage();
+ if (exception instanceof ParamException parameterException
+ && !(exception instanceof ParamException.PathParamException)) {
+ // Jersey uses 404 for query conversion failures; the route exists, but
the input is invalid.
+ status = Response.Status.BAD_REQUEST.getStatusCode();
+ message = "Invalid value for parameter '" +
parameterException.getParameterName() + "'";
+ }
+ LanceNamespaceException error;
+ if (status >= 400 && status < 500) {
+ error =
+ switch (status) {
+ case 401 -> new UnauthenticatedException(message, "", instance);
+ case 403 -> new PermissionDeniedException(message, "", instance);
+ case 404 -> new NamespaceNotFoundException(message, "", instance);
+ case 405, 406, 415 -> new
org.lance.namespace.errors.UnsupportedOperationException(
+ message, "", instance);
+ case 409 -> new ConcurrentModificationException(message, "",
instance);
+ case 429 -> new ThrottlingException(message, "", instance);
Review Comment:
**[P3]** Framework 429 correctly becomes `ThrottlingException` here (and
HTTP status is kept in this method). `toHttpStatus` still has no
`ThrottlingException` branch, so a backend-thrown `ThrottlingException` would
still become HTTP 500.
Please add `ThrottlingException` → 429 in `toHttpStatus` for symmetry.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -913,6 +917,27 @@ long handleLanceTableChange(Table table, TableChange[]
changes) {
}
}
+ private static void validateLanceColumnNames(Dataset dataset, TableChange[]
changes) {
+ // Use the dataset schema, including out-of-band changes, and validate the
whole ordered
+ // request before applying any changes. A late name conflict must not
leave earlier edits.
+ Set<String> columnNames =
+
dataset.getSchema().getFields().stream().map(Field::getName).collect(Collectors.toSet());
+ for (TableChange change : changes) {
+ if (change instanceof TableChange.AddColumn addColumn) {
+ String name = addColumn.fieldName()[0];
+ Preconditions.checkArgument(columnNames.add(name), "Column already
exists: %s", name);
+ } else if (change instanceof TableChange.DeleteColumn deleteColumn) {
+ columnNames.remove(deleteColumn.fieldName()[0]);
Review Comment:
**[P3]** `DeleteColumn` only `remove`s from the working set and does not
require the column to exist, while `RenameColumn` fails on a missing source.
That makes whole-batch prevalidation asymmetric: a delete of a missing name
silently no-ops in validation, then `dropColumns` may fail mid-apply after
earlier changes already ran — the failure mode this helper is meant to avoid.
Prefer `Preconditions.checkArgument(columnNames.remove(name), "Column does
not exist: %s", name)` to match rename.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java:
##########
@@ -511,6 +511,11 @@ private Table renameTable(NameIdentifier tableIdent,
TableChange.RenameTable ren
backend.wrapper().renameTable(renameTableRequest);
return loadTable(
NameIdentifier.of(ArrayUtils.add(destNamespace.levels(),
renameTable.getNewName())));
+ } catch (AlreadyExistsException e) {
Review Comment:
**[P2]** Good catch wrapping Iceberg `AlreadyExistsException` as Gravitino
`TableAlreadyExistsException` so rename-onto-existing is a client 409 instead
of 500.
One edge: `destNamespace` is captured before `getNewSchemaName()` rewriting.
Worth a unit/IT that renames across schemas onto an existing target and asserts
the identifier in the exception message.
--
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]
