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

Improve query modifications, skip implicit AND #74

Merged
merged 1 commit into from
Mar 10, 2024

Conversation

ddelemeny
Copy link
Collaborator

Our use of the default_operator parameter in the query allows us to rewrite ADD_FILTER operations without the "AND" operator. This, with the use of parentheses, should prevent producing queries that Quickwit can't parse.

@ddelemeny ddelemeny linked an issue Feb 16, 2024 that may be closed by this pull request
@ddelemeny
Copy link
Collaborator Author

I put parentheses by default to cover a corner case, but with further tests, I realize that it won't work either.

Query modifications add positive or negative occurrence filters to an existing query. The parser does not generally accept mixing occurrence operators and binary boolean operators, so if the initial query has boolean operators, it will fail.

Example :
-> foo AND bar // works
-> foo AND bar -baz // adding a negative filter fails

There's a corner case in the parser implementation that allows mixing operators, that is where explicit boolean binary operations are wrapped in parentheses
-> (foo AND bar) -baz // works?!
But it's unfortunately not just about isolating incompatible operations behind parentheses, the following fails:
-> foo AND bar (-baz) //bummer
Implicit and explicit operators are not consistent either
-> foo AND bar AND (-baz) // works??!?!
However, using the explicit boolean binary operator on top of an existing query with an occurence operator will fail:
-> foo -bar //works
-> foo -bar AND -baz //fails

So... Yeah, parens in this case were not applied where it matters. The only way to produce a reliably correct query for the parser is to isolate the initial query from the appended filter, which implies one of these two :

  • rewriting the initial query and making sure parentheses don't pile up
  • accepting the query to become ridden with parens with heavy use of the filter actions.

I have strong opinions against tampering with user input as a side effect of another action, there's no good solution here but to wait a fix from upstream.

@ddelemeny ddelemeny marked this pull request as draft February 19, 2024 14:47
@ddelemeny ddelemeny changed the title Impl parser-safe query modifications, fixes #56 Impl parser-safe query modifications Feb 21, 2024
@ddelemeny ddelemeny self-assigned this Mar 5, 2024
@ddelemeny ddelemeny force-pushed the ddelemeny/fix-query-actions branch from 0eeb154 to 4e2d78e Compare March 5, 2024 22:43
@ddelemeny ddelemeny linked an issue Mar 5, 2024 that may be closed by this pull request
@ddelemeny ddelemeny force-pushed the ddelemeny/fix-query-actions branch from 4e2d78e to 3651c95 Compare March 5, 2024 22:49
@ddelemeny ddelemeny force-pushed the ddelemeny/fix-query-actions branch from 3651c95 to 788fd75 Compare March 5, 2024 22:50
@ddelemeny ddelemeny marked this pull request as ready for review March 5, 2024 22:50
@ddelemeny ddelemeny changed the title Impl parser-safe query modifications Improve query modifications, skip implicit AND Mar 5, 2024
@ddelemeny
Copy link
Collaborator Author

Revamped this PR to keep only the query mutation part and the optional operator. Ready for review

@ddelemeny ddelemeny requested a review from fmassot March 5, 2024 22:55
@fmassot fmassot merged commit ecca730 into main Mar 10, 2024
2 checks passed
@ddelemeny ddelemeny deleted the ddelemeny/fix-query-actions branch March 11, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove AND on a positive filter in explore view and use simple quotes Exclude query not working
2 participants