[8.2] MOD-14342: Fix property order discrepancy in FILTER#8662
Merged
Conversation
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
raz-mon
changed the title
FILTER to 8.2FILTER
raz-mon
changed the title
FILTERFILTER
raz-mon
force-pushed
the
backport-MOD-14065-to-8.2
branch
from
21dff77 to
0237a3a
Compare
* WIP make the order of the properties in the FILTER expression not matter for evaluation * New approach * Handle missing fields comparison * Enhance readability * Enhance tests * Fix initialization and cleanup * fix tests * Remove unused APIs * Fix leak in test * Address review * Explicitly set mode of query evaluators * Fix leak in other test * Address review * small proposal to PR (#8587) * Revert accidental code removal --------- Co-authored-by: Joan Fontanals <[email protected]>
raz-mon
force-pushed
the
backport-MOD-14065-to-8.2
branch
from
0237a3a to
62fd32f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
- Revert EvalCtx_Eval return codes back to REDISMODULE_ERR (not EXPR_EVAL_ERR) - Remove testEvalCtxEvalExprNullExpr and testEvalCtxEvalExprUnknownProperty tests - Revert accidental submodule updates to match 8.2 branch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.2 #8662 +/- ##
==========================================
- Coverage 88.96% 88.95% -0.02%
==========================================
Files 260 260
Lines 41990 41994 +4
Branches 3851 3851
==========================================
- Hits 37357 37356 -1
- Misses 4584 4589 +5
Partials 49 49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add QueryError_ClearError(&r->status) before EvalCtx_Destroy(r) to prevent leaking error messages that may be set during filter evaluation (e.g., type mismatches in RSValue_Cmp).
|
Collaborator
Author
|
Approved by me. |
1 task
Itzikvaknin
approved these changes
Mar 19, 2026
oshadmi
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes in the pull request
Backport of #8548 to 8.2 - fixes property order discrepancy in
FILTER.FILTERexpression references properties in a certain order, documents may be incorrectly excluded when properties appear in a different order than in the filter expression.EVAL_MODE_INDEXandEVAL_MODE_QUERYto distinguish between indexing and query-time evaluation. InEVAL_MODE_INDEX, missing properties no longer cause errors but return undefined values.Which additional issues this PR fixes
waitForIndexcalls to prevent test flakinessMain objects this PR modified
src/aggregate/expr/expression.c- Core evaluation logic with eval mode distinctiontests/pytests/test_filter.py- Added regression teststests/cpptests/test_cpp_expr.cpp- Added C++ unit testsMark if applicable
Release Notes
8.2-specific adaptations
QUERY_ENOPROPKEY/QUERY_ENOPROPVALinstead ofQUERY_ERROR_CODE_*(8.2 naming){0}initialization for QueryError (noQueryError_Default()in 8.2)eval->err->code == QUERY_OKinstead ofQueryError_IsOk()(not available in 8.2)e->lookupObj->namedirectly instead ofRLookupKey_GetName()(not available in 8.2)RLookup_LoadRuleFieldssignature (no status parameter in 8.2)src/pipeline/pipeline_construction.cchanges (file doesn't exist in 8.2)Note
Medium Risk
Changes core expression evaluation for index-time
FILTERpredicates, altering how missing fields and type mismatches are handled; could affect which documents are indexed for some filters. Behavior is covered by new C++ and Python regression tests but touches central evaluation logic.Overview
Fixes an indexing bug where
FILTERexpressions could include/exclude documents depending on the order of referenced fields when some fields were missing.Adds
EvalModeto expression evaluation and makes index-time evaluation (EVAL_MODE_INDEX) treat missing properties as a non-match (false) instead of an error, while keeping query/FT.ADD IFevaluation strict (EVAL_MODE_QUERY). Predicate evaluation was refactored to centralize missing-field handling and preserve correct short-circuiting for&&/||.Adds regression tests (C++ and Python) for AND/OR with missing fields, missing-vs-NULL comparisons, and numeric type-mismatch behavior in filters.
Written by Cursor Bugbot for commit 827c25d. This will update automatically on new commits. Configure here.