absurdfarce commented on code in PR #1931:
URL:
https://github.com/apache/cassandra-java-driver/pull/1931#discussion_r1769320442
##########
query-builder/src/main/java/com/datastax/oss/driver/api/querybuilder/select/Select.java:
##########
@@ -146,6 +146,8 @@ default Select orderBy(@NonNull String columnName, @NonNull
ClusteringOrder orde
return orderBy(CqlIdentifier.fromCql(columnName), order);
}
+ @NonNull
+ Select orderBy(@NonNull Ann ann);
Review Comment:
Maybe I'm missing something but it seems more natural to support something
like the following:
```
Select orderByAnnOf(CqlIdentifier columnId, CqlVector ann);
Select orderByAnnOf(String columnName, CqlVector ann);
```
Advantage is that with this approach you don't even need to introduce an Ann
type... which kinda seems right as that type isn't really doing much for you
here.
You could also perhaps add a notion of type checking the specified column to
make sure it's a vector type (and to make sure it matches the type of the input
CqlVector).
To the point made by @lukasz-antoniak above we could add directionality here
(and throw warnings if the user tries to use a DESC order before there's
server-side support for it) but I'm not sure it's worth it. There's no mention
of ordering in the [relevant Cassandra
docs](https://cassandra.apache.org/doc/latest/cassandra/getting-started/vector-search-quickstart.html#query-vector-data-with-cql)
so my intuition says to just leave it out for now and add it when it becomes
more of a thing.
##########
query-builder/src/test/java/com/datastax/oss/driver/api/querybuilder/schema/AlterTableTest.java:
##########
@@ -108,4 +108,15 @@ public void
should_generate_alter_table_with_no_compression() {
assertThat(alterTable("bar").withNoCompression())
.hasCql("ALTER TABLE bar WITH compression={'sstable_compression':''}");
}
+
+ @Test
+ public void should_generate_alter_table_with_vector() {
+ assertThat(
+ alterTable("bar")
+ .alterColumn(
+ "v",
+ DataTypes.custom(
+
"org.apache.cassandra.db.marshal.VectorType(org.apache.cassandra.db.marshal.FloatType,3)")))
Review Comment:
```
DataTypes.custom(DataTypes.vectorOf(DataTypes.Float,3))
```
##########
query-builder/src/main/java/com/datastax/oss/driver/api/querybuilder/QueryBuilder.java:
##########
@@ -538,4 +541,12 @@ public static Truncate truncate(@Nullable String keyspace,
@NonNull String table
return truncate(
keyspace == null ? null : CqlIdentifier.fromCql(keyspace),
CqlIdentifier.fromCql(table));
}
+
+ public static Ann annOf(@NonNull CqlIdentifier cqlIdentifier, @NonNull
CqlVector<Number> vector) {
+ return new DefaultAnn(cqlIdentifier, vector);
+ }
+
+ public static Ann annOf(@NonNull String cqlIdentifier, @NonNull
CqlVector<Number> vector) {
+ return new DefaultAnn(CqlIdentifier.fromCql(cqlIdentifier), vector);
+ }
Review Comment:
These will need to be updated when the PR for JAVA-3143 is merged; the
CqlVector<Number> constraint won't apply once that's in.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/type/DefaultVectorType.java:
##########
@@ -60,7 +60,8 @@ public String getClassName() {
@NonNull
@Override
public String asCql(boolean includeFrozen, boolean pretty) {
- return String.format("'%s(%d)'", getClassName(), getDimensions());
+ return String.format(
+ "VECTOR<%s, %d>", this.subtype.asCql(includeFrozen,
pretty).toUpperCase(), getDimensions());
Review Comment:
This should be a lower case "VECTOR" to match what's done with the other
collection types
([list](https://github.com/apache/cassandra-java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/api/core/type/ListType.java#L27-L32),
[set](https://github.com/apache/cassandra-java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/api/core/type/SetType.java#L27-L32),
[map](https://github.com/apache/cassandra-java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/api/core/type/MapType.java#L33-L41))
##########
query-builder/src/test/java/com/datastax/oss/driver/api/querybuilder/insert/RegularInsertTest.java:
##########
@@ -41,6 +42,12 @@ public void should_generate_column_assignments() {
.hasCql("INSERT INTO foo (a,b) VALUES (?,?)");
}
+ @Test
+ public void should_generate_vector_literals() {
+ assertThat(insertInto("foo").value("a", literal(Arrays.asList(0.1, 0.2,
0.3))))
Review Comment:
As above: this should use a CqlVector rather than an Array
##########
query-builder/src/test/java/com/datastax/oss/driver/api/querybuilder/schema/AlterTypeTest.java:
##########
@@ -53,4 +54,10 @@ public void
should_generate_alter_table_with_rename_three_columns() {
assertThat(alterType("bar").renameField("x", "y").renameField("u",
"v").renameField("b", "a"))
.hasCql("ALTER TYPE bar RENAME x TO y AND u TO v AND b TO a");
}
+
+ @Test
+ public void should_generate_alter_type_with_vector() {
+ assertThat(alterType("foo", "bar").alterField("vec", new
DefaultVectorType(DataTypes.FLOAT, 3)))
Review Comment:
```
DataTypes.vectorOf(DataTypes.Float, 3)
```
##########
query-builder/src/test/java/com/datastax/oss/driver/api/querybuilder/delete/DeleteSelectorTest.java:
##########
@@ -34,6 +35,16 @@ public void should_generate_column_deletion() {
.hasCql("DELETE v FROM ks.foo WHERE k=?");
}
+ @Test
+ public void should_generate_vector_deletion() {
+ assertThat(
+ deleteFrom("foo")
+ .column("v")
+ .whereColumn("k")
+ .isEqualTo(literal(Arrays.asList(0.1, 0.2))))
Review Comment:
This test relies on the fact that vectors happen to be represented using the
same syntax as arrays. This test should be changed to use a CqlVector here
instead of an array.
##########
query-builder/src/test/java/com/datastax/oss/driver/api/querybuilder/schema/CreateTypeTest.java:
##########
@@ -83,4 +84,13 @@ public void should_create_type_with_collections() {
.withField("map", DataTypes.mapOf(DataTypes.INT,
DataTypes.TEXT)))
.hasCql("CREATE TYPE ks1.type (map map<int, text>)");
}
+
+ @Test
+ public void should_create_type_with_vector() {
+ assertThat(
+ createType("ks1", "type")
+ .withField("c1", DataTypes.INT)
+ .withField("vec", new DefaultVectorType(DataTypes.FLOAT, 3)))
Review Comment:
As above:
```
DataTypes.vectorOf(DataTypes.Float, 3)
```
##########
query-builder/src/test/java/com/datastax/oss/driver/api/querybuilder/schema/CreateTableTest.java:
##########
@@ -307,4 +308,13 @@ public void
should_generate_create_table_time_window_compaction() {
.hasCql(
"CREATE TABLE bar (k int PRIMARY KEY,v text) WITH
compaction={'class':'TimeWindowCompactionStrategy','compaction_window_size':10,'compaction_window_unit':'DAYS','timestamp_resolution':'MICROSECONDS','unsafe_aggressive_sstable_expiration':false}");
}
+
+ @Test
+ public void should_generate_vector_column() {
+ assertThat(
+ createTable("foo")
+ .withPartitionKey("k", DataTypes.INT)
+ .withColumn("v", new DefaultVectorType(DataTypes.FLOAT, 3)))
Review Comment:
As above:
```
DataTypes.vectorOf(DataTypes.Float, 3)
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
