Pre-filter evaluation #505
Conversation
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
KarthikSubbarao
force-pushed
the
fulltext
branch
from
bdd9be5 to
2604515
Compare
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
I, Manish Addanki <[email protected]>, hereby add my Signed-off-by to this commit: 2299b7b Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
…ter-eval-text Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
…ter-eval-text Signed-off-by: Manish Addanki <[email protected]>
Signed-off-by: Manish Addanki <[email protected]>
| class TestFullText(ValkeySearchTestCaseDebugMode): | ||
|
|
||
| def test_text_search(self): | ||
| @pytest.mark.parametrize("prefilter_eval_enabled", [pytest.param(False, id="Prefilter_eval=False"), pytest.param(True, id="Prefilter_eval=True")]) |
There was a problem hiding this comment.
You could just store "yes" / "no" as param values and then we can execute config set commands without any IF statement. It will keep the tests less verbose
There was a problem hiding this comment.
Ack will do this in followup as discussed
|
|
||
| // Access target key for proximity validation (only for Text) | ||
| virtual const InternedStringPtr& GetTargetKey() const = 0; | ||
| virtual bool IsPrefilterEvaluator() const { return false; } |
There was a problem hiding this comment.
A more generic API would be GetEvaluatorType and this can return either Prefilter or Predicate. This can be an enum return type
There was a problem hiding this comment.
Ack will do this in followup as discussed
| .filter = "\\\\\\\\\\(Hello, \\$how \\\\\\*are \\\\\\-you " | ||
| "\\\\\\\\\\%doing?", | ||
| .create_success = true, | ||
| .evaluate_success = true, |
There was a problem hiding this comment.
I would like to have this evaluation check added back in all the test cases that have them removed currently.
We could actually have more test cases that validate the positional check logic in the prefilter evaluator.
I understand that our text ingestion is missing escaped char handling. This is planned to be addressed soon, so we should get back on coverage on it here.
Can we do this in a follow up?
- Add more test cases that validate positional checks / prefilter logic checks
- Add escaped char handling after our ingestion is updated. (If we want to search for a term starting with a backslash).
There was a problem hiding this comment.
Ack will do this in followup
KarthikSubbarao
left a comment
KarthikSubbarao
left a comment
There was a problem hiding this comment.
Discussed with @daddaman-amz that these comments which are non blocking and more about maintainability & additional coverage will be addressed in a follow up
2 participants
After rebases with major PR's
Changes for pre-filter evaluator for Text.
There is a config to turn off the evaluation for Non vector (by default it is on).
Reason for config:
We wanted to see the performance difference for disabling pre-filter evaluation (i.e it passes through keys with only deduplication but no evaluation) vs enabling it