-
Notifications
You must be signed in to change notification settings - Fork 126
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
Move model metadata retrieval out of mapper build method #111
Move model metadata retrieval out of mapper build method #111
Conversation
|
||
public KNNVectorFieldType(String name, Map<String, String> meta, int dimension) { | ||
super(name, false, false, true, TextSearchInfo.NONE, meta); | ||
this.dimension = dimension; | ||
this.modelId = null; // By default, this is null. It will only be set for a mapping with a modelId set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor should call other constructor with modelId as null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will update
29d9b27
to
f76099f
Compare
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
f76099f
to
87b7b58
Compare
Signed-off-by: John Mazanec <[email protected]>
87b7b58
to
9ed7e52
Compare
ModelMetadata modelMetadata = this.modelDao.getMetadata(modelId); | ||
|
||
if (modelMetadata == null) { | ||
throw new IllegalStateException("Model \"" + modelId + "\" does not exist. A new index will need to " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Signed-off-by: John Mazanec <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks
…project#111) Signed-off-by: Jack Mazanec <[email protected]>
Signed-off-by: Jack Mazanec <[email protected]>
…project#111) Signed-off-by: Jack Mazanec <[email protected]>
…project#111) Signed-off-by: Jack Mazanec <[email protected]> Signed-off-by: Martin Gaievski <[email protected]>
…project#111) Signed-off-by: Jack Mazanec <[email protected]>
Description
This change moves model metadata retrieval out of the model mapper build logic. The reason for this can be found in #110. From a user's perspective, the model_id they pass in during index creation will not be validated until they start ingesting documents.
As a side effect of this change, we can no longer write values such as KNNEngine and SpaceType to the Mapper's Lucene fieldType. These values are read elsewhere in the plugin, such as during indexing in the KNNDocValuesConsumer, as well as during querying. Therefore, that logic needed to be changed as well.
I tested the changes manually on my train API implementation and they work. I also added a single node test case that creates an index from a model. Currently, this fails due to an issue with writing the timestamp. I will submit another PR to fix this.
Issues Resolved
#110
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.