You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "Beihao-Zhou (via GitHub)" <gi...@apache.org> on 2024/07/28 05:03:39 UTC
[PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Beihao-Zhou opened a new pull request, #2450:
URL: https://github.com/apache/kvrocks/pull/2450
This PR is part of #2426
## Summary
- [x] Hnsw Vector Redis Query Parser & Transformer (i.e. KNN & Range)
- [x] Hnsw Vector SQL Query Parser & Transformer (i.e. KNN & Range)
## Next
- [ ] Handle vector input (including blob and string representation)
## References
- [Redis Combined queries](https://redis.io/docs/latest/develop/interact/search-and-query/query/combined/#numeric-filter)
- [Pgvector SQL query](https://github.com/pgvector/pgvector?tab=readme-ov-file#querying)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697270841
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,12 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
Review Comment:
This was initially designed to accommodate future operators like `<=>` for cosine distance metrics (see [pgvector](https://github.com/pgvector/pgvector?tab=readme-ov-file#querying)). But upon reconsideration, I think it's probably unnecessary since distance metrics are typically specified during index creation, which doesn't allow users to alter them during queries.
Thanks for pointing this out; I'll remove the redundant code. <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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697958156
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
Ahh I've read the code in Transformer.
Seems it will be transformed into `VectorKnnExpr` rather than an `ORDER BY` with a vector literal?
Then I can get the idea, but I have several comments:
- Such a transformation may be better to put into an optimization pass instead of Transformer in the frontend,
- If we really want to put it inside the Transformer, maybe we can delete `vector` member in `SortByClause` (since it becomes a useless member after Transformer).
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694850941
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I'm wondering if we can introduce a new syntax of vector literals here, e.g.
```c++
struct VectorLiteral : seq<WSPad<one<'['>>, Number, star<seq<WSPad<one<','>>>, Number>, WSPad<one<']'>>> {};
```
And in the transformer you can collect all children node (typed Number) inside the VectorLiteral to construct a `std::vector<double>`
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699526576
##########
src/search/ir_sema_checker.h:
##########
@@ -60,8 +63,21 @@ struct SemaChecker {
return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
} else if (!iter->second.IsSortable()) {
return {Status::NotOK, fmt::format("field `{}` is not sortable", v->field->name)};
+ } else if (auto meta = iter->second.MetadataAs<redis::HnswVectorFieldMetadata>();
+ (meta != nullptr) != v->IsVectorField()) {
Review Comment:
```suggestion
} else if (auto isVectorField = iter->second.MetadataAs<redis::HnswVectorFieldMetadata>() != nullptr;
isVectorField != v->IsVectorField()) {
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697949863
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
> In the case that `ORDER BY vector_field <-> [3...] LIMIT n`, the syntax is guaranteed to be correct by parser.
Yeah but seems it will be rejected by SemaChecker.
Could you add this example into test cases of SemaChecker to confirm it?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699527573
##########
src/search/ir_sema_checker.h:
##########
@@ -60,8 +63,21 @@ struct SemaChecker {
return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
} else if (!iter->second.IsSortable()) {
return {Status::NotOK, fmt::format("field `{}` is not sortable", v->field->name)};
+ } else if (auto meta = iter->second.MetadataAs<redis::HnswVectorFieldMetadata>();
+ (meta != nullptr) != v->IsVectorField()) {
Review Comment:
current code is a little hard to read
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700306121
##########
src/search/ir.h:
##########
@@ -336,18 +393,30 @@ struct LimitClause : Node {
std::string Content() const override { return fmt::format("{}, {}", offset, count); }
std::unique_ptr<Node> Clone() const override { return std::make_unique<LimitClause>(*this); }
+ size_t Offset() const { return offset; }
+
+ size_t Count() const { return count; }
};
struct SortByClause : Node {
enum Order { ASC, DESC } order = ASC;
std::unique_ptr<FieldRef> field;
+ std::unique_ptr<VectorLiteral> vector = nullptr;
SortByClause(Order order, std::unique_ptr<FieldRef> &&field) : order(order), field(std::move(field)) {}
+ SortByClause(std::unique_ptr<FieldRef> &&field, std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), vector(std::move(vector)) {}
static constexpr const char *OrderToString(Order order) { return order == ASC ? "asc" : "desc"; }
+ bool IsVectorField() const { return vector != nullptr; }
std::string_view Name() const override { return "SortByClause"; }
- std::string Dump() const override { return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order)); }
+ std::string Dump() const override {
+ if (!IsVectorField()) {
+ return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order));
+ }
+ return fmt::format("sortby {} <-> {}", field->Dump(), vector->Dump());
Review Comment:
Nope, KNN executors always return the result from the nearest node to farthest.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694181810
##########
src/search/redis_query_transformer.h:
##########
@@ -141,11 +139,29 @@ struct Transformer : ir::TreeTransformer {
} else {
return std::make_unique<ir::AndExpr>(std::move(exprs));
}
+ } else if (Is<VectorRange>(query)) {
+ auto vector = GET_OR_RET(GetParam(query->children[2]));
+ return std::make_unique<VectorRangeExpr>(std::make_unique<FieldRef>(field),
+ GET_OR_RET(NumberOrParam(query->children[1])),
+ std::make_unique<VectorLiteral>(std::move(vector)));
Review Comment:
here the vector is in binary format, but in SQL it's text format.
I think we need to unify them for further processing.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1701023503
##########
src/search/sql_parser.h:
##########
@@ -84,7 +89,8 @@ struct Limit : string<'l', 'i', 'm', 'i', 't'> {};
struct WhereClause : seq<Where, QueryExpr> {};
struct AscOrDesc : sor<Asc, Desc> {};
-struct OrderByClause : seq<OrderBy, WSPad<Identifier>, opt<WSPad<AscOrDesc>>> {};
+struct OrderByExpr : sor<WSPad<VectorCompareExpr>, seq<WSPad<Identifier>, opt<WSPad<AscOrDesc>>>> {};
Review Comment:
Ahh got it.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): implement vector query for sql/redisearch parser & transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699530184
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
Great!
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694843305
##########
src/search/sql_transformer.h:
##########
@@ -57,7 +58,42 @@ StatusOr<std::unique_ptr<parse_tree::node>> ParseToTree(Input&& in) {
struct Transformer : ir::TreeTransformer {
explicit Transformer(const ParamMap& param_map) : TreeTransformer(param_map) {}
+ StatusOr<std::unique_ptr<VectorLiteral>> Transform2Vector(const TreeNode& node) {
+ std::string vector_str;
+ if (Is<StringL>(node)) {
+ vector_str = GET_OR_RET(UnescapeString(node->string_view()));
+ } else if (Is<Param>(node)) {
+ vector_str = GET_OR_RET(GetParam(node));
+ } else {
+ return {Status::NotOK, "encountered invalid vector input"};
+ }
+
+ if (vector_str.size() < 2 || vector_str[0] != '[' || vector_str.back() != ']') {
+ return {Status::NotOK, "encountered invalid vector input without brackets"};
+ }
+
+ std::vector<double> values = GET_OR_RET(ParseFloatArray(vector_str.substr(1, vector_str.size() - 2)));
Review Comment:
I believe we should standardize the parameter format here.
Regardless of the frontend language used, values of vector parameters should be represented in binary format.
This will make it easier to understand for users.
##########
src/search/sql_transformer.h:
##########
@@ -57,7 +58,42 @@ StatusOr<std::unique_ptr<parse_tree::node>> ParseToTree(Input&& in) {
struct Transformer : ir::TreeTransformer {
explicit Transformer(const ParamMap& param_map) : TreeTransformer(param_map) {}
+ StatusOr<std::unique_ptr<VectorLiteral>> Transform2Vector(const TreeNode& node) {
+ std::string vector_str;
+ if (Is<StringL>(node)) {
+ vector_str = GET_OR_RET(UnescapeString(node->string_view()));
+ } else if (Is<Param>(node)) {
+ vector_str = GET_OR_RET(GetParam(node));
+ } else {
+ return {Status::NotOK, "encountered invalid vector input"};
+ }
+
+ if (vector_str.size() < 2 || vector_str[0] != '[' || vector_str.back() != ']') {
+ return {Status::NotOK, "encountered invalid vector input without brackets"};
+ }
+
+ std::vector<double> values = GET_OR_RET(ParseFloatArray(vector_str.substr(1, vector_str.size() - 2)));
Review Comment:
I believe we should standardize the parameter format here, i.e. regardless of the frontend language used, values of vector parameters should be represented in binary format.
This will make it easier to understand for users.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696408551
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
For RediSearch part, we need to be compatible to the original implementation in Redis Stack. (which means we only need to support vector parameters in binary format, without any custom syntax).
But for SQL part, we can add our own syntax while trying to be similar to the syntax of MySQL. (so that we can support both vector parameters in binary format and also the syntax of vector literals).
There are many bad design in RediSearch and we can avoid them in SQL part (e.g. no vector literals).
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697864705
##########
src/search/ir.h:
##########
@@ -336,15 +393,22 @@ struct LimitClause : Node {
std::string Content() const override { return fmt::format("{}, {}", offset, count); }
std::unique_ptr<Node> Clone() const override { return std::make_unique<LimitClause>(*this); }
+ size_t Offset() const { return offset; }
+
+ size_t Count() const { return count; }
};
struct SortByClause : Node {
enum Order { ASC, DESC } order = ASC;
std::unique_ptr<FieldRef> field;
+ std::unique_ptr<VectorLiteral> vector = nullptr;
SortByClause(Order order, std::unique_ptr<FieldRef> &&field) : order(order), field(std::move(field)) {}
+ SortByClause(std::unique_ptr<FieldRef> &&field, std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), vector(std::move(vector)) {}
static constexpr const char *OrderToString(Order order) { return order == ASC ? "asc" : "desc"; }
+ bool IsKnn() const { return vector != nullptr; }
Review Comment:
```suggestion
bool IsVectorField() const { return vector != nullptr; }
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697949863
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
> In the case that `ORDER BY vector_field <-> [3...] LIMIT n`, the syntax is guaranteed to be correct by parser.
Yeah but seems it will be rejected by SemaChecker.
Could you add this example into test cases of sema checker confirm it?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700375220
##########
src/search/ir.h:
##########
@@ -336,18 +393,30 @@ struct LimitClause : Node {
std::string Content() const override { return fmt::format("{}, {}", offset, count); }
std::unique_ptr<Node> Clone() const override { return std::make_unique<LimitClause>(*this); }
+ size_t Offset() const { return offset; }
+
+ size_t Count() const { return count; }
};
struct SortByClause : Node {
enum Order { ASC, DESC } order = ASC;
std::unique_ptr<FieldRef> field;
+ std::unique_ptr<VectorLiteral> vector = nullptr;
SortByClause(Order order, std::unique_ptr<FieldRef> &&field) : order(order), field(std::move(field)) {}
+ SortByClause(std::unique_ptr<FieldRef> &&field, std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), vector(std::move(vector)) {}
static constexpr const char *OrderToString(Order order) { return order == ASC ? "asc" : "desc"; }
+ bool IsVectorField() const { return vector != nullptr; }
std::string_view Name() const override { return "SortByClause"; }
- std::string Dump() const override { return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order)); }
+ std::string Dump() const override {
+ if (!IsVectorField()) {
+ return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order));
+ }
+ return fmt::format("sortby {} <-> {}", field->Dump(), vector->Dump());
Review Comment:
hmm then maybe we can try `std::variant<Order, std::unique_ptr<VectorLiteral>>` to prevent misunderstanding since order will be useless for vector field?
##########
src/search/ir.h:
##########
@@ -336,18 +393,30 @@ struct LimitClause : Node {
std::string Content() const override { return fmt::format("{}, {}", offset, count); }
std::unique_ptr<Node> Clone() const override { return std::make_unique<LimitClause>(*this); }
+ size_t Offset() const { return offset; }
+
+ size_t Count() const { return count; }
};
struct SortByClause : Node {
enum Order { ASC, DESC } order = ASC;
std::unique_ptr<FieldRef> field;
+ std::unique_ptr<VectorLiteral> vector = nullptr;
SortByClause(Order order, std::unique_ptr<FieldRef> &&field) : order(order), field(std::move(field)) {}
+ SortByClause(std::unique_ptr<FieldRef> &&field, std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), vector(std::move(vector)) {}
static constexpr const char *OrderToString(Order order) { return order == ASC ? "asc" : "desc"; }
+ bool IsVectorField() const { return vector != nullptr; }
std::string_view Name() const override { return "SortByClause"; }
- std::string Dump() const override { return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order)); }
+ std::string Dump() const override {
+ if (!IsVectorField()) {
+ return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order));
+ }
+ return fmt::format("sortby {} <-> {}", field->Dump(), vector->Dump());
Review Comment:
hmm then maybe we can try `std::variant<Order, std::unique_ptr<VectorLiteral>>` to prevent misunderstanding since order will be useless for vector fields?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#issuecomment-2254825629
@PragmaTwice Ready for review now!
I'm not sure if there are better solutions for the unified abstraction to handle vector parsing. Let me know if you see any room of improvement! Thx! <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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699517196
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
+ return {Status::NotOK, "invalid knn search clause without limit"};
Review Comment:
```suggestion
return {Status::NotOK, "expect a LIMIT clause for vector field to construct a KNN search"};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696408551
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
For RediSearch part, we need to be compatible to the original implementation in Redis Stack. (which means we only need to support vector parameters in binary format, without any custom syntax).
But for SQL part, we can add our own syntax while trying to be similar to the syntax of MySQL. (so that we can support both vector parameters in binary format and also vector literals).
There are many bad design in RediSearch and we can avoid them in SQL part (e.g. no vector literals).
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697865419
##########
src/search/ir.h:
##########
@@ -356,6 +420,10 @@ struct SortByClause : Node {
std::unique_ptr<Node> Clone() const override {
return std::make_unique<SortByClause>(order, Node::MustAs<FieldRef>(field->Clone()));
}
+
+ std::unique_ptr<FieldRef> GetFieldRef() { return std::move(field); }
+
+ std::unique_ptr<VectorLiteral> GetVectorLiteral() { return std::move(vector); }
Review Comment:
usually "Get" implies that we don't modify the original object, but here `field` and `vector` will be nullptr after moving.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697958156
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
Ahh I've read the code in Transformer.
Seems it will be transformed into `VectorKnnExpr` rather than an `ORDER BY` with a vector literal?
Then I can get the idea, but I have several comments:
- Such a transformation may be better to put into an optimization pass instead of Transformer in the frontend,
- If we really want to put it inside the Transformer, maybe we can delete `vector` member in `SortByClause`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697201322
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,12 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
Review Comment:
seems `sor` here is useless?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697272807
##########
src/search/common_transformer.h:
##########
@@ -105,6 +105,46 @@ struct TreeTransformer {
return result;
}
+
+ template <typename T = double>
+ static StatusOr<std::vector<T>> Binary2Vector(std::string_view str) {
Review Comment:
Thanks for helping test it; will make the changes!
One thing is `std::bit_cast<T>` is supported until c++20, I may have to use `memcpy` instead
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700468942
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
I think we can allow range queries for noindex vector fields. But for KNN we don't need to support it for noindex fields.
For tag and numeric (and also vector), it depends on the query itself and the index selection pass to determine the query plan. e.g. we have a noindex numeric field `a` and non-noindex numeric field `b` in `idx`, the query plan of `select * from idx where a > 1 and b > 2` can be `filter a> 1 in (range-query b)`.
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
I think we can allow range queries for noindex vector fields. But for KNN we don't need to support it for noindex fields.
For tag and numeric (and also vector), it depends on the query itself and the index selection pass to determine the query plan. e.g. if we have a noindex numeric field `a` and non-noindex numeric field `b` in `idx`, the query plan of `select * from idx where a > 1 and b > 2` can be `filter a> 1 in (range-query b)`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700468942
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
I think we can allow range queries for noindex vector fields. But for KNN we don't need to support it for noindex fields.
For tag and numeric (and also vector), it depends on the query itself and index selection pass to determine the query plan. e.g. we have a noindex numeric field `a` and non-noindex numeric field `b` in `idx`, the query plan of `select * from idx where a > 1 and b > 2` can be `filter a> 1 in (range-query b)`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700440188
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
Just curious, is a `NOINDEX` vector field the only type of index that completely prohibits all query operations, such as KNN and range queries? For other field types, like tag and numeric filters, queries simply revert to a full scan right?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700468942
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
I think we can allow range queries for noindex vector fields. But for KNN we don't need to support it for noindex fields.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699527573
##########
src/search/ir_sema_checker.h:
##########
@@ -60,8 +63,21 @@ struct SemaChecker {
return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
} else if (!iter->second.IsSortable()) {
return {Status::NotOK, fmt::format("field `{}` is not sortable", v->field->name)};
+ } else if (auto meta = iter->second.MetadataAs<redis::HnswVectorFieldMetadata>();
+ (meta != nullptr) != v->IsVectorField()) {
Review Comment:
current code is a bit difficult to read
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699528808
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
maybe here we also need to consider if the current field is a `noindex` field?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694181810
##########
src/search/redis_query_transformer.h:
##########
@@ -141,11 +139,29 @@ struct Transformer : ir::TreeTransformer {
} else {
return std::make_unique<ir::AndExpr>(std::move(exprs));
}
+ } else if (Is<VectorRange>(query)) {
+ auto vector = GET_OR_RET(GetParam(query->children[2]));
+ return std::make_unique<VectorRangeExpr>(std::make_unique<FieldRef>(field),
+ GET_OR_RET(NumberOrParam(query->children[1])),
+ std::make_unique<VectorLiteral>(std::move(vector)));
Review Comment:
here the vector is in binary format, but in SQL it's text format.
We need to unify them.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696477564
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I see I see, finally get your point, thanks a lot!!
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696512629
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I could probably help with that after getting done most part of vector search, will take note of that on the tracking issues.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697866646
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
also in ORDER BY we need to check if the vector literal is provided when the field is typed vector.
otherwise `ORDER BY vector_field LIMIT n` is valid.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697871186
##########
src/search/ir.h:
##########
@@ -356,6 +420,10 @@ struct SortByClause : Node {
std::unique_ptr<Node> Clone() const override {
return std::make_unique<SortByClause>(order, Node::MustAs<FieldRef>(field->Clone()));
}
+
+ std::unique_ptr<FieldRef> GetFieldRef() { return std::move(field); }
+
+ std::unique_ptr<VectorLiteral> GetVectorLiteral() { return std::move(vector); }
Review Comment:
I see, will change it to `TakeFieldRef` and `TakeVectorLiteral`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697980495
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
Gotcha! I didn't think of putting it into an optimization pass, but it does make more sense to me; will implement this solution.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1698992663
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
Removed the transformation in the frontend; will add it in the next PR which will handle everything related to pass for hnsw vector search.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699514281
##########
src/search/ir.h:
##########
@@ -356,6 +425,10 @@ struct SortByClause : Node {
std::unique_ptr<Node> Clone() const override {
return std::make_unique<SortByClause>(order, Node::MustAs<FieldRef>(field->Clone()));
}
+
+ std::unique_ptr<FieldRef> TakeFieldRef() { return std::move(field); }
+
+ std::unique_ptr<VectorLiteral> TakeVectorLiteral() { return std::move(vector); }
Review Comment:
I prefer to delete them if they're currently not used.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700378418
##########
src/search/sql_parser.h:
##########
@@ -84,7 +89,8 @@ struct Limit : string<'l', 'i', 'm', 'i', 't'> {};
struct WhereClause : seq<Where, QueryExpr> {};
struct AscOrDesc : sor<Asc, Desc> {};
-struct OrderByClause : seq<OrderBy, WSPad<Identifier>, opt<WSPad<AscOrDesc>>> {};
+struct OrderByExpr : sor<WSPad<VectorCompareExpr>, seq<WSPad<Identifier>, opt<WSPad<AscOrDesc>>>> {};
Review Comment:
Also here seems it's allowed to pass an ASC/DESC after `VectorCompareExpr`. We can either prevent it in sql::Transformer or change the syntax definition here.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694177995
##########
src/search/ir.h:
##########
@@ -229,6 +229,61 @@ struct NumericCompareExpr : BoolAtomExpr {
}
};
+struct VectorLiteral : Literal {
+ std::string data;
Review Comment:
Hmm should it be a `std::vector<double>` instead of `std::string`?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700468942
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
I think we can allow range queries for noindex vector fields. But for KNN we don't need to support it for noindex fields.
For tag and numeric (and also vector), it depends on the query itself and the index selection pass to determine the query plan. e.g. if we have a noindex numeric field `a` and non-noindex numeric field `b` in `idx`, the query plan of `select * from idx where a > 1 and b > 2` can be `filter a > 1 in (range-query b)`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#issuecomment-2264307523
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2450) **Quality Gate failed**
Failed conditions
 [D Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2450) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2450)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1695407168
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
This looks good to me. Perhaps supporting both text and binary formats for SQL **AND** Redis would be more flexible, allowing users to choose the format that best suits their needs?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696499410
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
Also for the documentation, yeah we really need a separate documentation page for users to learn how to use Kvrocks Search and what the difference is between RediSearch and Kvrocks Search.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1695407168
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
This looks good to me. Perhaps supporting both text and binary formats for SQL **AND** Redis would be more flexible, allowing users to choose the format that best suits their needs?
```
struct BinaryOrVectorL : sor<StringL, VectorLiteral> {};
struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<BinaryOrVectorL>> {};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696408551
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
For RediSearch part, we need to be compatible to the original implementation in Redis Stack. (which means we only need to support vector parameters in binary format, without any custom syntax).
But for SQL part, we can add our own syntax while try to be similar to the syntax of MySQL. (so that we can support both vector parameters in binary format and also vector literals).
There are many bad design in RediSearch and we can avoid them in SQL part (e.g. no vector literals).
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696256156
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
It looks good, but I personally prefer not to use binary format in the syntax because it is more suited for programming interfaces and is not easily readable.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694177374
##########
src/search/ir.h:
##########
@@ -336,15 +391,22 @@ struct LimitClause : Node {
std::string Content() const override { return fmt::format("{}, {}", offset, count); }
std::unique_ptr<Node> Clone() const override { return std::make_unique<LimitClause>(*this); }
+ size_t GetOffset() const { return offset; }
+
+ size_t GetCount() const { return count; }
Review Comment:
```suggestion
size_t Offset() const { return offset; }
size_t Count() const { return count; }
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694831101
##########
src/search/redis_query_parser.h:
##########
@@ -30,6 +30,11 @@ namespace redis_query {
using namespace peg;
+struct VectorRangeKey : string<'V', 'E', 'C', 'T', 'O', 'R', '_', 'R', 'A', 'N', 'G', 'E'> {};
+struct KnnKey : string<'K', 'N', 'N'> {};
Review Comment:
```suggestion
struct VectorRangeToken : string<'V', 'E', 'C', 'T', 'O', 'R', '_', 'R', 'A', 'N', 'G', 'E'> {};
struct KnnToken : string<'K', 'N', 'N'> {};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699526576
##########
src/search/ir_sema_checker.h:
##########
@@ -60,8 +63,21 @@ struct SemaChecker {
return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
} else if (!iter->second.IsSortable()) {
return {Status::NotOK, fmt::format("field `{}` is not sortable", v->field->name)};
+ } else if (auto meta = iter->second.MetadataAs<redis::HnswVectorFieldMetadata>();
+ (meta != nullptr) != v->IsVectorField()) {
Review Comment:
```suggestion
} else if (auto is_vector = iter->second.MetadataAs<redis::HnswVectorFieldMetadata>() != nullptr;
is_vector != v->IsVectorField()) {
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700812232
##########
src/search/sql_parser.h:
##########
@@ -84,7 +89,8 @@ struct Limit : string<'l', 'i', 'm', 'i', 't'> {};
struct WhereClause : seq<Where, QueryExpr> {};
struct AscOrDesc : sor<Asc, Desc> {};
-struct OrderByClause : seq<OrderBy, WSPad<Identifier>, opt<WSPad<AscOrDesc>>> {};
+struct OrderByExpr : sor<WSPad<VectorCompareExpr>, seq<WSPad<Identifier>, opt<WSPad<AscOrDesc>>>> {};
Review Comment:
This is functionally correct since `opt<WSPad<AscOrDesc>>` is wrapped into`seq<WSPad<Identifier>, [...]>` but it is unreadable, so made it to the following and add corresponding test
```
struct SortableFieldExpr : seq<WSPad<Identifier>, opt<AscOrDesc>> {};
struct OrderByExpr : sor<WSPad<VectorCompareExpr>, WSPad<SortableFieldExpr>> {};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694850941
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I'm wondering if we can introduce a new syntax of vector literals here, e.g.
```c++
struct VectorLiteral
: seq<WSPad<one<'['>>, Number, star<seq<WSPad<one<','>>>, Number>, WSPad<one<']'>>> {};
```
And in the transformer you can collect all children node (typed `Number`) inside the `VectorLiteral` to construct a `std::vector<double>`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699525441
##########
src/search/ir.h:
##########
@@ -336,18 +393,30 @@ struct LimitClause : Node {
std::string Content() const override { return fmt::format("{}, {}", offset, count); }
std::unique_ptr<Node> Clone() const override { return std::make_unique<LimitClause>(*this); }
+ size_t Offset() const { return offset; }
+
+ size_t Count() const { return count; }
};
struct SortByClause : Node {
enum Order { ASC, DESC } order = ASC;
std::unique_ptr<FieldRef> field;
+ std::unique_ptr<VectorLiteral> vector = nullptr;
SortByClause(Order order, std::unique_ptr<FieldRef> &&field) : order(order), field(std::move(field)) {}
+ SortByClause(std::unique_ptr<FieldRef> &&field, std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), vector(std::move(vector)) {}
static constexpr const char *OrderToString(Order order) { return order == ASC ? "asc" : "desc"; }
+ bool IsVectorField() const { return vector != nullptr; }
std::string_view Name() const override { return "SortByClause"; }
- std::string Dump() const override { return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order)); }
+ std::string Dump() const override {
+ if (!IsVectorField()) {
+ return fmt::format("sortby {}, {}", field->Dump(), OrderToString(order));
+ }
+ return fmt::format("sortby {} <-> {}", field->Dump(), vector->Dump());
Review Comment:
Is ASC DESC useful in KNN search?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1700378418
##########
src/search/sql_parser.h:
##########
@@ -84,7 +89,8 @@ struct Limit : string<'l', 'i', 'm', 'i', 't'> {};
struct WhereClause : seq<Where, QueryExpr> {};
struct AscOrDesc : sor<Asc, Desc> {};
-struct OrderByClause : seq<OrderBy, WSPad<Identifier>, opt<WSPad<AscOrDesc>>> {};
+struct OrderByExpr : sor<WSPad<VectorCompareExpr>, seq<WSPad<Identifier>, opt<WSPad<AscOrDesc>>>> {};
Review Comment:
Also here seems it's allowed to pass an ASC/DESC after `VectorCompareExpr`. We can either prevent it in sql::Transformer or change the syntax definition.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694281012
##########
src/search/redis_query_transformer.h:
##########
@@ -141,11 +139,29 @@ struct Transformer : ir::TreeTransformer {
} else {
return std::make_unique<ir::AndExpr>(std::move(exprs));
}
+ } else if (Is<VectorRange>(query)) {
+ auto vector = GET_OR_RET(GetParam(query->children[2]));
+ return std::make_unique<VectorRangeExpr>(std::make_unique<FieldRef>(field),
+ GET_OR_RET(NumberOrParam(query->children[1])),
+ std::make_unique<VectorLiteral>(std::move(vector)));
Review Comment:
Oh right, this is the todo item left for this PR. I realized I forgot to leave some comments or notes about it—my bad. I’m currently wrapping up the ParseArray functions into parse_util.h. This update should make them more abstract and reusable. I’m aiming to push the new revision soon!
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1695407168
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
This looks good to me. Perhaps supporting both text and binary formats for **SQL AND Redis** would be more flexible, allowing users to choose the format that best suits their needs?
```
struct BinaryOrVectorL : sor<StringL, VectorLiteral> {};
struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<BinaryOrVectorL>> {};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694850941
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I'm wondering if we can introduce a new syntax of vector literals here, e.g.
```c++
struct VectorLiteral
: seq<WSPad<one<'['>>, Number, star<seq<WSPad<one<','>>>, Number>, WSPad<one<']'>>> {};
```
And in the transformer you can collect all children node (typed Number) inside the VectorLiteral to construct a `std::vector<double>`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696268229
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I also personally prefer to only support text, but we need to document it in the future, as this is different from Redis vector search which only supports binary format.
Will implement `VectorLiteral` and release the next revision very soon!
@PragmaTwice I'm also considering whether we should stick to Redis's protocol, which requires vectors to be passed in as parameters, or if we should parse VectorLiterals directly, like "*=>[KNN 10 @doc_embedding [1,2,3]]". Personally, I prefer the latter approach because it makes the query transform layer more intuitive. Let me know it this looks good to you
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696926532
##########
src/search/common_transformer.h:
##########
@@ -105,6 +105,46 @@ struct TreeTransformer {
return result;
}
+
+ template <typename T = double>
+ static StatusOr<std::vector<T>> Binary2Vector(std::string_view str) {
Review Comment:
Hmm I tried some simple queries on Redis Stack, e.g.
```
FT.SEARCH documents "*=>[KNN 10 @doc_embedding $BLOB]" PARAMS 2 BLOB "\x12\xa9\xf5\x6c" SORTBY __vector_score DIALECT 4
```
Seems here the parameter `"\x12\xa9\xf5\x6c"` is a redis-cli level string literal and it will be converted to binary format (i.e. a 4-byte binary string) before it is sent to the redis server.
So maybe in the server side we can just do some bit-level casts.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697863612
##########
src/search/ir.h:
##########
@@ -229,6 +229,63 @@ struct NumericCompareExpr : BoolAtomExpr {
}
};
+struct VectorLiteral : Literal {
+ std::vector<double> values;
+
+ explicit VectorLiteral(std::vector<double> &&values) : values(std::move(values)){};
+
+ std::string_view Name() const override { return "VectorLiteral"; }
+ std::string Dump() const override {
+ return fmt::format("[{}]", util::StringJoin(values, [](auto v) { return std::to_string(v); }));
+ }
+ std::string Content() const override { return Dump(); }
+
+ std::unique_ptr<Node> Clone() const override { return std::make_unique<VectorLiteral>(*this); }
+};
+
+struct VectorRangeExpr : BoolAtomExpr {
+ std::unique_ptr<FieldRef> field;
+ std::unique_ptr<NumericLiteral> range;
+ std::unique_ptr<VectorLiteral> vector;
+
+ VectorRangeExpr(std::unique_ptr<FieldRef> &&field, std::unique_ptr<NumericLiteral> &&range,
+ std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), range(std::move(range)), vector(std::move(vector)) {}
+
+ std::string_view Name() const override { return "VectorRangeExpr"; }
+ std::string Dump() const override {
+ return fmt::format("{} vector_range {} {}", field->Dump(), range->Dump(), vector->Dump());
+ }
+
+ std::unique_ptr<Node> Clone() const override {
+ return std::make_unique<VectorRangeExpr>(Node::MustAs<FieldRef>(field->Clone()),
+ Node::MustAs<NumericLiteral>(range->Clone()),
+ Node::MustAs<VectorLiteral>(vector->Clone()));
+ }
+};
+
+struct VectorSearchExpr : BoolAtomExpr {
Review Comment:
Would "VectorKnnExpr" be a more suitable name here?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697862828
##########
src/search/ir.h:
##########
@@ -229,6 +229,63 @@ struct NumericCompareExpr : BoolAtomExpr {
}
};
+struct VectorLiteral : Literal {
+ std::vector<double> values;
+
+ explicit VectorLiteral(std::vector<double> &&values) : values(std::move(values)){};
+
+ std::string_view Name() const override { return "VectorLiteral"; }
+ std::string Dump() const override {
+ return fmt::format("[{}]", util::StringJoin(values, [](auto v) { return std::to_string(v); }));
+ }
+ std::string Content() const override { return Dump(); }
+
+ std::unique_ptr<Node> Clone() const override { return std::make_unique<VectorLiteral>(*this); }
+};
+
+struct VectorRangeExpr : BoolAtomExpr {
+ std::unique_ptr<FieldRef> field;
+ std::unique_ptr<NumericLiteral> range;
+ std::unique_ptr<VectorLiteral> vector;
+
+ VectorRangeExpr(std::unique_ptr<FieldRef> &&field, std::unique_ptr<NumericLiteral> &&range,
+ std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), range(std::move(range)), vector(std::move(vector)) {}
+
+ std::string_view Name() const override { return "VectorRangeExpr"; }
+ std::string Dump() const override {
+ return fmt::format("{} vector_range {} {}", field->Dump(), range->Dump(), vector->Dump());
Review Comment:
```suggestion
return fmt::format("{} <-> {} < {}", field->Dump(), vector->Dump(), range->Dump());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1694836926
##########
src/search/redis_query_transformer.h:
##########
@@ -53,7 +53,32 @@ StatusOr<std::unique_ptr<parse_tree::node>> ParseToTree(Input&& in) {
struct Transformer : ir::TreeTransformer {
explicit Transformer(const ParamMap& param_map) : TreeTransformer(param_map) {}
+ StatusOr<std::unique_ptr<VectorLiteral>> Transform2Vector(const TreeNode& node) {
+ std::string vector_str = GET_OR_RET(GetParam(node));
+
+ auto vector_chars = GET_OR_RET(Binary2Chars(vector_str));
+ std::vector<double> values = GET_OR_RET(CharsToVector<double>(vector_chars));
Review Comment:
I think we can just create a function to convert the binary data to `std::vector<double>` since it's more intuitive.
Also in the SemaChecker we need to check if the length of the vector is valid.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696268229
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
I also personally prefer to only support text, but we need to document it in the future, as this is different from Redis vector search which only supports binary format.
Will implement `VectorLiteral` and release the next revision very soon!
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699528808
##########
src/search/ir_sema_checker.h:
##########
@@ -50,6 +50,9 @@ struct SemaChecker {
GET_OR_RET(Check(v->query_expr.get()));
if (v->limit) GET_OR_RET(Check(v->limit.get()));
if (v->sort_by) GET_OR_RET(Check(v->sort_by.get()));
+ if (v->sort_by && v->sort_by->IsVectorField() && !v->limit) {
Review Comment:
maybe here we also need to consider if the current field is a `noindex` field? (i.e. no index is constructed for such a field, just used in filtering)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697923172
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
Double checked this case with `select f4 from ia order by f4 limit 5`. In the case that query only gives a field, it will go through
```
if (auto v = dynamic_cast<SortByClause *>(node)) {
// ...
if (!iter->second.IsSortable()) {
return {Status::NotOK, fmt::format("field `{}` is not sortable", v->field->name)};
}
}
```
So this wouldn't be a problem for vector type in particular, as `SortByClause` will invalidate all non-numeric literal.
In the case that `ORDER BY vector_field <-> [3...] LIMIT n`, the syntax is guaranteed to be correct by parser.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696926532
##########
src/search/common_transformer.h:
##########
@@ -105,6 +105,46 @@ struct TreeTransformer {
return result;
}
+
+ template <typename T = double>
+ static StatusOr<std::vector<T>> Binary2Vector(std::string_view str) {
Review Comment:
Hmm I tried some simple queries on Redis Stack, e.g.
```
FT.SEARCH documents "*=>[KNN 10 @doc_embedding $BLOB]" PARAMS 2 BLOB "\x12\xa9\xf5\x6c" SORTBY __vector_score DIALECT 4
```
Seems here the parameter `"\x12\xa9\xf5\x6c"` is a redis-cli level string literal and it will be converted to binary format (i.e. a 4-byte binary string) before it is sent to the redis server.
So maybe in the server side we can just do some bit-level casts, e.g. `std::bit_cast<double>(first_8_bytes_of_str)`
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1697866646
##########
src/search/ir_sema_checker.h:
##########
@@ -97,6 +97,42 @@ struct SemaChecker {
} else {
v->field->info = &iter->second;
}
+ } else if (auto v = dynamic_cast<VectorSearchExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
+ } else if (auto v = dynamic_cast<VectorRangeExpr *>(node)) {
+ if (auto iter = current_index->fields.find(v->field->name); iter == current_index->fields.end()) {
+ return {Status::NotOK, fmt::format("field `{}` not found in index `{}`", v->field->name, current_index->name)};
+ } else if (!iter->second.MetadataAs<redis::HnswVectorFieldMetadata>()) {
+ return {Status::NotOK, fmt::format("field `{}` is not a vector field", v->field->name)};
+ } else {
+ v->field->info = &iter->second;
+
+ auto meta = v->field->info->MetadataAs<redis::HnswVectorFieldMetadata>();
+ if (meta->distance_metric == redis::DistanceMetric::L2 && v->range->val < 0) {
+ return {Status::NotOK, "range cannot be a negative number for l2 distance metric"};
+ }
+
+ if (meta->distance_metric == redis::DistanceMetric::COSINE && (v->range->val < 0 || v->range->val > 2)) {
+ return {Status::NotOK, "range has to be between 0 and 2 for cosine distance metric"};
+ }
+
+ if (v->vector->values.size() != meta->dim) {
+ return {Status::NotOK,
+ fmt::format("vector should be of size `{}` for field `{}`", meta->dim, v->field->name)};
+ }
+ }
Review Comment:
also in ORDER BY we need to check if the vector literal is provided when the field is typed vector.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1696499410
##########
src/search/sql_parser.h:
##########
@@ -41,7 +41,11 @@ struct NumericAtomExpr : WSPad<sor<NumberOrParam, Identifier>> {};
struct NumericCompareOp : sor<string<'!', '='>, string<'<', '='>, string<'>', '='>, one<'=', '<', '>'>> {};
struct NumericCompareExpr : seq<NumericAtomExpr, NumericCompareOp, NumericAtomExpr> {};
-struct BooleanAtomExpr : sor<HasTagExpr, NumericCompareExpr, WSPad<Boolean>> {};
+struct VectorCompareOp : sor<string<'<', '-', '>'>> {};
+struct VectorCompareExpr : seq<WSPad<Identifier>, VectorCompareOp, WSPad<StringOrParam>> {};
Review Comment:
Also for the documentation, yeah we really need a separate documentation page for users to learn how to use Kvrocks Search and what the difference is between RediSearch and Kvrocks Search.
Currently it's fine since we haven't release Kvrocks Search. Also I didn't find any spare time to create the page.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699522995
##########
src/search/ir.h:
##########
@@ -229,6 +229,63 @@ struct NumericCompareExpr : BoolAtomExpr {
}
};
+struct VectorLiteral : Literal {
+ std::vector<double> values;
+
+ explicit VectorLiteral(std::vector<double> &&values) : values(std::move(values)){};
+
+ std::string_view Name() const override { return "VectorLiteral"; }
+ std::string Dump() const override {
+ return fmt::format("[{}]", util::StringJoin(values, [](auto v) { return std::to_string(v); }));
+ }
+ std::string Content() const override { return Dump(); }
+
+ std::unique_ptr<Node> Clone() const override { return std::make_unique<VectorLiteral>(*this); }
+};
+
+struct VectorRangeExpr : BoolAtomExpr {
+ std::unique_ptr<FieldRef> field;
+ std::unique_ptr<NumericLiteral> range;
+ std::unique_ptr<VectorLiteral> vector;
+
+ VectorRangeExpr(std::unique_ptr<FieldRef> &&field, std::unique_ptr<NumericLiteral> &&range,
+ std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), range(std::move(range)), vector(std::move(vector)) {}
+
+ std::string_view Name() const override { return "VectorRangeExpr"; }
+ std::string Dump() const override {
+ return fmt::format("{} <-> {} < {}", field->Dump(), vector->Dump(), range->Dump());
+ }
+
+ std::unique_ptr<Node> Clone() const override {
+ return std::make_unique<VectorRangeExpr>(Node::MustAs<FieldRef>(field->Clone()),
+ Node::MustAs<NumericLiteral>(range->Clone()),
+ Node::MustAs<VectorLiteral>(vector->Clone()));
+ }
+};
+
+struct VectorKnnExpr : BoolAtomExpr {
+ // TODO: Support pre-filter for hybrid query
+ std::unique_ptr<FieldRef> field;
+ std::unique_ptr<NumericLiteral> k;
+ std::unique_ptr<VectorLiteral> vector;
+
+ VectorKnnExpr(std::unique_ptr<FieldRef> &&field, std::unique_ptr<NumericLiteral> &&k,
+ std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), k(std::move(k)), vector(std::move(vector)) {}
+
+ std::string_view Name() const override { return "VectorKnnExpr"; }
+ std::string Dump() const override {
+ return fmt::format("{} <-> {} knn {}", field->Dump(), vector->Dump(), k->Dump());
Review Comment:
```suggestion
return fmt::format("KNN(k={}) {} <-> {}", k->Dump(), field->Dump(), vector->Dump());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(search): Hnsw Vector Query Parser & Transformer [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2450:
URL: https://github.com/apache/kvrocks/pull/2450#discussion_r1699522995
##########
src/search/ir.h:
##########
@@ -229,6 +229,63 @@ struct NumericCompareExpr : BoolAtomExpr {
}
};
+struct VectorLiteral : Literal {
+ std::vector<double> values;
+
+ explicit VectorLiteral(std::vector<double> &&values) : values(std::move(values)){};
+
+ std::string_view Name() const override { return "VectorLiteral"; }
+ std::string Dump() const override {
+ return fmt::format("[{}]", util::StringJoin(values, [](auto v) { return std::to_string(v); }));
+ }
+ std::string Content() const override { return Dump(); }
+
+ std::unique_ptr<Node> Clone() const override { return std::make_unique<VectorLiteral>(*this); }
+};
+
+struct VectorRangeExpr : BoolAtomExpr {
+ std::unique_ptr<FieldRef> field;
+ std::unique_ptr<NumericLiteral> range;
+ std::unique_ptr<VectorLiteral> vector;
+
+ VectorRangeExpr(std::unique_ptr<FieldRef> &&field, std::unique_ptr<NumericLiteral> &&range,
+ std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), range(std::move(range)), vector(std::move(vector)) {}
+
+ std::string_view Name() const override { return "VectorRangeExpr"; }
+ std::string Dump() const override {
+ return fmt::format("{} <-> {} < {}", field->Dump(), vector->Dump(), range->Dump());
+ }
+
+ std::unique_ptr<Node> Clone() const override {
+ return std::make_unique<VectorRangeExpr>(Node::MustAs<FieldRef>(field->Clone()),
+ Node::MustAs<NumericLiteral>(range->Clone()),
+ Node::MustAs<VectorLiteral>(vector->Clone()));
+ }
+};
+
+struct VectorKnnExpr : BoolAtomExpr {
+ // TODO: Support pre-filter for hybrid query
+ std::unique_ptr<FieldRef> field;
+ std::unique_ptr<NumericLiteral> k;
+ std::unique_ptr<VectorLiteral> vector;
+
+ VectorKnnExpr(std::unique_ptr<FieldRef> &&field, std::unique_ptr<NumericLiteral> &&k,
+ std::unique_ptr<VectorLiteral> &&vector)
+ : field(std::move(field)), k(std::move(k)), vector(std::move(vector)) {}
+
+ std::string_view Name() const override { return "VectorKnnExpr"; }
+ std::string Dump() const override {
+ return fmt::format("{} <-> {} knn {}", field->Dump(), vector->Dump(), k->Dump());
Review Comment:
```suggestion
return fmt::format("KNN k={}, {} <-> {}", k->Dump(), field->Dump(), vector->Dump());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org