List tables command #1510

maheshrajamani · 2024-10-08T00:21:53Z

What this PR does:

Which issue(s) this PR fixes:
Fixes #1500

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA

…needed because tables need multiple vector config.

…ormation

…TOR_EMBEDDING_TEXT_FIELD`

…ct-multiple-vectorize

…te/data-api into schema-outject-multiple-vectorize

…needed because tables need multiple vector config.

.../java/io/stargate/sgv2/jsonapi/api/model/command/table/definition/datatype/ComplexTypes.java

...in/java/io/stargate/sgv2/jsonapi/api/model/command/table/definition/datatype/ColumnType.java

...n/java/io/stargate/sgv2/jsonapi/api/model/command/serializer/ColumnDefinitionSerializer.java

tatu-at-datastax · 2024-10-08T17:11:02Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/serializer/OrderingKeySerializer.java

+    jsonGenerator.writeEndObject();
+  }
+
+  public record ApiSupport(


Could/should this be private?

... or, should the 2 separate definitions be combined?

Made to private for now. But this will evolve as we analyze how different unsuported format are handled.

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/serializer/OrderingKeySerializer.java

...in/java/io/stargate/sgv2/jsonapi/api/model/command/table/definition/datatype/ColumnType.java

tatu-at-datastax · 2024-10-08T17:14:54Z

...ava/io/stargate/sgv2/jsonapi/api/model/command/table/definition/datatype/PrimitiveTypes.java


 /** Interface for primitive column types similar to what is defined in cassandra java driver. */
-public class PrimitiveTypes {
+public enum PrimitiveTypes implements ColumnType {


Nice simplification by changing to Enum!

...ava/io/stargate/sgv2/jsonapi/api/model/command/table/definition/datatype/PrimitiveTypes.java

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

tatu-at-datastax · 2024-10-08T17:25:37Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

+                  columnVectorDefinition.vectorizeConfig().modelName(),
+                  columnVectorDefinition.vectorizeConfig().authentication(),
+                  columnVectorDefinition.vectorizeConfig().parameters());
+      return new ComplexTypes.VectorType(PrimitiveTypes.FLOAT, vt.getDimensions(), vectorizeConfig);


Should we do any verification wrt vector being of "Float" type? I know we currently only support that, but is there any benefit from future-proofing?

I left this to have a format hardcoded since cql driver has it. If and when cassandra adds support for other type, We can enable it with minimal changes and exposing it in api request.

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/ListTablesOperation.java

src/test/java/io/stargate/sgv2/jsonapi/api/v1/tables/ListTablesIntegrationTest.java

tatu-at-datastax

Looks mostly good, but did have some notes on CqlIdentifier handling, integration tests that I'd like clarified.

tatu-at-datastax

LGTM

amorton

I would have -1 this PR, it needs changes to use the OperationAttempt model and there are a number of code quality issues.

amorton · 2024-10-09T03:36:12Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/ListTablesCommand.java

+import org.eclipse.microprofile.openapi.annotations.enums.SchemaType;
+import org.eclipse.microprofile.openapi.annotations.media.Schema;
+
+@Schema(description = "Command that lists all available tables in a namespace.")


This should say keyspace not namespace

amorton · 2024-10-09T03:37:37Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/ListTablesCommand.java

+public record ListTablesCommand(Options options) implements TableOnlyCommand {
+  public record Options(
+      @Schema(
+              description = "include table properties.",


typo: "include" to "Include" also this is not the most descriptive.

is this more truthful ? "When True the schema for the table is returned, when false just the table name"

amorton · 2024-10-09T03:42:22Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/serializer/OrderingKeysSerializer.java

+   * @param read
+   * @param cqlDefinition
+   */
+  private record ApiSupport(


why is this record here and in ColumnDefinitionSerializer ?

I have added a comment in there, this is just a place holder object to generate apiSupport for unsupported columns, This needs to be analyzed for what operation can be supported for unsupported types.

amorton · 2024-10-09T03:43:29Z

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/table/definition/PrimaryKey.java

@@ -16,9 +19,15 @@
 //    implementation = Object.class,
 //    description = "Represents the table primary key")


can we remove the commented out lines now ?

Eventually these are needed when we go public, why remove them?

amorton · 2024-10-09T03:46:23Z

...in/java/io/stargate/sgv2/jsonapi/api/model/command/table/definition/datatype/ColumnType.java

+  default String getApiName() {
+    return getApiDataType().getApiName();
+  }
+


why does this list getSupportedTypes() exist seperate to the definitions of the data types we have else where ?

There is no consolidated place to get all supported data types. Error messages needs to be revisited, May be not a good idea to return list of supported types.

amorton · 2024-10-09T04:05:35Z

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/TableSchemaObject.java

@@ -115,4 +128,121 @@ public static TableSchemaObject from(TableMetadata tableMetadata, ObjectMapper o
    }
    return new TableSchemaObject(tableMetadata, vectorConfig);
  }
+


there are a number of things to imprve in the from() function #1521

amorton · 2024-10-09T04:07:01Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/ListTablesOperation.java

+ * @param tableMatcher {@link CollectionTableMatcher}
+ * @param commandContext {@link CommandContext}
+ */
+public record ListTablesOperation(


pls use a class, cut back on records unless they are basic pojo objects

amorton · 2024-10-09T04:08:04Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/ListTablesOperation.java

+  // below ?
+  private static final CollectionTableMatcher TABLE_MATCHER = new CollectionTableMatcher();
+
+  public ListTablesOperation(


Why dont we use the GenericOperation and OperationAttempt ?

amorton · 2024-10-09T04:08:23Z

src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/ListTablesOperation.java

+    if (keyspaceMetadata == null) {
+      return Uni.createFrom()
+          .failure(
+              ErrorCodeV1.KEYSPACE_DOES_NOT_EXIST.toApiException(


no v1 error code in the table work

amorton · 2024-10-09T04:09:34Z

src/main/java/io/stargate/sgv2/jsonapi/service/resolver/ListTablesCommandResolver.java

+  /** {@inheritDoc} */
+  @Override
+  public Operation resolveKeyspaceCommand(
+      CommandContext<KeyspaceSchemaObject> ctx, ListTablesCommand command) {


This should be using the OperationAttempt model

We didn't have a cql command executed in here, so did not use the OperationAttempt model.

maheshrajamani added 25 commits October 4, 2024 23:12

Made the SchemaObject store and return List of VectorConfig. This is …

42d0fb5

…needed because tables need multiple vector config.

Changes for TableSchemaObject to deserialize vector and vectorize inf…

c016640

…ormation

Changed the vector config to unmodifiable list

d590679

Fixed the comments

aa656ef

Fixed the comments

02bc38e

Use $vectorize field name from constant `DocumentConstants.Fields.VEC…

