vector: add support for vector type #22488

QuerthDP · 2025-01-24T17:53:59Z

This pull request is an implementation of vector data type similar to one used by Apache Cassandra.

The patch contains:

implementation of vector_type_impl class
necessary functionalities similar to other data types
support for serialization and deserialization of vectors
support for Lua and JSON format
valid CQL syntax for vector<> type
type_parser support for vectors
expression adjustments such as:
- add collection_constructor::style_type::vector
- rename collection_constructor::style_type::list to collection_constructor::style_type::list_or_vector
vector type encoding (for drivers)
unit tests
cassandra compatibility tests
necessary documentation

Fixes #19455

scylladb-promoter · 2025-01-24T17:54:46Z

@scylladb/scylla-maint please review and trigger CI for this pull request
CI Build: https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/parambuild/?PR_NUMBER=22488

scylladb-promoter · 2025-01-24T17:57:04Z

Docs Preview 📖

Docs Preview for this pull request is available here

Changed Files:

Note: This preview will be available for 30 days and will be automatically deleted after that period. You can manually trigger a new build by committing changes.

cql3/expr/expression.cc

concrete_types.hh

avikivity · 2025-01-24T18:26:16Z

cql3/type_json.cc

+    }
+    return vector_type_impl::build_value(std::move(raw_vector), t.value_length_if_fixed());
+}
+


Is this tested somewhere?

This functionality is tested here.
I believe this test is sufficient, but we might adjust/expand it, if there are still any concerns.

Personally, I don't understand why write any functionality tests in a Boost test. It should be in cqlpy (I'd create a new file test/cqlpy/test_type_vector.py for it), so you'll be able to compare the functionality to that in Cassandra.
Although specifically for JSON output and input, we actually have a lot of these tests in test/cqlpy/test_json.py. For example see test_tojson_timestamp and test_fromjson_timestamp which checked tojson() of the "timestamp" type - which we used to have a bug in.

This boost test have been added due to the reasoning that other data types have this kind of test here as well.

We are now aware that this PR lacks the reasonable amount of cqlpy tests, and it is our high priority TODO to deliver them in a close future. Will keep you updated!

lang/lua.cc

avikivity · 2025-01-24T18:30:53Z

types/types.cc

+            return cmp;
+        }
+    }
+


Please verify that this ordering is compatible with Cassandra.

This is why I encourage people to write all tests that can be exercised through CQL in test/cqlpy and not in the test/boost cql_env framework. The latter always leaving you wondering whether the behavior you just wrote a test for actually matches Cassandra's behavior. The former allows you to really be sure that Scylla's behavior is identical to Cassandra and not incompatible with Cassandra for no good read.

I saw you translated Cassandra's unit tests and put it in test/cqlpy/cassandra_tests, which is great, but you can also add new cqlpy tests, just don't put them in cassandra_tests (a new file test/cqlpy/test_type_vector.py would be a good place for new tests for the vector type).

types/types.cc

cql3/Cql.g

cql3/cql3_type.cc

avikivity · 2025-01-24T18:52:44Z

cql3/expr/expression.cc

@@ -70,6 +70,7 @@ unresolved_identifier::~unresolved_identifier() = default;
 static cql3::raw_value do_evaluate(const bind_variable&, const evaluation_inputs&);
 static cql3::raw_value do_evaluate(const tuple_constructor&, const evaluation_inputs&);


In particular, this separates the implementation from collections,
which could've been quite misleading and inelegant.

But is there any practical difference from collection_constructor? This adds a lot of noise.

I do believe it is crucial to add the vector_constructor, regardless.

First of all, if we used collection_constructor the way it is implemented now, we would have to use the do_evaluate function with it.
There emerges a problem, as if we come into evaluate_list (as we would be redirected here, while literals for vectors and the lists are the same) then the wrong serialization function would be executed (serialize_listlike function differs from the one required for vectors according to the protocol [1]).
We cannot just simply differ the vector from the list here, because there is no access to the column type from this part of code, it would require major refactoring if we wanted to do it this way.
Moreover, which is a minor issue, the exception messages would not match, as the code would believe we executed the evaluate_list for the list, not vector, and list is hard coded here in the messages.

💡 We could have introduced new collection_constructor::style_type different from the list one and implement crucial functions for vectors in collection_constructor the same way as we split maps and sets from lists, BUT as mentioned before it is not possible due to the ambiguity of list and vector literals...

To resolve the issue, we implemented the distinction in prepare_expression, as there is a simple access to the column type through the receiver.

Furthermore, we needed the split during preparation anyway, because the preparation for vectors and lists slightly differs (mainly due to the fact that lists can be empty), and that we need to produce proper column specification of vector_type_impl.

On the other hand, I think it could be done the other way around, if we truly don't want to introduce a new constructor, but it would require more changes and does not come naturally to my mind how to do it.

On a final note, I do think clean and tidy code is a good thing to introduce, and splitting the vector from collections is,
I believe an example of it, as it delivers more clarity for future development and maintenance.
Of course, it might not be perfect as we do still learn and gain experience, but I hope it's clear enough what should it do.

[1] the protocol itself has some other issues, which will be mentioned here

I think adding a new style_type would be better (alternatively, switching on collection_constructor::type).

I understand your motivation to have the vector code separate, but my experience with the expression code is that we won't touch collection_constructor again (or at least until someone adds a new form of collections). On the other hand anyone implementing or maintaining a visitor will have to handle yet another top-level case which just leads to repetition. Better to isolate construction in one place.

Oh OK, I think I understand your idea now.
From a fresh glimpse at the implementation we might indeed replace the vector_constructor which a new separate collection_constructor::style_type, which would be used only after preparation (as vector_constructor is used now).

I'm going to look into it deeper from now on, but high hopes it can be done the way with additional style type.
Feels like a good solution for me as well, since I noticed it. 🤞

I've just rebased the mentioned commits to implement vector style type instead of vector constructor.

Nevertheless I have two new ideas how could we improve the code from now on:

We might introduce the list style type, which would be used exactly the same as vector style type now (i.e. only after preparation). It would leave us with a straight look at what is truly a list, and what could be also a vector.

We could revert the rename of list to list_or_vector and provide more comments about the vector using list style before the preparation (feels a bit misleading, but would be also a good idea, and it requires less changes to the code).

Please take a look at these changes and let me know what do you think!

@piodul please take a look at the changes as well, as we discussed how the implementation of the expressions should look like.

avikivity · 2025-01-24T19:01:01Z

