Skip to content
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

don't error on phrase query schema error for old splits #5289

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

trinity-1686a
Copy link
Contributor

Description

part of #5084
don't execute phrase queries on old splits if they miss positions

How was this PR tested?

re-enabled tests

Copy link

github-actions bot commented Aug 1, 2024

On SSD:

Average search latency is 1.01x that of the reference (lower is better).
Ref run id: 2746, ref commit: 9d7c7ae
Link

On GCS:

Average search latency is 0.982x that of the reference (lower is better).
Ref run id: 2747, ref commit: 9d7c7ae
Link

Copy link
Contributor

@rdettai rdettai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the bug (so approving the PR), but we should add unit tests to cover these scenarii and more. In the various files of the query_ast module there are some unit tests that could be extended. For instance in full_text_query.rs you have tests for some successful TantivyQueryAst builds. We should extend these against "all" possible schema field types (expected to be valid or not).

@trinity-1686a trinity-1686a enabled auto-merge (squash) August 6, 2024 16:14
@trinity-1686a trinity-1686a merged commit e911e99 into main Aug 6, 2024
5 checks passed
@trinity-1686a trinity-1686a deleted the trinity/update-phrase-query branch August 6, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants