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

Implement the query parsing in the default doc mapper & disable range queries #140

Closed
fulmicoton opened this issue Jun 21, 2021 · 5 comments
Assignees
Labels
bug Something isn't working high-priority

Comments

@fulmicoton
Copy link
Contributor

Range queries are not possible today in quickwit. We need to disable them.

@fulmicoton fulmicoton added the bug Something isn't working label Jun 21, 2021
@fmassot
Copy link
Contributor

fmassot commented Jun 24, 2021

Renamed the issue as we first need to implement the query parsing in the default doc mapper.

@fmassot fmassot changed the title Disable range queries Implement the query parsing in the default doc mapper & disable range queries Jun 24, 2021
@evanxg852000 evanxg852000 self-assigned this Jun 24, 2021
@fulmicoton
Copy link
Contributor Author

@evanxg852000 I see that you assigned the ticket to yourself. Here is a bit of guidance.

One "clean" way to do this is as follows.

Depend on the tantivy-query-grammar crate.
https://docs.rs/tantivy-query-grammar/0.15.0/tantivy_query_grammar/enum.UserInputAst.html

Parse the user query into an UserInputQuery object.
Go recursively through the Ast (no worries there cannot be any cycle) and check that there are no range within the query.

If there are no range, just go on with parsing the query the way we did before. (it feels stupid because we end up parsing the query twice but that's fine).

If there is a range, return an Error explaining ranges are not supported for the moment.

@fulmicoton
Copy link
Contributor Author

fulmicoton commented Jun 25, 2021

@evanxg852000 I think you should also factorize our trivial query building logic.

A good way to do it might be to introduce a pub(crate) in doc_mapper.rs

pub(crate) fn build_query(
    schema: &Schema,
    request: &SearchRequest,
    default_fields: Vec<String>,
) -> Result<Box<dyn Query>, QueryParserError> { ... }

That implements does the field name resolution, the no range query check, and the query parsing boilerplate.

Then have the different mapper call this function.

(an alternative solution that might seem logic would be to have a blanket implementation in DocMapper and add a default_fields method in the trait.. I am not fond of this solution because it requires adding the default_fields method in the trait, but feel free to challenge it.)

@evanxg852000
Copy link
Collaborator

Thanks @fulmicoton for the explanation, I was already wondering how would I plug my code to check for Range since the tantivy query parser does all the work without exposing intermediate step/object. But yes this is the cleanest solution for our case while avoiding duplication of the parser.
On the second note, I will go with your solution. To me, this has an advantage in handling all this in one site.

@fulmicoton
Copy link
Contributor Author

The benefit is minor... By adding the build_query function, you can implement the logic in every call traits. On the other hand, it forces us to add a default_fields method in the trait, even though this is not necessarily something that always makes sense.

@fmassot fmassot added this to the Quickwit 0.1 milestone Jul 4, 2021
@fmassot fmassot closed this as completed Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high-priority
Projects
None yet
Development

No branches or pull requests

3 participants