-
Notifications
You must be signed in to change notification settings - Fork 169
Issue #459: Add Mustache Scripting Support for knn_vector Fields #2807
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
base: main
Are you sure you want to change the base?
Issue #459: Add Mustache Scripting Support for knn_vector Fields #2807
Conversation
|
Thanks @night-owl-1709 - will take a look! |
|
@night-owl-1709 One question: for painless, we extend the lang-painless plugin and use SPI to extend it. Is that not possible with expression and mustache? |
eb40c52 to
23d7779
Compare
|
Instead of treating knn_l2_distance like a value (and the other distance methods as values), is it possible to treat them like functions in painless? For example: Im not sure if this is possible, but I feel the distance functions should be somewhat standardized, similar to operations like "+". |
fa08347 to
ae3b35c
Compare
c2ecf16 to
046b25a
Compare
046b25a to
4601e68
Compare
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.
Just want to
src/main/java/org/opensearch/knn/plugin/script/KNNScriptService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNScriptService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNScoringScriptEngine.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
a67ba8b to
1cfa160
Compare
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheContextInjector.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/knn/plugin/script/KNNMustacheIT.java
Outdated
Show resolved
Hide resolved
|
@Vikasht34, @shatejas to review as well |
58d8150 to
3080d8e
Compare
…vector Fields Signed-off-by: Abhinav Tripathi <[email protected]>
3080d8e to
5707ab5
Compare
| public ScriptEngine getScriptEngine(Settings settings, Collection<ScriptContext<?>> contexts) { | ||
| // Check if this is specifically a template script request | ||
| if (contexts.size() == 1 && contexts.contains(TemplateScript.CONTEXT)) { | ||
| return new KNNMustacheEngine(); |
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.
I was wondering if plugin should reuse the existing implementation of mustache engine. The plugin will probably need a dependency on lang-mustache module. @night-owl-1709 have you explored the approach? were there any problems with it?
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.
I checked but we do not have lang-mustache support yet like we have for painless in opensearch.
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.
Not sure if I understand this but did you try adding lang-mustache here https://github.com/opensearch-project/k-NN/blob/main/build.gradle#L318
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.
So the implementation here is similar to https://github.com/opensearch-project/OpenSearch/blob/main/modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java#L67 with minor changes.
- This uses default factory compared to custom factory. The compile implementation of both is the same as custom factory inherits from default factory.
- There are some node metrics that are being added, if core implementation is used, I don't think its needed.
so instead of returning knn mustache engine we can return core implementation of it.
I would prefer using core implementation to be consistent and less maintenance on the plugin. Let me know if I am missing something here
| public ScriptEngine getScriptEngine(Settings settings, Collection<ScriptContext<?>> contexts) { | ||
| // Check if this is specifically a template script request | ||
| if (contexts.size() == 1 && contexts.contains(TemplateScript.CONTEXT)) { | ||
| return new KNNMustacheEngine(); |
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.
So the implementation here is similar to https://github.com/opensearch-project/OpenSearch/blob/main/modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java#L67 with minor changes.
- This uses default factory compared to custom factory. The compile implementation of both is the same as custom factory inherits from default factory.
- There are some node metrics that are being added, if core implementation is used, I don't think its needed.
so instead of returning knn mustache engine we can return core implementation of it.
I would prefer using core implementation to be consistent and less maintenance on the plugin. Let me know if I am missing something here
| public String execute() { | ||
| StringWriter writer = new StringWriter(); | ||
| try { | ||
| template.execute(writer, params); |
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.
Summary
This PR introduces Mustache templating support for k-NN queries by implementing a dedicated
KNNMustacheEnginethat enables dynamic query generation through the Search Template API. This enhancement allows users to create reusable k-NN query templates with parameterized field names, vectors, and search parameters.Issues
Implements #459
Changes Made
1. New Script Engine Implementation
src/main/java/org/opensearch/knn/plugin/script/KNNMustacheEngine.javaScriptEngineinterfaceTemplateScript.CONTEXTfor query templatingKNNCounter2. Plugin Integration
KNNPlugin.getScriptEngine()methodKNNMustacheEnginewhenTemplateScript.CONTEXTis requestedKNNScoringScriptEngine3. Build Configuration
build.gradlecom.github.spullara.mustache.java:compiler:0.9.10How Mustache Integration Works
The integration follows OpenSearch's plugin architecture patterns and works through these components:
KNNPlugin.getScriptEngine()and responds toTemplateScript.CONTEXTrequestsDefaultMustacheFactoryto compile template strings into executableMustacheobjects{{field}},{{vector}},{{k}}) with actual values from the params mapUsage Examples
Basic k-NN Query Template
k-NN Query with Conditional Parameters
k-NN Query with Filter Template
Benefits
Testing
KNNMustacheEnginecompilation and executionBackward Compatibility
This change is fully backward compatible:
KNNScoringScriptEngineremains the default for script scoring contexts