feat: Make elastic TermsQuery use TermSetQuery internally#6151
feat: Make elastic TermsQuery use TermSetQuery internally#6151Darkheir wants to merge 2 commits intoquickwit-oss:mainfrom
Conversation
Signed-off-by: Darkheir <raphael.cohen@sekoia.io>
|
Small clarification: we don't expect performances to be always improved, but we believe mapping the Ideally we could imagine an optimisation during the warmup to have something in between warming up individual terms and the whole field. |
|
Test 0028-fast_only_field_query.yaml fails because TermSet queries seem to not be supported on fast fields. |
Should we add some logic to use a bool query when there are only few terms and a term set once a threshold is reached ?
It seems that tantivy's |
Signed-off-by: Darkheir <raphael.cohen@sekoia.io>
that would probably be for the better, though idk what the cutoff should be at all.
TermSetQuery on fastfield has the potential to always be faster than multiple TermQuery on fastfield, it would be sad to not leverage that. if you don't mind, i think i'd rather we support termsetquery over fastfield, and then merge your PR so there is no feature regression, but we still get the performance gain |
trinity-1686a
left a comment
trinity-1686a
left a comment
There was a problem hiding this comment.
i'll keep that unmerged for a few days and see if i can find the time to improve tantivy, otherwise we'll merge that, and improve tantivy+quickwit eventually
| #[test] | ||
| fn test_term_set_query_with_fast_only_field_returns_bool_query() { | ||
| let mut schema_builder = Schema::builder(); | ||
| schema_builder.add_u64_field("fast_field", FAST); |
There was a problem hiding this comment.
if we keep things that way, i think a test with mixed "one fast field and one indexed field" would be nice to have
given this is a workaround and we may update tantivy so it's no longer needed, it's not really needed though
3 participants
Description
Rely on TermSetQuery instead of a bool should query with many criteria on the same field as it should improve performances.
How was this PR tested?
Integration tests should still pass