Copilot commented on code in PR #10637:
URL: https://github.com/apache/gravitino/pull/10637#discussion_r3032864896
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceNamespaceOperations.java:
##########
@@ -84,6 +100,19 @@ public Response listNamespaces(
public Response describeNamespace(
@PathParam("id") String namespaceId,
@DefaultValue(NAMESPACE_DELIMITER_DEFAULT) @QueryParam("delimiter")
String delimiter) {
+ return describeNamespaceInternal(namespaceId, delimiter);
+ }
+
+ @POST
+ @Path("/describe")
+ @Timed(name = "describe-namespaces-root." +
MetricNames.HTTP_PROCESS_DURATION, absolute = true)
+ @ResponseMetered(name = "describe-namespaces-root", absolute = true)
+ public Response describeNamespaceOnRoot(
+ @DefaultValue(NAMESPACE_DELIMITER_DEFAULT) @QueryParam("delimiter")
String delimiter) {
+ return describeNamespaceInternal(delimiter, delimiter);
+ }
Review Comment:
`describeNamespaceOnRoot` calls `describeNamespaceInternal(delimiter,
delimiter)`, which uses the delimiter string as a fake namespace id. This is
fragile and makes the error `instance` misleading. Prefer using an explicit
root id (e.g., empty string) and let downstream validation produce the expected
error for missing namespace id.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceNamespaceOperations.java:
##########
@@ -66,6 +66,22 @@ public Response listNamespaces(
@DefaultValue(NAMESPACE_DELIMITER_DEFAULT) @QueryParam("delimiter")
String delimiter,
@QueryParam("page_token") String pageToken,
@QueryParam("limit") Integer limit) {
+ return listNamespacesInternal(namespaceId, delimiter, pageToken, limit);
+ }
+
+ @GET
+ @Path("/list")
+ @Timed(name = "list-namespaces-root." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "list-namespaces-root", absolute = true)
+ public Response listNamespacesOnRoot(
+ @DefaultValue(NAMESPACE_DELIMITER_DEFAULT) @QueryParam("delimiter")
String delimiter,
+ @QueryParam("page_token") String pageToken,
+ @QueryParam("limit") Integer limit) {
+ return listNamespacesInternal(delimiter, delimiter, pageToken, limit);
+ }
Review Comment:
`listNamespacesOnRoot` passes the delimiter value as the `namespaceId`
placeholder (`listNamespacesInternal(delimiter, delimiter, ...)`). This couples
root-list behavior to the chosen delimiter and also produces confusing
`instance` values in error responses. Use an explicit root identifier (e.g.,
empty string) when listing from root, and keep `delimiter` only for splitting.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceNamespaceOperations.java:
##########
@@ -66,6 +66,22 @@ public Response listNamespaces(
@DefaultValue(NAMESPACE_DELIMITER_DEFAULT) @QueryParam("delimiter")
String delimiter,
@QueryParam("page_token") String pageToken,
@QueryParam("limit") Integer limit) {
+ return listNamespacesInternal(namespaceId, delimiter, pageToken, limit);
+ }
+
+ @GET
+ @Path("/list")
+ @Timed(name = "list-namespaces-root." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "list-namespaces-root", absolute = true)
+ public Response listNamespacesOnRoot(
+ @DefaultValue(NAMESPACE_DELIMITER_DEFAULT) @QueryParam("delimiter")
String delimiter,
+ @QueryParam("page_token") String pageToken,
+ @QueryParam("limit") Integer limit) {
+ return listNamespacesInternal(delimiter, delimiter, pageToken, limit);
Review Comment:
New endpoints `/v1/namespace/list` and `/v1/namespace/describe` were added,
but there are no unit tests covering these routes (existing tests only cover
`/{id}/list` and `/{id}/describe`). Please add Jersey tests exercising the root
routes (success path for `/list`, and expected error behavior for `/describe`)
to avoid regressions.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -337,13 +341,24 @@ private void
validateAlterColumnsRequest(AlterTableAlterColumnsRequest request)
Preconditions.checkArgument(
!request.getAlterations().isEmpty(), "Columns to alter cannot be
empty.");
- for (ColumnAlteration alteration : request.getAlterations()) {
+ for (AlterColumnsEntry alteration : request.getAlterations()) {
Preconditions.checkArgument(
- StringUtils.isBlank(alteration.getCastTo()),
- "Only RENAME alteration is supported currently.");
+ StringUtils.isNotBlank(alteration.getPath()), "Column path to alter
cannot be empty.");
Preconditions.checkArgument(
StringUtils.isNotBlank(alteration.getRename()),
- "Rename field must be specified when castTo is not provided.");
+ "Only RENAME alteration is supported currently.");
+ Preconditions.checkArgument(
+ alteration.getDataType() == null
+ && alteration.getNullable() == null
+ && alteration.getVirtualColumn() == null,
+ "Only RENAME alteration is supported currently.");
Review Comment:
In `validateAlterColumnsRequest`, the precondition that `rename` must be
non-blank throws the message "Only RENAME alteration is supported currently.",
which is misleading when the request is missing `rename`. Consider using an
error message that directly states `rename` is required, and keep the separate
validation that rejects non-rename fields.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -147,10 +152,9 @@ public Response createEmptyTable(
validateCreateEmptyTableRequest(request);
String tableLocation = request.getLocation();
- Map<String, String> props =
- request.getProperties() == null
- ? Maps.newHashMap()
- : Maps.newHashMap(request.getProperties());
+ MultivaluedMap<String, String> headersMap = headers.getRequestHeaders();
+ String tableProperties =
headersMap.getFirst(LANCE_TABLE_PROPERTIES_PREFIX_HEADER);
+ Map<String, String> props =
SerializationUtils.deserializeProperties(tableProperties);
CreateEmptyTableResponse response =
lanceNamespace.asTableOps().createEmptyTable(tableId, delimiter,
tableLocation, props);
Review Comment:
`createEmptyTable` no longer reads properties from the JSON request body and
instead only deserializes them from the `x-lance-table-properties` header. This
breaks backward compatibility with existing clients/docs that send `properties`
in the body, and it also makes the endpoint inconsistent with its declared JSON
request model. Preserve compatibility by accepting request-body properties when
present (and optionally merging with header-provided properties with a clear
precedence).
##########
docs/lance-rest-service.md:
##########
@@ -312,26 +312,26 @@ Map<String, String> props = new HashMap<>();
props.put(RestNamespaceConfig.URI, "http://localhost:9101/lance";);
props.put(RestNamespaceConfig.DELIMITER,
RestNamespaceConfig.DELIMITER_DEFAULT);
Review Comment:
The Java example still uses `RestNamespaceConfig.URI/DELIMITER` to populate
connection properties, but the rest of this PR (and the upgraded Lance SDK)
appears to have moved to string keys like `"uri"`/`"delimiter"` (see
integration test usage). Please update the example so it compiles and uses the
correct config keys/classes for `org.lance:lance-namespace-core:0.4.5`.
```suggestion
props.put("uri", "http://localhost:9101/lance";);
props.put("delimiter", ".");
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -903,8 +878,7 @@ void testAlterColumns() {
Mockito.reset(tableOps);
when(tableOps.alterTable(any(), any(),
any(AlterTableAlterColumnsRequest.class)))
.thenThrow(
- LanceNamespaceException.notFound(
- "Table not found", "NoSuchTableException", tableIds, ""));
+ new org.lance.namespace.errors.TableNotFoundException("Table not
found", "", tableIds));
resp =
Review Comment:
Avoid using a fully qualified class name inside the test method body. Import
`org.lance.namespace.errors.TableNotFoundException` and use the simple name to
match the project's Java import/style guidelines.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -749,9 +730,7 @@ void testDropTable() {
Assertions.assertEquals(dropTableResponse.getLocation(),
response.getLocation());
// test throw exception
- doThrow(
- LanceNamespaceException.notFound(
- "Table not found", "NoSuchTableException", tableIds, ""))
+ doThrow(new org.lance.namespace.errors.TableNotFoundException("Table not
found", "", tableIds))
.when(tableOps)
.dropTable(any(), any());
Review Comment:
Avoid using a fully qualified class name inside the test method body. Import
`org.lance.namespace.errors.TableNotFoundException` and use the simple name to
match the project's Java import/style guidelines.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/SerializationUtils.java:
##########
@@ -35,19 +35,19 @@ private SerializationUtils() {
// see
https://github.com/lancedb/lance-namespace/blob/2033b2fca126e87e56ba0d5ec19c5ec010c7a98f/
//
java/lance-namespace-core/src/main/java/com/lancedb/lance/namespace/rest/RestNamespace.java#L207-L208
public static Map<String, String> deserializeProperties(String
serializedProperties) {
- return StringUtils.isBlank(serializedProperties)
- ? ImmutableMap.of()
- : JsonUtil.parse(
- serializedProperties,
- jsonNode -> {
- Map<String, String> map = new HashMap<>();
- jsonNode
- .fields()
- .forEachRemaining(
- entry -> {
- map.put(entry.getKey(), entry.getValue().asText());
- });
- return map;
- });
+ if (StringUtils.isBlank(serializedProperties)) {
+ return new HashMap<>();
+ }
+
+ try {
+ Map<String, Object> rawProperties =
+ JsonUtils.anyFieldMapper()
+ .readValue(serializedProperties, new TypeReference<Map<String,
Object>>() {});
+ Map<String, String> deserializedProperties = new HashMap<>();
+ rawProperties.forEach((k, v) -> deserializedProperties.put(k,
String.valueOf(v)));
+ return deserializedProperties;
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Failed to deserialize table
properties JSON", e);
+ }
Review Comment:
`deserializeProperties` catches a generic `Exception`. Per project
guidelines, prefer catching the specific Jackson exceptions thrown by
`readValue` (e.g., `IOException` / `JsonProcessingException`) so that
unexpected runtime errors are not inadvertently wrapped as input errors.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -595,8 +583,7 @@ void testDeregisterTable() {
Mockito.reset(tableOps);
when(tableOps.deregisterTable(any(), any()))
.thenThrow(
- LanceNamespaceException.notFound(
- "Table not found", "NoSuchTableException", tableIds, ""));
+ new org.lance.namespace.errors.TableNotFoundException("Table not
found", "", tableIds));
resp =
Review Comment:
Avoid using a fully qualified class name inside the test method body. Import
`org.lance.namespace.errors.TableNotFoundException` and use the simple name to
match the project's Java import/style guidelines.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -850,8 +826,7 @@ void testDropColumns() {
Mockito.reset(tableOps);
when(tableOps.alterTable(any(), any(),
any(AlterTableDropColumnsRequest.class)))
.thenThrow(
- LanceNamespaceException.notFound(
- "Table not found", "NoSuchTableException", tableIds, ""));
+ new org.lance.namespace.errors.TableNotFoundException("Table not
found", "", tableIds));
resp =
Review Comment:
Avoid using a fully qualified class name inside the test method body. Import
`org.lance.namespace.errors.TableNotFoundException` and use the simple name to
match the project's Java import/style guidelines.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -644,14 +630,13 @@ void testDescribeTable() {
Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
DescribeTableResponse response =
resp.readEntity(DescribeTableResponse.class);
Assertions.assertEquals(createTableResponse.getLocation(),
response.getLocation());
- Assertions.assertEquals(createTableResponse.getProperties(),
response.getProperties());
+ Assertions.assertEquals(createTableResponse.getMetadata(),
response.getMetadata());
// Test not found exception
Mockito.reset(tableOps);
when(tableOps.describeTable(any(), any(), any()))
.thenThrow(
- LanceNamespaceException.notFound(
- "Table not found", "NoSuchTableException", tableIds, ""));
+ new org.lance.namespace.errors.TableNotFoundException("Table not
found", "", tableIds));
resp =
Review Comment:
Avoid using a fully qualified class name inside the test method body. Import
`org.lance.namespace.errors.TableNotFoundException` and use the simple name to
match the project's Java import/style guidelines.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -693,9 +677,7 @@ void testTableExists() {
Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
// test throw exception
- doThrow(
- LanceNamespaceException.notFound(
- "Table not found", "NoSuchTableException", tableIds, ""))
+ doThrow(new org.lance.namespace.errors.TableNotFoundException("Table not
found", "", tableIds))
.when(tableOps)
.tableExists(any(), any());
Review Comment:
Avoid using a fully qualified class name inside the test method body. Import
`org.lance.namespace.errors.TableNotFoundException` and use the simple name to
match the project's Java import/style guidelines.
--
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]
