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

ID issue #156

Closed
vsly-ru opened this issue Mar 26, 2023 · 5 comments · Fixed by #157
Closed

ID issue #156

vsly-ru opened this issue Mar 26, 2023 · 5 comments · Fixed by #157

Comments

@vsly-ru
Copy link

vsly-ru commented Mar 26, 2023

As stated in the readme:

Every document in mimir needs to have a field ending in id (or simply just id)
If you have multiple fields ending in id, please open an issue so we can discuss

I have id field, but also categoryId, rootId, createdById, etc.
In my case it'd be enough if mimir first checked for an id field and only then continues search for fields ending in id.
But for configurability reasons it's better to clearly specify id field during documents import.

@GregoryConrad
Copy link
Owner

Hi! 👋

Thanks for raising this issue. Didn't realize anyone would have this issue this soon!

Had to refresh my memory on how to set a primary key (id field) manually in milli; looks like it is done via a settings update. Thus, the way I need go about implementing this at a basic level would be to add a primaryKey field to MimirIndexSettings:

final instance = await Mimir.defaultInstance;
final index = instance.getIndex('movies');
await index.updateSettings(primaryKey: 'id');

That is easy enough, but I would want to also do (as a syntactic simplification):

final instance = await Mimir.defaultInstance;
final index = instance.getIndex('movies', primaryKey: 'id');

But getIndex is synchronous (because all operations are completed lazily) and I would like to avoid changing that behavior. To avoid making it async, I would then need to call updateSettings with the specified pkey in every add/set documents call. That's added overhead at runtime and not as maintainable since it'd be easy to forget that updateSettings call in future changes.

Perhaps I could come to some sort of middle ground, such as:

final instance = await Mimir.defaultInstance;
final index = await instance.openIndexWithSettings('movies', primaryKey: 'id', someOtherSetting: true);

This would open a MimirIndex eagerly and update all the supplied settings before returning the index.

How does that sound? Any suggestions for a better method name (I'm not completely sold on openIndexWithSettings--maybe just openIndex)?

Note: changing the pkey after some initial documents are added may result in an error, but I am not sure. That is part of the reason I originally didn't add the feature.

@GregoryConrad
Copy link
Owner

GregoryConrad commented Mar 26, 2023

Thanks for bringing this up when you did; found a slight hiccup in milli that I'll need to make a PR for (you wouldn't be able to reopen a MimirIndex with a manually set pkey). First stable version of mimir might be delayed a couple months due to this unfortunately though.

@vsly-ru
Copy link
Author

vsly-ru commented Mar 26, 2023

Didn't realize anyone would have this issue this soon!

Mimir is really unique and demanded in my opinion! And it's exactly what I need. Even I've used Isar before(and other libs) and even this one is in dev phase, I picked it instead of Isar for search because search is so much powerful out of the box.
Isar is a pain to setup and use properly (split query by words and search each one separately, and then somehow sort by "relevance" which is not calculated for you). So thank you very much for your work!

await index.updateSettings(primaryKey: 'id');

This looks okay. For (rare?) cases when the default behaviour is not working.
But you can also simply predefine a reserved pk field, e.g. id (or _id as in MongoDB) and in cases specified primaryKey key is different, use it to assign its value to the predefined pk field.
If I understood the issue correctly, this could help with pk ambiguity problem.

@GregoryConrad
Copy link
Owner

GregoryConrad commented Mar 26, 2023

You can take a peek at #157, which hopefully will be done sometime tomorrow. I'll try to publish a new dev release shortly thereafter. Mimir wraps around milli (part of meilisearch) under the hood and milli looks for fields ending in "id" to set the PK, which is where your ambiguity issue comes from.

TL;DR You will be able to do:

final instance = await Mimir.defaultInstance;
final index = await instance.openIndex('movies', primaryKey: 'id');

But to maintain backwards compatibility, you can also do:

final instance = await Mimir.defaultInstance;
final index = instance.getIndex('movies');
await index.updateSettings(primaryKey: 'id')

openIndex will just be shorthand for calling getIndex directly followed by the appropriate settings updates, as I think it is a bit more intuitive if you know the settings in advance.

GregoryConrad added a commit that referenced this issue Mar 27, 2023
Fixes #156

TODO:
- [x] `instance.openIndex`
- [x] PR to meilisearch with the update pkey in settings fix
- [x] Point milli in this PR to my fork
- [x] Tests with manually set PK
- [x] Update docs
@GregoryConrad
Copy link
Owner

@vsly-ru Will make a new dev release for this now! Should be on pub.dev in a few hours once everything builds in CI.

bors bot added a commit to meilisearch/meilisearch that referenced this issue Mar 29, 2023
3608: In a settings update, check to see if the primary key actually changes before erroring out r=irevoire a=GregoryConrad

Previously, if the primary key was set and a Settings update contained a primary key, an error would be returned.
However, this error is not needed if the new PK == the current PK. This PR just checks to see if the PK actually changes before raising an error.

I came across this slight hiccup in GregoryConrad/mimir#156 (comment)

Co-authored-by: Gregory Conrad <gregorysconrad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants