PS-11203 Vector Index Syntax - #5987
Conversation
c055ddc to
bac48eb
Compare
bac48eb to
6f38434
Compare
a985587 to
96481cb
Compare
3c65f43 to
67722d7
Compare
09426a6 to
b22f12a
Compare
| eng "Vector index can only be created in tables with a BIGINT UNSIGNED primary key." | ||
|
|
||
| ER_ONLY_SINGLE_VECTOR_INDEX_ALLOWED | ||
| eng "A table can have at most one vector index." |
There was a problem hiding this comment.
why is this the case? I've seen examples where having two indexed vector columns make a lot of sense. Is this only for the MVP?
There was a problem hiding this comment.
Was not documented properly, but now is:
| inline constexpr const decltype(handlerton::flags) | ||
| HTON_SECONDARY_SUPPORTS_TEMPORARY_TABLE(1 << 25); | ||
|
|
||
|
|
There was a problem hiding this comment.
please revert these changes to the end of the file as they are not related to this commit
There was a problem hiding this comment.
Good catch. These were actually part of the underlying PR, so I'll update that one, too.
| ); | ||
| SHOW CREATE TABLE t1; | ||
| SHOW INDEXES FROM t1; | ||
|
|
There was a problem hiding this comment.
I think we don't have any test cases where you try creating a VECTOR index on a table without BIGINT UNSIGNED, also one where you create the table with such primary key but then you alter the table by droping it or perhaps change data type(though I think the second part of this idea should not be possible)
There was a problem hiding this comment.
The latter I have: https://github.com/percona/percona-server/pull/5987/changes#diff-5daf5ca2072c4bcd935fb51611868b7a7a550bd8b541804f20556541446216e0R18-R19
Added the rest.
b22f12a to
fa59cb0
Compare
| } | ||
| hnsw_param.M = std::atoi(p.value.str); | ||
| } else if (my_strcasecmp(system_charset_info, p.key.str, "metric") == 0) { | ||
| if (my_strcasecmp(system_charset_info, p.value.str, "euclidean") == 0) { |
There was a problem hiding this comment.
how will this tie into the distance functions? does it need to support all the metrics?
There was a problem hiding this comment.
I also wonder what visibility do we have from the optimizer if an index is defined on one metric and we query it using distance function using another metric. Shouldn't this be more "static" since there are only a few available options?
There was a problem hiding this comment.
Indeed. I changed the code to make it more obvious how to add new metrics. If you have a list of metrics already, feel free to drop them!
Not sure what you mean by static? You mean coded into the Bison parser?
| } | ||
| ; | ||
|
|
||
| index_construction_parameter: |
There was a problem hiding this comment.
I wonder if it's a good idea to expect IDENT_QUOTED instead of IDENT. As far as I know the former accepts non-ASCII bytes/multi-byte. This should be a simple config and I don't expect anything "exotic" here, it's easier to start with something restricted, and relax requirements later
There was a problem hiding this comment.
There's some ugly lexer hack around IDENT, I never managed to get it to work
mysql> CREATE TABLE t1 ( id BIGINT UNSIGNED PRIMARY KEY, v1 VECTOR( 1234 ), VECTOR KEY( v1 ) TYPE hnsw WITH ( m=a ) );
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'm=a ) )' at line 1
The rest of the string is eaten up by the IDENT above
There was a problem hiding this comment.
Changing to ident for now. That seems to be what people do
fa59cb0 to
c044c64
Compare
| | XML_SYM | ||
| | YEAR_SYM | ||
| | ZONE_SYM | ||
| | VECTOR_SYM |
There was a problem hiding this comment.
Is this necessary? this makes VECTOR a reserved keyword so this will break existing schema using this name
There was a problem hiding this comment.
I think it will be almost impossible to support the VECTOR INDEX syntax we have without it. Currently it's perfectly legal to define a column named vector:
CREATE TABLE t ( ..., vector char(1) )
Heck, you can even do CREATE TABLE t ( ..., vector VECTOR(1) )!
We want to support the syntax to create an in-line vector definition
CREATE TABLE t ( ..., vector index ix (col) )
You can probably see where this is going.
We have these options afaiu:
- Re-shuffle the rules so we never reduce a until we have the full set of items. Only then can we know whether it's a column definition or an index definition
- Not do the
VECTOR INDEXsyntax at all, onlyINDEXand let typeTYPE/USINGclause decide. Fwiw, this seems to be what Oracle are considering. - Do as above and make
VECTORa properly reserved word - We could also in theory create another category of reserved words which can be used as anything but column names :P
| key_create_info.comment = key_info->comment; | ||
|
|
||
| if (key_info->vector_index_type.str != nullptr) | ||
| key_create_info.vector_index_type = key_info->vector_index_type; |
There was a problem hiding this comment.
I think this where the other parameters of the index config gets lots and triggers this bug:
CREATE TABLE t1 (
id BIGINT UNSIGNED PRIMARY KEY,
v1 VECTOR( 8 ),
VECTOR KEY( v1 ) TYPE hnsw WITH ( M = 6, metric = euclidean )
);
SHOW CREATE TABLE t1;
# Unrelated ALTER
ALTER TABLE t1 ADD COLUMN c INT;
# see how the parameters gets lost
SHOW CREATE TABLE t1;
There was a problem hiding this comment.
Ouch. This was a particularly gnarly issue. It made me re-implement where the whole serialization and de-serialization happens. But it's probably cleaner this way.
| assert((key_info->flags & flags_before_switch) == flags_before_switch); | ||
| if (key->generated) key_info->flags |= HA_GENERATED_KEY; | ||
|
|
||
| // Serialize vector index construction params (WITH clause). |
There was a problem hiding this comment.
CREATE TABLE t2 ( a INT, KEY k ( a ) TYPE hnsw WITH ( M = 6 ) ); should not be accepted
There was a problem hiding this comment.
Isn't now 😄
mysql> CREATE TABLE t2 ( a INT, KEY k ( a ) TYPE hnsw WITH ( M = 6 ) );
ERROR 7034 (HY000): A VECTOR index may only contain a vector type column.
c044c64 to
475d45c
Compare
96b9e15 to
f857287
Compare
Vector indexes have the type (algorithm) SE_SPECIFIC, and we add a column option in the data dictionary saying `vector_index=1;` which gets picked up by dedicated code in the data dictionary and the handler part of InnoDB. In the SQL layer, the vector index is very much a thing; there is an `HA_KEY_ALG_VECTOR`, an `HA_VECTOR` and a `KEYTYPE_VECTOR`. Extra SQL is added to display the type of a vector index as VECTOR rather than SE_SPECIFIC.
Adding the syntax `TYPE <ident> WITH ( <ident> = <ident>...) ` to index creation syntax. E.g.: `CREATE INDEX <name> ( <table> ) TYPE hnsw WITH ( M = 6 )` The syntax in the `WITH` list, christened Index Construction Parameters in this commit, must be verified by the storage engine. There were no hooks for this in InnoDB so one has been added in check_engine(). We have to do it fairly early so that we can prevent table creation in the DD in case of errors. Hence, the index type and parameter list are validated inside the storage engine using a new interface validate_engine_attributes(). The index construction parameters are serialized as a string of key-value pairs inside the index's `option` field. The serialization and de-serialization happen entirely inside the Data Dictionary. To do: We will probably still need a hook to handle the case of an already-existing table with invalid attributes; there are no hooks for this.
f857287 to
739ac40
Compare
Stacked on #6000