Copilot commented on code in PR #9201:
URL: https://github.com/apache/gravitino/pull/9201#discussion_r2563492581
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -192,6 +195,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 if not found and, otherwise throw exception
+ 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,
+ DropTableRequest dropTableRequest) {
+ try {
+
Review Comment:
There's an extra blank line after the `try {` statement. This should be
removed for consistency with the codebase style seen in other methods.
```suggestion
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -266,6 +267,56 @@ public DeregisterTableResponse deregisterTable(String
tableId, String delimiter)
return response;
}
+ @Override
+ public boolean tableExists(String tableId, String delimiter) {
+ ObjectIdentifier nsId = ObjectIdentifier.of(tableId,
Pattern.quote(delimiter));
+ Preconditions.checkArgument(
+ nsId.levels() == 3, "Expected at 3-level namespace but got: %s",
nsId.levels());
+
+ String catalogName = nsId.levelAtListPos(0);
+ Catalog catalog =
namespaceWrapper.loadAndValidateLakehouseCatalog(catalogName);
+
+ NameIdentifier tableIdentifier =
+ NameIdentifier.of(nsId.levelAtListPos(1), nsId.levelAtListPos(2));
+
+ return catalog.asTableCatalog().tableExists(tableIdentifier);
Review Comment:
The `tableExists` implementation has an inconsistency with how the REST
endpoint and integration tests expect it to behave. The method returns a
boolean (true/false), but:
1. The REST endpoint (line 209-213 in LanceTableOperations.java) converts
`false` to a 404 NOT_FOUND response
2. The integration test (line 716-720 in LanceRESTServiceIT.java) expects a
`LanceNamespaceException` to be thrown for non-existing tables
The implementation should throw a `LanceNamespaceException` when the table
doesn't exist (similar to `namespaceExists` pattern), rather than returning
false. For example:
```java
boolean exists = catalog.asTableCatalog().tableExists(tableIdentifier);
if (!exists) {
throw LanceNamespaceException.notFound(
"Table not found: " + tableId,
NoSuchTableException.class.getSimpleName(),
tableId,
CommonUtil.formatCurrentStackTrace());
}
```
```suggestion
boolean exists = catalog.asTableCatalog().tableExists(tableIdentifier);
if (!exists) {
throw LanceNamespaceException.notFound(
"Table not found: " + tableId,
NoSuchTableException.class.getSimpleName(),
tableId,
CommonUtil.formatCurrentStackTrace());
}
return true;
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/LanceTableOperations.java:
##########
@@ -94,4 +95,22 @@ RegisterTableResponse registerTable(
* @return the response of the deregister table operation
*/
DeregisterTableResponse deregisterTable(String tableId, String delimiter);
+
+ /**
+ * Check if a table exists.
+ *
+ * @param tableId table ids are in the format of
"{namespace}{delimiter}{table_name}"
+ * @param delimiter the delimiter used in the namespace
+ * @return true if the table exists, false otherwise
+ */
+ boolean tableExists(String tableId, String delimiter);
Review Comment:
The return type `boolean` is inconsistent with the expected behavior. Based
on the integration test expectations (line 716-720 in LanceRESTServiceIT.java)
and the pattern used by `namespaceExists`, this method should return `void` and
throw a `LanceNamespaceException` when the table doesn't exist, rather than
returning a boolean value.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -192,6 +195,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 if not found and, otherwise throw exception
Review Comment:
The comment contains a grammatical error. Change "false if not found and,
otherwise throw exception" to "false if not found, otherwise throws exception".
```suggestion
// True if exists, false if not found, otherwise throws exception
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -266,6 +267,56 @@ public DeregisterTableResponse deregisterTable(String
tableId, String delimiter)
return response;
}
+ @Override
+ public boolean tableExists(String tableId, String delimiter) {
+ ObjectIdentifier nsId = ObjectIdentifier.of(tableId,
Pattern.quote(delimiter));
+ Preconditions.checkArgument(
+ nsId.levels() == 3, "Expected at 3-level namespace but got: %s",
nsId.levels());
+
+ String catalogName = nsId.levelAtListPos(0);
+ Catalog catalog =
namespaceWrapper.loadAndValidateLakehouseCatalog(catalogName);
+
+ NameIdentifier tableIdentifier =
+ NameIdentifier.of(nsId.levelAtListPos(1), nsId.levelAtListPos(2));
+
+ return catalog.asTableCatalog().tableExists(tableIdentifier);
+ }
+
+ @Override
+ public DropTableResponse dropTable(String tableId, String delimiter) {
+ ObjectIdentifier nsId = ObjectIdentifier.of(tableId,
Pattern.quote(delimiter));
+ Preconditions.checkArgument(
+ nsId.levels() == 3, "Expected at 3-level namespace but got: %s",
nsId.levels());
+
+ String catalogName = nsId.levelAtListPos(0);
+ Catalog catalog =
namespaceWrapper.loadAndValidateLakehouseCatalog(catalogName);
+
+ NameIdentifier tableIdentifier =
+ NameIdentifier.of(nsId.levelAtListPos(1), nsId.levelAtListPos(2));
+
+ Table table;
+ try {
+ table = catalog.asTableCatalog().loadTable(tableIdentifier);
+ } catch (NoSuchTableException e) {
+ throw LanceNamespaceException.notFound(
+ "Table not found: " + tableId,
+ NoSuchTableException.class.getSimpleName(),
+ tableId,
+ CommonUtil.formatCurrentStackTrace());
+ }
+
+ catalog.asTableCatalog().purgeTable(tableIdentifier);
Review Comment:
The `purgeTable` method typically returns a boolean indicating success or
failure, but the return value is not being checked. If `purgeTable` returns
`false` instead of throwing an exception, the table won't be deleted but the
method will still return a successful response.
Consider checking the return value and throwing an appropriate exception if
the operation fails:
```java
boolean deleted = catalog.asTableCatalog().purgeTable(tableIdentifier);
if (!deleted) {
throw LanceNamespaceException.internalServerError(
"Failed to delete table: " + tableId,
"InternalServerError",
tableId,
CommonUtil.formatCurrentStackTrace());
}
```
```suggestion
boolean deleted = catalog.asTableCatalog().purgeTable(tableIdentifier);
if (!deleted) {
throw LanceNamespaceException.internalServerError(
"Failed to delete table: " + tableId,
"InternalServerError",
tableId,
CommonUtil.formatCurrentStackTrace());
}
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -665,4 +668,108 @@ void testDescribeTable() {
Assertions.assertEquals("Runtime exception", errorResp.getError());
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
}
+
+ @Test
+ void testTableExists() {
+ String tableIds = "catalog.scheme.table_exists";
+ String delimiter = ".";
+
+ doReturn(true).when(tableOps).tableExists(any(), any());
+
+ Response resp =
+ target(String.format("/v1/table/%s/exists", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(null);
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+
+ // test throw exception
+ doThrow(
+ LanceNamespaceException.notFound(
+ "Table not found", "NoSuchTableException", tableIds, ""))
+ .when(tableOps)
+ .tableExists(any(), any());
+ resp =
+ target(String.format("/v1/table/%s/exists", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(null);
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(404, errorResp.getCode());
+ Assertions.assertEquals("Table not found", errorResp.getError());
+ Assertions.assertEquals("NoSuchTableException", errorResp.getType());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ doThrow(new RuntimeException("Runtime
exception")).when(tableOps).tableExists(any(), any());
+ resp =
+ target(String.format("/v1/table/%s/exists", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(null);
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ }
Review Comment:
The unit test is missing coverage for the case where `tableExists` returns
`false`. The test only covers:
1. When it returns `true` (line 677)
2. When it throws a `LanceNamespaceException` (line 688-692)
3. When it throws a `RuntimeException` (line 709)
But the REST endpoint has logic to handle `false` by returning 404 (lines
212-213 in LanceTableOperations.java). This test case should be added for
completeness, or the false-handling code should be removed if the method is
expected to throw exceptions instead of returning false.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -192,6 +195,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 if not found and, otherwise throw exception
+ 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,
+ DropTableRequest dropTableRequest) {
Review Comment:
The `dropTable` method accepts a `DropTableRequest` parameter but never uses
it. The table ID is taken from the path parameter instead. Consider either:
1. Removing the unused parameter if it's not needed by the API contract
2. Adding a validation method similar to `validateDeregisterTableRequest` to
acknowledge the parameter is intentionally ignored
This is inconsistent with the pattern used in other endpoints like
`deregisterTable` which calls `validateDeregisterTableRequest`.
--
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]
