Skip to content

Improve query parsing - [MOD-7794, MOD-7764]#5216

Merged
GuyAv46 merged 19 commits intomasterfrom
guyav-query_validate_fields
Nov 24, 2024
Merged

Improve query parsing - [MOD-7794, MOD-7764]#5216
GuyAv46 merged 19 commits intomasterfrom
guyav-query_validate_fields

Conversation

@GuyAv46
Copy link
Copy Markdown
Collaborator

@GuyAv46 GuyAv46 commented Nov 14, 2024 •
edited
Loading

Describe the changes in the pull request

Reducing the cases where the query quietly returns 0 results with no explanation.

Including:

  1. Validating field types while parsing the query
  2. Validating the minimum value in a numeric range is smaller than the maximal value

Some fixes are only available with DIALECT > 1.

We also drop the support of the legacy FILTER and GEOFILTER options of FT.SEARCH

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.70899% with 20 lines in your changes missing coverage. Please review.

Project coverage is 86.63%. Comparing base (357d356) to head (f69d561).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/query_parser/v2/parser.c 95.45% 11 Missing ⚠️
src/query_parser/v1/lexer.c 86.95% 3 Missing ⚠️
src/query_parser/v2/lexer.c 86.95% 3 Missing ⚠️
src/document.c 50.00% 1 Missing ⚠️
src/query_optimizer.c 66.66% 1 Missing ⚠️
src/spec.c 90.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5216      +/-   ##
==========================================
+ Coverage   86.60%   86.63%   +0.03%     
==========================================
  Files         193      193              
  Lines       34767    34693      -74     
==========================================
- Hits        30109    30057      -52     
+ Misses       4658     4636      -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@GuyAv46 GuyAv46 marked this pull request as draft November 14, 2024 18:24
@GuyAv46 GuyAv46 marked this pull request as ready for review November 15, 2024 10:33
@raz-mon
Copy link
Copy Markdown
Collaborator

raz-mon commented Nov 19, 2024 •
edited
Loading

@GuyAv46 Why are we dropping FILTER and GEOFILTER? Do we have some alternative functionality or just don't want them anymore.
Remember to update the docs accordingly.

@GuyAv46
Copy link
Copy Markdown
Collaborator Author

GuyAv46 commented Nov 19, 2024

@raz-mon Ticket MOD-7764 is about removing these capabilities. I implemented this change in this PR because we used to parse these legacy filters before we had the context of the spec. I could either change the flow order or simply drop them, as this is the plan regardless.

raz-mon
raz-mon reviewed Nov 19, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@raz-mon raz-mon left a comment

Choose a reason for hiding this comment

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

Partial review (23/46)

raz-mon
raz-mon reviewed Nov 19, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@raz-mon raz-mon left a comment

Choose a reason for hiding this comment

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

AWESOME 🔥
See some comments

@GuyAv46 GuyAv46 requested a review from raz-mon November 20, 2024 15:26
raz-mon
raz-mon previously approved these changes Nov 20, 2024
View reviewed changes
alonre24
alonre24 reviewed Nov 24, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Woohoo

@GuyAv46 GuyAv46 enabled auto-merge November 24, 2024 15:32
@GuyAv46 GuyAv46 requested a review from alonre24 November 24, 2024 15:35
@GuyAv46 GuyAv46 added this pull request to the merge queue Nov 24, 2024
Merged via the queue into master with commit d67d645 Nov 24, 2024
@GuyAv46 GuyAv46 deleted the guyav-query_validate_fields branch November 24, 2024 16:51
@redisearch-backport-pull-request
Copy link
Copy Markdown
Contributor

redisearch-backport-pull-request bot commented Nov 24, 2024

Backport failed for 8.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 8.0
git worktree add -d .worktree/backport-5216-to-8.0 origin/8.0
cd .worktree/backport-5216-to-8.0
git switch --create backport-5216-to-8.0
git cherry-pick -x d67d645a411d8409adb6e2662233a450d12b054a

GuyAv46 added a commit that referenced this pull request Nov 25, 2024
@GuyAv46
Improve query parsing - [MOD-7794, MOD-7764] (#5216)
5962931 
* general cleanup and improvements

* validate field in the parser

* drop legacy geo and numeric filters

* fix unit tests

* test fixes

* cleanup and v1 bug fix

* another test fix

* missed cleanup

* skip config test on coord

* add numeric validation min <= max

* remove redundant code

* fix more unit-tests

* fix flow tests again

* fix unit-test again

* leak fixes

* improve coverage

* addressing some review comments

* fix after merging master

(cherry picked from commit d67d645)
@GuyAv46 GuyAv46 mentioned this pull request Nov 25, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 25, 2024
@GuyAv46
[8.0] Improve query parsing - [MOD-7794, MOD-7764] (#5216) (#5251)
8ceb090 
Improve query parsing - [MOD-7794, MOD-7764] (#5216)

* general cleanup and improvements

* validate field in the parser

* drop legacy geo and numeric filters

* fix unit tests

* test fixes

* cleanup and v1 bug fix

* another test fix

* missed cleanup

* skip config test on coord

* add numeric validation min <= max

* remove redundant code

* fix more unit-tests

* fix flow tests again

* fix unit-test again

* leak fixes

* improve coverage

* addressing some review comments

* fix after merging master

(cherry picked from commit d67d645)
@meiravgri meiravgri mentioned this pull request Nov 26, 2024
Merged
@GuyAv46 GuyAv46 mentioned this pull request Jan 14, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alonre24 alonre24 alonre24 approved these changes

@raz-mon raz-mon raz-mon left review comments

@kei-nan kei-nan Awaiting requested review from kei-nan

Assignees

No one assigned

Labels

breaking change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GuyAv46 @raz-mon @alonre24