Copilot commented on code in PR #9097:
URL: https://github.com/apache/gravitino/pull/9097#discussion_r2556362279
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java:
##########
@@ -554,7 +554,10 @@ public static TableEntity fromTableAndColumnPOs(
: JsonUtils.anyFieldMapper()
.readValue(tablePO.getPartitions(),
Partitioning[].class))
.withComment(tablePO.getComment())
- .withProperties(properties)
+ .withProperties(
+ StringUtils.isBlank(tablePO.getProperties())
+ ? null
+ :
JsonUtils.anyFieldMapper().readValue(tablePO.getProperties(), Map.class))
Review Comment:
Duplicate property parsing: Properties are parsed twice - once at lines
518-521 where the format is added to the map, and again at lines 557-560 where
a new map is created from the JSON. The second parsing overwrites the first,
causing the `Table.PROPERTY_TABLE_FORMAT` set at line 523 to be lost. This
change should combine both operations: parse the properties once, add the
format if present, and then set it on the builder.
```suggestion
.withProperties(properties)
```
##########
catalogs/catalog-generic-lakehouse/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceCatalogOperations.java:
##########
@@ -296,4 +273,104 @@ public boolean dropTable(NameIdentifier ident) {
throw new UnsupportedOperationException(
"LanceCatalogOperations does not support dropTable operation.");
}
+
+ private List<Index> addLanceIndex(String location, List<Index> addedIndexes)
{
+ List<Index> newIndexes = Lists.newArrayList();
+ try (Dataset dataset = Dataset.open(location, new RootAllocator())) {
+ for (Index index : addedIndexes) {
+ IndexType indexType = getIndexType(index);
+ IndexParams indexParams = generateIndexParams(index);
+ dataset.createIndex(
+ Arrays.stream(index.fieldNames())
+ .map(fieldPath -> String.join(".", fieldPath))
+ .collect(Collectors.toList()),
+ indexType,
+ Optional.ofNullable(index.name()),
+ indexParams,
+ false);
+
+ // Currently lance only supports single-field indexes, so we can use
the first field name
+ String lanceIndexName =
+ index.name() == null ? index.fieldNames()[0][0] + "_idx" :
index.name();
+ newIndexes.add(
+ Indexes.of(index.type(), lanceIndexName, index.fieldNames(),
index.properties()));
+ }
+
+ return newIndexes;
+ }
+ }
+
+ private IndexType getIndexType(Index index) {
+ IndexType indexType = IndexType.valueOf(index.type().name());
+ return switch (indexType) {
+ // API only supports these index types for now, but there are more
index types in Lance.
+ case SCALAR, BTREE, INVERTED, BITMAP -> indexType;
+ // According to real test, we need to map IVF_SQ/IVF_PQ/IVF_HNSW_SQ to
VECTOR type in Lance,
+ // or it will throw exception. For more, please refer to
+ // https://github.com/lancedb/lance/issues/5182#issuecomment-3524372490
+ case IVF_FLAT, IVF_PQ, IVF_HNSW_SQ -> IndexType.VECTOR;
Review Comment:
Missing cases in switch statement: The `getIndexType` method maps
`IVF_FLAT`, `IVF_PQ`, and `IVF_HNSW_SQ` to `VECTOR`, but doesn't handle
`IVF_SQ`, `IVF_HNSW_PQ`, or `FTS` which are defined in the `Index.IndexType`
enum. The comment mentions that `IVF_SQ` should be mapped to VECTOR (line 308),
but it's not included in the case statement. This will cause the default case
to throw an exception for these valid index types.
```suggestion
// According to real test, we need to map
IVF_SQ/IVF_PQ/IVF_HNSW_SQ/IVF_HNSW_PQ to VECTOR type in Lance,
// or it will throw exception. For more, please refer to
//
https://github.com/lancedb/lance/issues/5182#issuecomment-3524372490
case IVF_FLAT, IVF_PQ, IVF_HNSW_SQ, IVF_SQ, IVF_HNSW_PQ ->
IndexType.VECTOR;
case FTS -> indexType; // If FTS is not supported, you may want to
throw instead
```
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Index.java:
##########
@@ -108,9 +116,24 @@ enum IndexType {
IVF_SQ,
/** IVF_PQ (Inverted File with Product Quantization) is an indexing method
used for efficient */
IVF_PQ,
Review Comment:
Incomplete documentation: The comments for `IVF_FLAT` (line 113) and
`IVF_PQ` (line 117) are cut off mid-sentence. They should provide complete
descriptions similar to the detailed documentation for `IVF_HNSW_SQ` and
`IVF_HNSW_PQ`, explaining what these indexing methods are used for (e.g.,
"efficient approximate nearest neighbor search in high-dimensional vector
spaces").
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java:
##########
@@ -554,7 +554,10 @@ public static TableEntity fromTableAndColumnPOs(
: JsonUtils.anyFieldMapper()
.readValue(tablePO.getPartitions(),
Partitioning[].class))
.withComment(tablePO.getComment())
- .withProperties(properties)
+ .withProperties(
+ StringUtils.isBlank(tablePO.getProperties())
+ ? null
+ :
JsonUtils.anyFieldMapper().readValue(tablePO.getProperties(), Map.class))
.withColumns(fromColumnPOs(columnPOs))
Review Comment:
Duplicate method call: `.withColumns(fromColumnPOs(columnPOs))` is called
twice - once at line 530 and again at line 561. This is redundant and the
second call overwrites the first. One of these calls should be removed.
```suggestion
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceTableOperations.java:
##########
@@ -0,0 +1,531 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.lance.service.rest;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.common.collect.ImmutableMap;
+import com.lancedb.lance.namespace.LanceNamespaceException;
+import com.lancedb.lance.namespace.model.CreateEmptyTableRequest;
+import com.lancedb.lance.namespace.model.CreateEmptyTableResponse;
+import com.lancedb.lance.namespace.model.CreateTableIndexRequest;
+import com.lancedb.lance.namespace.model.CreateTableIndexResponse;
+import com.lancedb.lance.namespace.model.CreateTableResponse;
+import com.lancedb.lance.namespace.model.DeregisterTableRequest;
+import com.lancedb.lance.namespace.model.DeregisterTableResponse;
+import com.lancedb.lance.namespace.model.DescribeTableRequest;
+import com.lancedb.lance.namespace.model.DescribeTableResponse;
+import com.lancedb.lance.namespace.model.ErrorResponse;
+import com.lancedb.lance.namespace.model.IndexContent;
+import com.lancedb.lance.namespace.model.ListTableIndicesRequest;
+import com.lancedb.lance.namespace.model.ListTableIndicesResponse;
+import com.lancedb.lance.namespace.model.RegisterTableRequest;
+import com.lancedb.lance.namespace.model.RegisterTableResponse;
+import java.io.IOException;
+import java.util.List;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.client.Entity;
+import javax.ws.rs.core.Application;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import org.apache.gravitino.lance.common.ops.LanceTableOperations;
+import org.apache.gravitino.lance.common.ops.NamespaceWrapper;
+import org.apache.gravitino.rest.RESTUtils;
+import org.glassfish.jersey.internal.inject.AbstractBinder;
+import org.glassfish.jersey.server.ResourceConfig;
+import org.glassfish.jersey.test.JerseyTest;
+import org.glassfish.jersey.test.TestProperties;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+public class TestLanceTableOperations extends JerseyTest {
+
+ private static NamespaceWrapper namespaceWrapper =
mock(NamespaceWrapper.class);
+ private static org.apache.gravitino.lance.common.ops.LanceTableOperations
tableOps =
+ mock(LanceTableOperations.class);
+
+ @Override
+ protected Application configure() {
+ try {
+ forceSet(
+ TestProperties.CONTAINER_PORT,
String.valueOf(RESTUtils.findAvailablePort(2000, 3000)));
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
+ ResourceConfig resourceConfig = new ResourceConfig();
+
resourceConfig.register(org.apache.gravitino.lance.service.rest.LanceTableOperations.class);
+ resourceConfig.register(
+ new AbstractBinder() {
+ @Override
+ protected void configure() {
+ bind(namespaceWrapper).to(NamespaceWrapper.class).ranked(2);
+
bindFactory(MockServletRequestFactory.class).to(HttpServletRequest.class);
+ }
+ });
+
+ return resourceConfig;
+ }
+
+ @BeforeAll
+ public static void setup() {
+ when(namespaceWrapper.asTableOps()).thenReturn(tableOps);
+ }
+
+ @Test
+ void testCreateTable() {
+ String tableIds = "catalog.scheme.create_table";
+ String delimiter = ".";
+
+ // Test normal
+ CreateTableResponse createTableResponse = new CreateTableResponse();
+ when(tableOps.createTable(any(), any(), any(), any(), any(), any()))
+ .thenReturn(createTableResponse);
+
+ byte[] bytes = new byte[] {0x01, 0x02, 0x03};
+ Response resp =
+ target(String.format("/v1/table/%s/create", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(bytes, "application/vnd.apache.arrow.stream"));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test illegal argument
+ when(tableOps.createTable(any(), any(), any(), any(), any(), any()))
+ .thenThrow(new IllegalArgumentException("Illegal argument"));
+
+ resp =
+ target(String.format("/v1/table/%s/create", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(bytes, "application/vnd.apache.arrow.stream"));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.createTable(any(), any(), any(), any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/create", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(bytes, "application/vnd.apache.arrow.stream"));
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("Runtime exception", errorResp.getError());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testCreateEmptyTable() {
+ String tableIds = "catalog.scheme.create_empty_table";
+ String delimiter = ".";
+
+ // Test normal
+ CreateEmptyTableResponse createTableResponse = new
CreateEmptyTableResponse();
+ createTableResponse.setLocation("/path/to/table");
+ createTableResponse.setProperties(ImmutableMap.of("key", "value"));
+ when(tableOps.createEmptyTable(any(), any(), any(),
any())).thenReturn(createTableResponse);
+
+ CreateEmptyTableRequest tableRequest = new CreateEmptyTableRequest();
+ tableRequest.setLocation("/path/to/table");
+
+ Response resp =
+ target(String.format("/v1/table/%s/create-empty", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ CreateEmptyTableResponse response =
resp.readEntity(CreateEmptyTableResponse.class);
+ Assertions.assertEquals(createTableResponse.getLocation(),
response.getLocation());
+ Assertions.assertEquals(createTableResponse.getProperties(),
response.getProperties());
+
+ Mockito.reset(tableOps);
+ // Test illegal argument
+ when(tableOps.createEmptyTable(any(), any(), any(), any()))
+ .thenThrow(new IllegalArgumentException("Illegal argument"));
+
+ resp =
+ target(String.format("/v1/table/%s/create-empty", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.createEmptyTable(any(), any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/create-empty", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("Runtime exception", errorResp.getError());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testRegisterTable() {
+ String tableIds = "catalog.scheme.register_table";
+ String delimiter = ".";
+
+ // Test normal
+ RegisterTableResponse registerTableResponse = new RegisterTableResponse();
+ registerTableResponse.setLocation("/path/to/registered_table");
+ registerTableResponse.setProperties(ImmutableMap.of("key", "value"));
+ when(tableOps.registerTable(any(), any(), any(),
any())).thenReturn(registerTableResponse);
+
+ RegisterTableRequest tableRequest = new RegisterTableRequest();
+ tableRequest.setLocation("/path/to/registered_table");
+ tableRequest.setMode(RegisterTableRequest.ModeEnum.CREATE);
+
+ Response resp =
+ target(String.format("/v1/table/%s/register", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ RegisterTableResponse response =
resp.readEntity(RegisterTableResponse.class);
+ Assertions.assertEquals(registerTableResponse.getLocation(),
response.getLocation());
+ Assertions.assertEquals(registerTableResponse.getProperties(),
response.getProperties());
+
+ // Test illegal argument
+ Mockito.reset(tableOps);
+ when(tableOps.registerTable(any(), any(), any(), any()))
+ .thenThrow(new IllegalArgumentException("Illegal argument"));
+ resp =
+ target(String.format("/v1/table/%s/register", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.registerTable(any(), any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/register", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("Runtime exception", errorResp.getError());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testDeregisterTable() {
+ String tableIds = "catalog.scheme.deregister_table";
+ String delimiter = ".";
+
+ DeregisterTableRequest tableRequest = new DeregisterTableRequest();
+
+ DeregisterTableResponse deregisterTableResponse = new
DeregisterTableResponse();
+ deregisterTableResponse.setLocation("/path/to/deregistered_table");
+ deregisterTableResponse.setProperties(ImmutableMap.of("key", "value"));
+ // Test normal
+ when(tableOps.deregisterTable(any(),
any())).thenReturn(deregisterTableResponse);
+
+ Response resp =
+ target(String.format("/v1/table/%s/deregister", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ DeregisterTableResponse response =
resp.readEntity(DeregisterTableResponse.class);
+ Assertions.assertEquals(deregisterTableResponse.getLocation(),
response.getLocation());
+ Assertions.assertEquals(deregisterTableResponse.getProperties(),
response.getProperties());
+
+ // Test illegal argument
+ Mockito.reset(tableOps);
+ when(tableOps.deregisterTable(any(), any()))
+ .thenThrow(new IllegalArgumentException("Illegal argument"));
+ resp =
+ target(String.format("/v1/table/%s/deregister", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test not found exception
+ Mockito.reset(tableOps);
+ when(tableOps.deregisterTable(any(), any()))
+ .thenThrow(
+ LanceNamespaceException.notFound(
+ "Table not found", "NoSuchTableException", tableIds, ""));
+ resp =
+ target(String.format("/v1/table/%s/deregister", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.deregisterTable(any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/deregister", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("Runtime exception", errorResp.getError());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testDescribeTable() {
+ String tableIds = "catalog.scheme.describe_table";
+ String delimiter = ".";
+
+ // Test normal
+ DescribeTableResponse createTableResponse = new DescribeTableResponse();
+ createTableResponse.setLocation("/path/to/describe_table");
+ createTableResponse.setProperties(ImmutableMap.of("key", "value"));
+ when(tableOps.describeTable(any(), any(),
any())).thenReturn(createTableResponse);
+
+ DescribeTableRequest tableRequest = new DescribeTableRequest();
+ Response resp =
+ target(String.format("/v1/table/%s/describe", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ 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());
+
+ // Test not found exception
+ Mockito.reset(tableOps);
+ when(tableOps.describeTable(any(), any(), any()))
+ .thenThrow(
+ LanceNamespaceException.notFound(
+ "Table not found", "NoSuchTableException", tableIds, ""));
+ resp =
+ target(String.format("/v1/table/%s/describe", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.describeTable(any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/describe", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("Runtime exception", errorResp.getError());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testCreateTableIndex() {
+ String tableIds = "catalog.scheme.to_create_index_table";
+ String delimiter = ".";
+
+ // Test normal
+ CreateTableIndexRequest tableRequest = new CreateTableIndexRequest();
+
+ CreateTableIndexResponse response = new CreateTableIndexResponse();
+ response.setProperties(ImmutableMap.of("key", "value"));
+ when(tableOps.createTableIndex(any(), any(), any())).thenReturn(response);
+
+ Response resp =
+ target(String.format("/v1/table/%s/create_index", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ response = resp.readEntity(CreateTableIndexResponse.class);
+ Assertions.assertEquals(response.getProperties(),
response.getProperties());
+
+ Mockito.reset(tableOps);
+ // Test illegal argument
+ when(tableOps.createTableIndex(any(), any(), any()))
+ .thenThrow(new IllegalArgumentException("Illegal argument"));
+
+ resp =
+ target(String.format("/v1/table/%s/create_index", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.createTableIndex(any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/create_index", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("Runtime exception", errorResp.getError());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testListTableIndices() {
+ String tableIds = "catalog.scheme.to_list_index_table";
+ String delimiter = ".";
+
+ ListTableIndicesRequest tableRequest = new ListTableIndicesRequest();
+
+ ListTableIndicesResponse response = new ListTableIndicesResponse();
+ IndexContent indexContent = new IndexContent();
+ indexContent.setIndexName("test_index");
+ indexContent.setColumns(List.of("col1"));
+ response.setIndexes(List.of(indexContent));
+ when(tableOps.listTableIndices(any(), any(), any())).thenReturn(response);
+
+ Response resp =
+ target(String.format("/v1/table/%s/index/list", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ ListTableIndicesResponse actualResponse =
resp.readEntity(ListTableIndicesResponse.class);
+ Assertions.assertEquals(1, actualResponse.getIndexes().size());
+ Assertions.assertEquals("test_index",
actualResponse.getIndexes().get(0).getIndexName());
+ Assertions.assertEquals(List.of("col1"),
actualResponse.getIndexes().get(0).getColumns());
+
+ Mockito.reset(tableOps);
+
+ // Test illegal argument
+ when(tableOps.listTableIndices(any(), any(), any()))
+ .thenThrow(new IllegalArgumentException("Illegal argument"));
+ resp =
+ target(String.format("/v1/table/%s/index/list", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(tableRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test
Review Comment:
Incomplete comment: The comment only says "// Test" without explaining what
is being tested. This should describe the test case, e.g., "// Test not found
exception" to match the pattern used in other test sections.
```suggestion
// Test not found exception
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -688,6 +717,241 @@ void testDeregisterNonExistingTable() {
Assertions.assertEquals(406, lanceNamespaceException.getCode());
}
+ @Test
+ void testCreateTableIndex() throws IOException {
+ catalog = createCatalog(CATALOG_NAME);
+ createSchema();
+ List<String> ids = List.of(CATALOG_NAME, SCHEMA_NAME,
"non_existing_table");
+
+ // We need to create a table first;
+ org.apache.arrow.vector.types.pojo.Schema schema =
+ new org.apache.arrow.vector.types.pojo.Schema(
+ Arrays.asList(
+ Field.nullable("id", new ArrowType.Int(32, true)),
+ Field.nullable("value", new ArrowType.Utf8()),
+ new Field(
+ "vector",
+ FieldType.nullable(new ArrowType.FixedSizeList(4)),
+ ImmutableList.of(
+ Field.nullable(
+ "fake", new
ArrowType.FloatingPoint(FloatingPointPrecision.SINGLE))))));
+ byte[] body = ArrowUtils.generateIpcStream(schema);
+
+ CreateTableRequest request = new CreateTableRequest();
+ request.setId(ids);
+ request.setLocation(tempDir + "/" + "table_for_index/");
+ request.setProperties(
+ ImmutableMap.of(
+ "key1", "v1",
+ "lance.storage.a", "value_a",
+ "lance.storage.c", "value_c"));
+
+ CreateTableResponse response = ns.createTable(request, body);
+ Assertions.assertEquals(request.getLocation(), response.getLocation());
+
+ writeDataToLance(request.getLocation());
+
+ // Now try to create Btree index on an existing table
+ CreateTableIndexRequest createTableIndexRequest = new
CreateTableIndexRequest();
+ createTableIndexRequest.setId(ids);
+ createTableIndexRequest.setIndexType(IndexTypeEnum.BTREE);
+ createTableIndexRequest.setColumn("id");
+ createTableIndexRequest.setMetricType(MetricTypeEnum.L2);
+ CreateTableIndexResponse createTableIndexResponse =
+ Assertions.assertDoesNotThrow(() ->
ns.createTableIndex(createTableIndexRequest));
+ Assertions.assertNotNull(createTableIndexResponse);
+
+ // Now try to create bitmap index on an existing table
+ createTableIndexRequest.setIndexType(IndexTypeEnum.BITMAP);
+ createTableIndexRequest.setColumn("value");
+ createTableIndexResponse =
+ Assertions.assertDoesNotThrow(() ->
ns.createTableIndex(createTableIndexRequest));
+ Assertions.assertNotNull(createTableIndexResponse);
+ List<String> indices = listIndices(request.getLocation());
+ Assertions.assertEquals(2, indices.size());
+ // Now try to create vector index on an existing table
+ createTableIndexRequest.setIndexType(IndexTypeEnum.IVF_FLAT);
+ createTableIndexRequest.setColumn("vector");
+ createTableIndexResponse =
+ Assertions.assertDoesNotThrow(() ->
ns.createTableIndex(createTableIndexRequest));
+ Assertions.assertNotNull(createTableIndexResponse);
+
+ ListTableIndicesRequest listTableIndicesRequest = new
ListTableIndicesRequest();
+ listTableIndicesRequest.setId(ids);
+ ListTableIndicesResponse listTableIndicesResponse =
+ ns.listTableIndices(listTableIndicesRequest);
+ Assertions.assertEquals(3, listTableIndicesResponse.getIndexes().size());
+ List<String> expectedIndexName = listIndices(request.getLocation());
+ for (IndexContent indexContent : listTableIndicesResponse.getIndexes()) {
+ Assertions.assertTrue(
+ expectedIndexName.contains(indexContent.getIndexName()),
+ "Index name should be in the expected index names.");
+ if (indexContent.getIndexName().equals("id_idx")) {
+ Assertions.assertEquals("id", indexContent.getColumns().get(0));
+ } else if (indexContent.getIndexName().equals("value_idx")) {
+ Assertions.assertEquals("value", indexContent.getColumns().get(0));
+ } else if (indexContent.getIndexName().equals("vector_idx")) {
+ Assertions.assertEquals("vector", indexContent.getColumns().get(0));
+ }
+ }
+
+ // create another table to test other index types
+ ids = List.of(CATALOG_NAME, SCHEMA_NAME, "table_for_other_indexes");
+ request.setId(ids);
+ request.setLocation(tempDir + "/" + "table_for_other_indexes/");
+ response = ns.createTable(request, body);
+ Assertions.assertEquals(request.getLocation(), response.getLocation());
+ writeDataToLance(request.getLocation());
+
+ // Now try to create FTS index on an existing table
+ createTableIndexRequest.setId(ids);
+ createTableIndexRequest.setIndexType(IndexTypeEnum.FTS);
+ createTableIndexRequest.setColumn("value");
+
+ LanceNamespaceException exception =
+ Assertions.assertThrows(
+ LanceNamespaceException.class, () ->
ns.createTableIndex(createTableIndexRequest));
+ // com.lancedb.lance.index.IndexType does not have FTS yet, so it should
throw exception
+ Assertions.assertTrue(
+ exception.getMessage().contains("No enum constant
com.lancedb.lance.index.IndexType.FTS"));
+ }
+
+ private List<String> listIndices(String lanceTableLocation) {
+ try (Dataset dataset = Dataset.open(lanceTableLocation)) {
+ return dataset.listIndexes();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private void writeDataToLance(String tableLocation) {
+ try (Dataset dataset = Dataset.open(tableLocation)) {
+ org.apache.arrow.vector.types.pojo.Schema lanceSchema =
dataset.getSchema();
+ Transaction trans =
+ dataset
+ .newTransactionBuilder()
+ .operation(
+ Append.builder()
+ .fragments(
+ createFragmentMetadata(tableLocation,
generateLanceData(), lanceSchema))
+ .build())
+ .writeParams(ImmutableMap.of())
+ .build();
+
+ Dataset newDataset = dataset.commitTransaction(trans);
+
+ try (LanceScanner scanner =
+ newDataset.newScan(
+ new ScanOptions.Builder()
+ .columns(Arrays.asList("id", "value", "vector"))
+ .batchSize(1000)
+ .build())) {
+
+ List<LanceDataValue> dataValues =
com.google.common.collect.Lists.newArrayList();
+ try (ArrowReader reader = scanner.scanBatches()) {
+ while (reader.loadNextBatch()) {
+ VectorSchemaRoot root = reader.getVectorSchemaRoot();
+ List<FieldVector> fieldVectors = root.getFieldVectors();
+
+ IntVector ids = (IntVector) fieldVectors.get(0);
+ VarCharVector values = (VarCharVector) fieldVectors.get(1);
+ FixedSizeListVector vectors = (FixedSizeListVector)
fieldVectors.get(2);
+
+ for (int i = 0; i < root.getRowCount(); i++) {
+ int id = ids.get(i);
+ String value = new String(values.get(i), StandardCharsets.UTF_8);
+ List<Float> vector =
com.google.common.collect.Lists.newArrayList();
+ for (int j = 0; j < 4; j++) {
+ Float floatValue = ((Float4Vector)
vectors.getDataVector()).get(i * 4 + j);
+ vector.add(floatValue);
+ }
+
+ dataValues.add(new LanceDataValue(id, value, vector));
+ }
+ }
+ }
+
+ Assertions.assertEquals(5120, dataValues.size());
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(e);
Review Comment:
Redundant exception handling: There are three nested try-catch blocks where
both the outer catch blocks (lines 878-879 and 880-882) catch `Exception` and
wrap it in `RuntimeException`. This is redundant since any
`JsonProcessingException` is already a subclass of `Exception`. The two
separate outer catch blocks can be combined into a single catch block.
```suggestion
```
--
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]
