Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reindexer v5: support KNN index #22

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

AlexAUtkin
Copy link
Contributor

  • Fix build for Reindexer v5
  • Add support for KNN: Query.where_knn and index search params
  • Update readme
  • Add new test for search in KNN index
  • Add examples for using KNN index (where_knn and select(query))

transaction.commit(timedelta(seconds = 1))

# do query
param = IndexSearchParamHnsw(44, 500)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Предлагаю тут более осмысленные параметры сделать - пример всё-таки. 'K' что-то типа 20, а ef, например, 30. Сейчас у тебя ef=500 при общем размере индекса 100, что означает, по сути, полное сканирование графа.

Ну и ещё субъективный момент: лично мне было бы понятнее с именованными параметрами. IndexSearchParamHnsw(k=20, ef=30)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
IDE в помощь, но поправлю, для тех кто в блокноте разрабатывает.
Изменил

[More](https://github.com/Restream/reindexer/blob/master/fulltext.md).
config (dict): A config for a fulltext and float_vector engine.
[More about `fulltext`](https://github.com/Restream/reindexer/blob/master/fulltext.md) or
[More about `float_vector`](https://github.com/Restream/reindexer/blob/master/cpp_src/float_vector.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Напоминалка: тут нужно будет не забыть перед мёрджем правильный путь указать, когда мы его скорректируем


"""

def __init__(self, k: int, ef: int):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ef можно опциональным сделать и проставлять туда 1.5 * k по дефолту. Это чуток упростит интерфейс для пользователя - в отличие от IVF тут так можно

Copy link
Contributor Author

@AlexAUtkin AlexAUtkin Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float * int и сохранить в int. Ну, кроме того, что k так использовать нельзя: Unresolved reference 'k'.
Осталось как было. Дописал комментарий

return reindexer::KnnSearchParams::Hnsw(k, ef);
}
if (nprobe > 0) {
return reindexer::KnnSearchParams::Ivf(k, nprobe);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лишний пробел после return. Как будто форматтер не отработал

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Убрал

auto vect = reindexer::FloatVector::CreateNotInitialized(reindexer::FloatVectorDimension(vals.size()));
size_t pos = 0;
for (const auto& value : vals) {
vect.RawData()[pos] = value.As<double>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как будто указатель из RawData() лучше прихранить за пределами цикла. Там внутри, как я вижу, assertrx(!IsEmpty()), и не факт, что тут компилятор сможет сам лишнюю проверку выкинуть из каждой итерации. Надёжнее, на мой взгляд, подстраховаться

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

копия с родительского проекта (vect.RawData()[pos] = p->via.f64;). Поправил

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants