Skip to content

MOD-10224: Add vector filter validation#6376

Closed
nafraf wants to merge 13 commits intomasterfrom
nafraf_vector-filter-parser
Closed

MOD-10224: Add vector filter validation#6376
nafraf wants to merge 13 commits intomasterfrom
nafraf_vector-filter-parser

Conversation

@nafraf
Copy link
Collaborator

@nafraf nafraf commented Jun 25, 2025 •
edited by oshadmi
Loading

Description:
For HYBRID command add:

  • Vector filter validation - cannot use weights, cannot use vector commands
  • Search query validation - cannot use vector commands
  1. Current:
    • There is no function to validate if the vector (VSIM) filter is correct.
  2. Change:
    • Introduced explicitWeight option in QueryNodeOptions to track user-defined weights.
    • Introduced validationFlags property in QueryAST to specify if the query should accept VECTOR and weight.
    • Modified QAST_CheckIsValid to validate vector filter queries according to validationFlags
    • Created query_test_utils.h for C++ wrappers and utility functions.
    • Added test cases for vector filter validation in test_cpp_query_validation.cpp.
    • Refactored existing tests to utilize new utility functions.
  3. Outcome:
    • QAST_CheckIsValid() can be used to validate VSIM filter clause and the SEARCH query expressions

Which additional issues this PR fixes

  1. MOD-10224

Main objects this PR modified

  1. ...

Mark if applicable

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

@nafraf nafraf marked this pull request as ready for review June 25, 2025 23:28
@codecov
Copy link

codecov bot commented Jun 25, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.56%. Comparing base (7f7b054) to head (323ab09).
Report is 13 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6376      +/-   ##
==========================================
+ Coverage   89.41%   89.56%   +0.15%     
==========================================
  Files         252      254       +2     
  Lines       41678    41727      +49     
  Branches     3963     4000      +37     
==========================================
+ Hits        37267    37374     +107     
+ Misses       4354     4297      -57     
+ Partials       57       56       -1
Flag Coverage Δ
flow 81.91% <92.30%> (-0.24%) ⬇️
unit 47.76% <53.84%> (+0.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nafraf nafraf requested review from oshadmi and raz-mon June 26, 2025 17:32
JoanFM
JoanFM reviewed Jun 27, 2025
View reviewed changes
JoanFM
JoanFM reviewed Jun 27, 2025
View reviewed changes
JoanFM
JoanFM reviewed Jun 27, 2025
View reviewed changes
raz-mon
raz-mon reviewed Jun 29, 2025
View reviewed changes
Copy link
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.

Nicely and cleanly done! 💪

My main comment: With this implementation, we would need to traverse the AST again after the first traversal, done in QAST_CheckIsValid, which is probably ok for most queries, but not optimal. Consider implementing it such that we will perform all validations in one traversal, e.g., by invoking a check on a node according to some bit-field of checks relevant for this traversal, or something of that sort.

@nafraf nafraf requested a review from raz-mon July 2, 2025 01:56
@nafraf
Copy link
Collaborator Author

nafraf commented Jul 2, 2025

My main comment: With this implementation, we would need to traverse the AST again after the first traversal, done in QAST_CheckIsValid, which is probably ok for most queries, but not optimal. Consider implementing it such that we will perform all validations in one traversal, e.g., by invoking a check on a node according to some bit-field of checks relevant for this traversal, or something of that sort.

@raz-mon Thanks. QAST_CheckIsValid was modified to support the vector filter validation, the validation is done if QAST isVectorFilter is true.

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

raz-mon
raz-mon reviewed Jul 3, 2025
View reviewed changes
src/query.c Outdated

// Handle children
if (withChildren && res == REDISMODULE_OK) {
if ((validationFlags & QAST_NO_WEIGHT_ATTRIBUTE) && n->opts.explicitWeight) {
Copy link
Collaborator

@raz-mon raz-mon Jul 3, 2025

Choose a reason for hiding this comment

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

Why is this validation done only for the withChildren && res == REDISMODULE_OK cases?

Copy link
Collaborator

@oshadmi oshadmi Jul 3, 2025

Choose a reason for hiding this comment

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

We do not support query attributes for ismissing, right?

Copy link
Collaborator Author

@nafraf nafraf Jul 3, 2025

Choose a reason for hiding this comment

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

It does not make sense, but the parser accept it:

127.0.0.1:6379> FT.explaincli myidx '(ismissing(@t))=>{$slop:2; $inorder:true; $weight:0.4}' withscores dialect 2
1) ISMISSING{t} => { $weight: 0.4; $slop: 2; $inorder: true; }
2) 
127.0.0.1:6379> FT.explaincli myidx '(ismissing(@t))=>{$weight:0.4}' withscores dialect 2
1) ISMISSING{t} => { $weight: 0.4; }
``

@nafraf nafraf requested a review from raz-mon July 3, 2025 19:52
raz-mon
raz-mon reviewed Jul 6, 2025
View reviewed changes
Copy link
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.

LGTM 💪
Some small comments

@nafraf nafraf requested a review from raz-mon July 7, 2025 22:25
raz-mon
raz-mon previously approved these changes Jul 8, 2025
View reviewed changes
Copy link
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.

Nicely done

@nafraf nafraf requested a review from raz-mon July 8, 2025 14:01
raz-mon
raz-mon previously approved these changes Jul 9, 2025
View reviewed changes
oshadmi
oshadmi reviewed Jul 9, 2025
View reviewed changes
oshadmi
oshadmi reviewed Jul 9, 2025
View reviewed changes
oshadmi
oshadmi reviewed Jul 9, 2025
View reviewed changes
oshadmi
oshadmi reviewed Jul 9, 2025
View reviewed changes
@nafraf nafraf changed the base branch from master to feature-RRF July 9, 2025 15:04
@nafraf nafraf dismissed raz-mon’s stale review July 9, 2025 15:04

The base branch was changed.

@nafraf nafraf changed the base branch from feature-RRF to master July 9, 2025 15:05
@nafraf nafraf requested a review from oshadmi July 9, 2025 16:11
oshadmi
oshadmi reviewed Jul 9, 2025
View reviewed changes
oshadmi
oshadmi previously approved these changes Jul 9, 2025
View reviewed changes
raz-mon
raz-mon previously approved these changes Jul 10, 2025
View reviewed changes
@nafraf nafraf changed the base branch from master to feature-RRF July 10, 2025 12:33
@nafraf nafraf dismissed stale reviews from raz-mon and oshadmi July 10, 2025 12:33

The base branch was changed.

@nafraf nafraf changed the base branch from feature-RRF to master July 10, 2025 12:33
@nafraf nafraf mentioned this pull request Jul 10, 2025
Merged
@nafraf nafraf requested review from Itzikvaknin and removed request for Itzikvaknin July 10, 2025 13:03
@nafraf
Copy link
Collaborator Author

nafraf commented Jul 10, 2025

This PR is closed because its changes were applied to feature-RRF branch.
PR #6487

@nafraf nafraf closed this Jul 10, 2025
@nafraf nafraf deleted the nafraf_vector-filter-parser branch August 25, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoanFM JoanFM JoanFM left review comments

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

@oshadmi oshadmi oshadmi left review comments

Assignees

No one assigned

Labels

size:M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nafraf @CLAassistant @JoanFM @raz-mon @oshadmi