Copilot commented on code in PR #9201:
URL: https://github.com/apache/gravitino/pull/9201#discussion_r2554529899
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -191,6 +193,45 @@ public Response deregisterTable(
}
}
+ @POST
+ @Path("/exists")
+ @Timed(name = "table-exists." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
+ @ResponseMetered(name = "table-exists", absolute = true)
+ public Response tableExists(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ TableExistsRequest tableExistsRequest) {
+ try {
+ // True if exists, false no found and other errors
+ boolean exists = lanceNamespace.asTableOps().tableExists(tableId,
delimiter);
+ if (exists) {
+ return Response.status(Response.Status.OK).build();
+ }
+ return Response.status(Response.Status.NOT_FOUND).build();
Review Comment:
There's a logical inconsistency in the `tableExists` endpoint. When
`tableExists` returns `false` (table doesn't exist), the endpoint returns a
`NOT_FOUND` (404) status. However, the integration test expects a
`LanceNamespaceException` with a 404 code to be thrown (line 698-702 in
LanceRESTServiceIT.java), not just a 404 status response. This creates a
mismatch between the implementation behavior and the test expectations.
Consider either:
1. Throwing a `LanceNamespaceException` from the implementation when the
table doesn't exist, or
2. Updating the integration test to expect a 404 response status instead of
an exception.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -191,6 +193,45 @@ public Response deregisterTable(
}
}
+ @POST
+ @Path("/exists")
+ @Timed(name = "table-exists." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
+ @ResponseMetered(name = "table-exists", absolute = true)
+ public Response tableExists(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ TableExistsRequest tableExistsRequest) {
+ try {
+ // True if exists, false no found and other errors
+ boolean exists = lanceNamespace.asTableOps().tableExists(tableId,
delimiter);
+ if (exists) {
+ return Response.status(Response.Status.OK).build();
+ }
+ return Response.status(Response.Status.NOT_FOUND).build();
+ } catch (Exception e) {
+ return LanceExceptionMapper.toRESTResponse(tableId, e);
+ }
+ }
+
+ @POST
+ @Path("/drop")
+ @Timed(name = "drop-table." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
+ @ResponseMetered(name = "drop-table", absolute = true)
+ public Response dropTable(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ DeregisterTableRequest deregisterTableRequest) {
Review Comment:
The parameter type should be `DropTableRequest` instead of
`DeregisterTableRequest`. The parameter name and the actual operation don't
match - this endpoint drops a table, not deregisters it.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -191,6 +193,45 @@ public Response deregisterTable(
}
}
+ @POST
+ @Path("/exists")
+ @Timed(name = "table-exists." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
+ @ResponseMetered(name = "table-exists", absolute = true)
+ public Response tableExists(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ TableExistsRequest tableExistsRequest) {
+ try {
+ // True if exists, false no found and other errors
+ boolean exists = lanceNamespace.asTableOps().tableExists(tableId,
delimiter);
+ if (exists) {
+ return Response.status(Response.Status.OK).build();
+ }
+ return Response.status(Response.Status.NOT_FOUND).build();
+ } catch (Exception e) {
+ return LanceExceptionMapper.toRESTResponse(tableId, e);
+ }
+ }
+
+ @POST
+ @Path("/drop")
+ @Timed(name = "drop-table." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
+ @ResponseMetered(name = "drop-table", absolute = true)
+ public Response dropTable(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ DeregisterTableRequest deregisterTableRequest) {
Review Comment:
The `deregisterTableRequest` parameter is not used in the method
implementation. The dropTable operation only requires the `tableId` and
`delimiter` from the path/query parameters. This parameter should likely be a
`DropTableRequest` type (not `DeregisterTableRequest`), but either way it's
currently unused. Consider removing it or validating it if needed.
```suggestion
@Context HttpHeaders headers) {
```
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -191,6 +193,45 @@ public Response deregisterTable(
}
}
+ @POST
+ @Path("/exists")
+ @Timed(name = "table-exists." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
+ @ResponseMetered(name = "table-exists", absolute = true)
+ public Response tableExists(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ TableExistsRequest tableExistsRequest) {
Review Comment:
The `tableExistsRequest` parameter is not used in the method implementation.
The tableExists operation only requires the `tableId` and `delimiter` from the
path/query parameters. Consider removing this unused parameter or validating it
if it's intended to be used in the future.
```suggestion
@Context HttpHeaders headers) {
```
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -191,6 +193,45 @@ public Response deregisterTable(
}
}
+ @POST
+ @Path("/exists")
+ @Timed(name = "table-exists." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
+ @ResponseMetered(name = "table-exists", absolute = true)
+ public Response tableExists(
+ @PathParam("id") String tableId,
+ @QueryParam("delimiter") @DefaultValue("$") String delimiter,
+ @Context HttpHeaders headers,
+ TableExistsRequest tableExistsRequest) {
+ try {
+ // True if exists, false no found and other errors
Review Comment:
Typo in comment: "no found" should be "not found".
```suggestion
// True if exists, false if not found and other errors
```
--
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]
