-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ESQL - KNN function uses prefilters when pushed down to Lucene #131004
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
Changes from all commits
b7a99fc
61a1e4b
fc0e54b
6a52b63
a73973f
ef12ee5
616b725
3ae0e9b
50d6e19
32715ce
7ebd1d9
9455f77
62b0d1c
82fd2cc
bfdd227
b347bba
1083764
6e4c1a9
91d4a0a
a99e75f
4c6bb4d
47a6d99
ac7c3bc
32ada3d
939705c
dd29fec
b03f2a3
20fe8fe
89c86e4
8a550a1
ea2fddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1213,7 +1213,7 @@ public enum Cap { | |
| /** | ||
| * Support knn function | ||
| */ | ||
| KNN_FUNCTION_V2(Build.current().isSnapshot()), | ||
| KNN_FUNCTION_V3(Build.current().isSnapshot()), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Super nitpick - I wonder if we could create an alias that this could reference (so when we go to V4 we only need to change one variable when testing for test compatibility)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you provide an example of what it would look like?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like KNN_FUNCTION_CURRENT = EsqlCapabilities.KNN_FUNCTION_V3?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sue if that works with CSV tests 🤔 . I'd say let's keep this as it's an established pattern in ES|QL. Maybe I'll give it a try for the next one and see what it looks like from a changes perspective 👍 |
||
|
|
||
| /** | ||
| * Support for the LIKE operator with a list of wildcards. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,20 +166,19 @@ public boolean equals(Object obj) { | |
|
|
||
| @Override | ||
| public Translatable translatable(LucenePushdownPredicates pushdownPredicates) { | ||
| // In isolation, full text functions are pushable to source. We check if there are no disjunctions in Or conditions | ||
| return Translatable.YES; | ||
| } | ||
|
|
||
| @Override | ||
| public Query asQuery(LucenePushdownPredicates pushdownPredicates, TranslatorHandler handler) { | ||
| return queryBuilder != null ? new TranslationAwareExpressionQuery(source(), queryBuilder) : translate(handler); | ||
| return queryBuilder != null ? new TranslationAwareExpressionQuery(source(), queryBuilder) : translate(pushdownPredicates, handler); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added pushdownPredicates to translate method, as we need to evaluate the prefilters with them in order to decide |
||
| } | ||
|
|
||
| public QueryBuilder queryBuilder() { | ||
| return queryBuilder; | ||
| } | ||
|
|
||
| protected abstract Query translate(TranslatorHandler handler); | ||
| protected abstract Query translate(LucenePushdownPredicates pushdownPredicates, TranslatorHandler handler); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be easier to have two signatures for this, for a smaller file change count?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's more confusing. |
||
|
|
||
| public abstract Expression replaceQueryBuilder(QueryBuilder queryBuilder); | ||
|
|
||
|
|
||
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.
Using prefilter means that we no longer need to retrieve all results, but can use k as inteneded as conjunction is pushed down as a prefilter now