Looks generally good. I did not like the addition of vector_constructor, it looks like another special case of collection_constructor (even if the name isn't an exact match) and doesn't merit forcing everyone who implements a visitor to special-case it.

I'd like to see tests that check that an INSERT with a bind variable and a vector with incorrect length fails validation.

nyh · 2025-01-26T08:55:47Z

test/cqlpy/cassandra_tests/validation/operations/cql_vector_test.py

+from ...porting import *
+
+
+# Some of the lines in these tests are commented out because Scylla doesn't support arithmetic operations in literals.


Thanks. I'm happy you translated those tests, but just wanted to confirm something about the process of doing that. Am I correct in understanding you didn't delete or change any tests except the few things that you left commented out (not deleted) with an explanation why in a comment? If my understanding in correct, than it looks very good.

Some context: When I translated the rest of the tests (this is part of an on-going process that I've been doing on and off for a couple of years...) the main guiding principle was not to change the tests. We can't drop a test, or change it, just because it fails on Scylla or we think Cassandra is wrong and we want all our tests to be successful ;-) For example, if a test needs arithmetic operations (#2693) and Scylla doesn't have them, the test should "xfail " with that issue stated as a reason. But when this happens while I'm translating the test, I also comment out the lines which expose 2693 to see nothing else in the test fails because sometimes there is more than one bug exposed by one test. Usually, I do this commenting out temporarily and then when I'm done I restore that line it and leave the "xfail". But I am sort of fine in your case you left some of these lines exposing 2693 as commented out, because we have plenty of other tests exposing 2693 (arithmetic literals). But... on the other hand... There's also something to be said about testing this syntax. I.e., the fact that 1+1 works (and tested elsewhere and xfails until we do 2693) doesn't prove that a vector [1, 1+1] works - it's a separate piece of syntax, and also needs to be tested somewhere.

On the third (?) hand, I agree that having all the vector tests xfail because they inserted 1+1 to each and every one of them is silly and counter-productive, because it makes the tests unusable as regression tests (the tests xfail, so if the vector code breaks nobody can notice, because the test will fail just like before). So another alternative is to comment out all those instances of 1+1 except one, or perhaps leave all of them commented out and just create a tiny new test checking this specific syntax, leaving it xfailing on 2693.

Beyond that detail of #2693, and other things I saw commented out in the test (one thing that isn't a CQL test, so it's fine to comment out, another thing which the Python driver can't test, which is more sad but sometimes unavoidable) I just wanted to confirm there are no other changes in the translated test - nothing you just deleted without leaving commented-out code, and so on. The approach of leaving behind commented-out code with an explanation why you had to comment it out is good - it is possible that for some of them (e.g., that missing Python-driver feature) we can come back to fix it later.

Yes, your understanding is correct and thanks for the context!

The only other meaningful changes in the translated tests (except for 1 accidental one that I just found), are the error messages in the assertInvalidThrowMessage assertions. That is because some of the error messages differ between Scylla and Cassandra, so we have to test a subset of each message, e.g. check for "bytes" instead of "Unexpected 2 extraneous bytes after vector<text, 2> value".

As for the 1+1 syntax, I think we will add a new cqlpy test that covers it and mark it xfail on 2693.

test/cqlpy/cassandra_tests/validation/operations/cql_vector_test.py

nyh · 2025-01-26T09:23:45Z

Hi, we have an issue #19455 about adding Cassandra 5's "VECTOR" type to Scylla.
If this PR implements this feature as this issue requests (namely, Cassandra 5 compatibility) please mention "Fixes #19455" in the cover letter. If it only partially implements this feature, please explicitly mention what still needs to be done or what's different, and mention "Refs #19455" (which connects the patch to the issue, but won't close the issue automatically).

The vector is a fixed-length array of non-null specified type elements. Implement serialization, deserialization, comparison, JSON and Lua support, and other functionalities. Co-authored-by: Dawid Pawlik <[email protected]>

scylladb-promoter · 2025-01-26T19:10:21Z

@scylladb/scylla-maint please review and trigger CI for this pull request
CI Build: https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/parambuild/?PR_NUMBER=22488

QuerthDP · 2025-01-26T19:13:13Z

Changelog:

adjusted documentation for vector support for Lua
removed unnecessary support for empty type in vectors
added description of vector type in proper commit message

scylladbbot · 2025-01-27T09:52:07Z

@QuerthDP new branch branch-2025.1 was added, please add backport label if needed

scylladb-promoter · 2025-01-27T23:44:07Z

@scylladb/scylla-maint please review and trigger CI for this pull request
CI Build: https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/parambuild/?PR_NUMBER=22488

QuerthDP · 2025-01-27T23:50:24Z

Changelog:

removed vector_constructor and implemented vector style type instead

nyh

The whole series looks very good to me, I only left a few minor comments questions and requests.

types/types.cc

nyh · 2025-01-28T11:17:49Z

types/types.cc

+        : concrete_type(kind::vector, make_name(elements, dimension),
+        elements->value_length_if_fixed() ? std::optional(elements->value_length_if_fixed().value()*dimension):std::nullopt),
+        _elements_type(elements), _dimension(dimension) {
+    _contains_set_or_map = _elements_type->contains_set_or_map();


I'm curious why vectors of set or map are even allowed, or why it matters if it's a "set or map" and not a list of or other strange things. I hope I'll understand this later as I read this series.

I wanted to ban vectors of collections too, but it turns our Cassandra allows them.

types/types.cc

cql3/Cql.g

cql3/cql3_type.cc

transport/server.cc

docs/cql/types.rst

nyh · 2025-01-28T12:28:50Z

docs/cql/types.rst

+it is not possible to update only some elements of a vector (without updating the whole vector).
+
+Nevertheless, types stored in a vector are not implicitly frozen, so if you want to store a frozen collection or 
+frozen UDT in a vector, you need to explicitly wrap them using `frozen` keyword.


The last paragraph is interesting. I think the word "neverthless" is a bit confusing (it confused me for a second) because this paragraph doesn't actually contradict to the previous paragraph and in my opinion - not even related to it. The previous paragraph is about frozen<vector<...>, and this one is about vector<frozen<...>. In any case, I wonder if anyone would ever ever use a vector of anything except float or double :-)

Indeed it might be a bit confusing at a first glance.
As you mentioned the core purpose for vectors are for storing floats and doubles, so I think this paragraph is unnecessary and may be removed. Don't you agree?

I guess it's not unnecessary if in fact vector<list,3> is supported and not identical to vector<frozen<list,3>. But I think it could be made identical... Since the data held by a vector is immutable (you can't do something like SET a[3]++) , it's pointless to have a non-frozen collection as the vector's item.

Anyway, I think we first need to decide what we want to happen and only then document exactly what we decided. If what this paragraph says is truely what we decided, then I think it should stay (just with a different word instead of "nevertheless").

I guess we can also base our decision on what Cassandra did - even if it's weird.

AFAIK Cassandra did exactly that what we implemented, i.e. it allows both frozen and unfrozen collections without implicitly freezing them.

In this case, as you suggested, we might only change the misleading vocabulary and it should be just fine.

The nested collections are implicitly frozen, even if the "frozen" flag isn't set.

Frozen means individual cells cannot be updated independently (UPDATE tab SET my_set = my_set + {3}).

There's no syntax (or support in the storage engine) to update a nested set element, so it's implicitly frozen.

Similarly, a vector is itself implicitly frozen (you cannot update a single element), so any nested collections would be similarly implicitly frozen.

I guess we have to distinguish between "visible frozen" (shows up with frozen<> wrappers) and implicitly frozen (the entire collection is atomic). Yuch.

That's all true. I might have used the wrong vocabulary here as well.

What I really meant by 'implicitly freezing' was a recursive call of freeze() function on the element type (like it is done in both frozen collections and tuples).

The lack of the recursive call implies that the types inside the vector are not visibly frozen, while efectively being frozen (cannot be updated) as you stated.

Cassandra 5.0 does support a vector with text subtype for example ... - see https://issues.apache.org/jira/browse/CASSANDRA-18960?focusedCommentId=17782187&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17782187

What's the use case? I donno.

docs/reference/limits.rst

This change is introduced due to lack of support for vector class name, used by type_parser to create data_type based on given class name (especially compound class name with inner types or other parameters). Add function that parses vector type parameters from a class name.

Contains two type_parser tests: one for a valid vector and another for invalid vector.

These tests check serialization and deserialization (including JSON), basic inserts and selects, aggregate functions, element validation, vector usage in user defined types and functions. test_vector_between_user_types is a translated Apache Cassandra test to check if it is handled properly internally.

Motivation for this changes is to provide a distinguishable interface for vector type expressions. The square bracket literal is ambigious for lists and vectors, so that we need to perform a distinction not using CQL layer. At first we should use the collection constructor to manage both lists and vectors (although a vector is not a collection). Later during preparation of expressions we should be able to get to know the exact type using given receiver (column specification). Knowing the type of expression we may use their respective style type (in this case the vector style type being introduced), which would make the implementation more precise and allow us to evaluate the expressions properly. This commit introduces vector style type and functions making use of it. However vector style type is not yet used anywhere, the next commit should adjust collection constructor and make use of the new vector style type and it's features.

Like mentioned in the previous commit, this changes introduce usage of vector style type and adjusts the functions using list style type to distinguish vectors from lists. Rename collection constructor style list to list_or_vector.

Add and adjust tests using vector and list_or_vector style types. Implemented utilities used in expr_test similar to those added in 8f6309b.

This change has been introduced to enable CQL drivers to recognize vector type in query results. The encoding has been imported from Apache Cassandra implementation to match Cassandra's and latest drivers' behaviour. Co-authored-by: Dawid Pawlik <[email protected]>

Add cql_vector_test which tests the basic functionalities of the vector type using CQL. Add vectors_test which tests if descending ordering of vector is supported.

Add missing vector type documentation including: definition of vector, adjustment of term definition, JSON encoding, Lua and cql3 type mapping, vector dimension limit, and keyword specification.

scylladb-promoter · 2025-01-28T20:17:29Z

@scylladb/scylla-maint please review and trigger CI for this pull request
CI Build: https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/parambuild/?PR_NUMBER=22488

QuerthDP · 2025-01-28T20:19:28Z

Changelog:

adjust docs (used @nyh's description of a vector. Thank you!)
disallow negative numbers as vector dimension
remove unexpected freeze<> in one of JSON tests

scylladb-promoter · 2025-01-29T21:21:44Z

🔴 CI State: FAILURE

✅ - Build
✅ - Unit Tests Custom
The following new/updated tests ran 100 times for each mode:
🔹 boost/cql_functions_test
🔹 boost/cql_query_test
🔹 boost/expr_test
🔹 boost/json_cql_query_test
🔹 boost/types_test
🔹 boost/user_function_test
🔹 boost/user_types_test
🔹 boost/view_schema_test
✅ - dtest with tablets
✅ - dtest with topology changes
✅ - dtest
❌ - Unit Tests

Failed Tests (1/43308):

cqlpy.cassandra_tests/validation/entities/secondary_index_test.debug.1 🔍

Build Details:

Duration: 5 hr 40 min
Builder: spider2.cloudius-systems.com

QuerthDP · 2025-01-30T18:36:26Z

I've noticed that the CI didn't pass on a single test.

Failed Tests (1/43308):
cqlpy.cassandra_tests/validation/entities/secondary_index_test.debug.1 🔍

As I don't have access to read the jenkins logs, could I ask anyone responsible to let me know what was the reason for the failure?
Thank you in advance.

nyh · 2025-01-30T19:55:12Z

As I don't have access to read the jenkins logs, could I ask anyone responsible to let me know what was the reason for the failure? Thank you in advance.

This is cqlpy, cassandra_tests/validation/entities/secondary_index_test.py::testCreateAndDropIndexWithCamelCaseIdentifier the test crashed Scylla once in debug build, on an AddressSanitizer failure. Almost surely unrelated to your patch.

nyh · 2025-01-30T19:57:54Z

As I don't have access to read the jenkins logs, could I ask anyone responsible to let me know what was the reason for the failure? Thank you in advance.

This is cqlpy, cassandra_tests/validation/entities/secondary_index_test.py::testCreateAndDropIndexWithCamelCaseIdentifier the test crashed Scylla once in debug build, on an AddressSanitizer failure. Almost surely unrelated to your patch.

It appears to be the same as #22040. I'll update that issue with more information.

Jadw1 · 2025-01-31T09:55:05Z

Retriggered CI

scylladb-promoter · 2025-01-31T21:43:18Z

🔴 CI State: FAILURE

✅ - Build
✅ - Unit Tests Custom
The following new/updated tests ran 100 times for each mode:
🔹 boost/cql_functions_test
🔹 boost/cql_query_test
🔹 boost/expr_test
🔹 boost/json_cql_query_test
🔹 boost/types_test
🔹 boost/user_function_test
🔹 boost/user_types_test
🔹 boost/view_schema_test
❌ - dtest with topology changes
❌ - dtest with tablets
❌ - dtest
❌ - Unit Tests

Failed Tests (36/43900):

Build Details:

Duration: 11 hr
Builder: i-05bc3aef4e87c2b58 (m5ad.8xlarge)

Jadw1 · 2025-02-06T07:53:47Z

Failed tests look unrelated, some of them are known (like #20448), retriggered.

scylladb-promoter · 2025-02-06T17:59:52Z

🟢 CI State: SUCCESS

✅ - Build
✅ - Unit Tests Custom
The following new/updated tests ran 100 times for each mode:
🔹 boost/cql_functions_test
🔹 boost/cql_query_test
🔹 boost/expr_test
🔹 boost/json_cql_query_test
🔹 boost/types_test
🔹 boost/user_function_test
🔹 boost/user_types_test
🔹 boost/view_schema_test
✅ - dtest with topology changes
✅ - dtest with tablets
✅ - dtest
✅ - Unit Tests

Build Details:

Duration: 10 hr
Builder: i-0d778f093c96f4304 (m5ad.8xlarge)

avikivity · 2025-02-06T18:38:52Z

Queued

mykaul · 2025-02-07T17:03:39Z

docs/cql/types.rst

+For example, a ``vector<float, 3>`` holds vectors of 3 floats
+(in mathematical terms, these are 3-dimensional float vectors).
+None of the elements stored in a vector can be null.
+The vector type and it's respective literal are defined by:


it's -> its

QuerthDP requested review from nyh, piodul and tgrabiec as code owners January 24, 2025 17:54