24da975

…TOR_EMBEDDING_TEXT_FIELD`

Use $vectorize field name from constant `DocumentConstants.Fields.VEC…

ecabca3

…TOR_EMBEDDING_TEXT_FIELD`

Use $vectorize field name from constant `DocumentConstants.Fields.VEC…

bc30acb

…TOR_EMBEDDING_TEXT_FIELD`

Backup commit for list tables

f0cb7bd

Updated the code based on review.

550ce05

Merge branch 'main' into schema-outject-multiple-vectorize

0a24bd0

Merge branch 'main' of github.com:stargate/data-api into schema-outje…

5220f57

…ct-multiple-vectorize

Merged the refactor code based on review.

d94ce09

Working listTables

d47be3a

Fixed the vectorize config deserializer

2683c66

Changes based on review

e7e92cb

Merge branch 'schema-outject-multiple-vectorize' of github.com:starga…

78ba059

…te/data-api into schema-outject-multiple-vectorize

Merge branch 'main' into schema-outject-multiple-vectorize

366a503

Resolve the merge compile error

27f3bef

Resolve the merge compile error

11b589e

Resolve the merge compile error

6ad43dc

Formatted the file

dd2f2cb

Fix for IT

5e45661

Resolved conflict after merge

7483d58

Added UNSUPPORTED type support

da04424

maheshrajamani requested a review from a team as a code owner October 8, 2024 00:21

maheshrajamani added 4 commits October 7, 2024 20:27

Made the SchemaObject store and return List of VectorConfig. This is …

d6cd08f

…needed because tables need multiple vector config.

Backup commit for list tables

9f386d2

Updated the code based on review.

fc48d0d

Working listTables

e01992e