[8.4] MOD-14065: Fix property order discrepancy in FILTER#8661
Merged
Conversation
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
raz-mon
force-pushed
the
backport-MOD-14065-to-8.4
branch
from
b50fa51 to
2acdf81
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
* 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.4
branch
from
2acdf81 to
db79b05
Compare
- 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.4 branch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.4 #8661 +/- ##
==========================================
- Coverage 85.53% 85.53% -0.01%
==========================================
Files 337 337
Lines 53354 53358 +4
Branches 11023 11023
==========================================
+ Hits 45639 45640 +1
- Misses 7572 7575 +3
Partials 143 143
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).
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.
|
Collaborator
Author
|
Approved by me. |
1 task
Itzikvaknin
approved these changes
Mar 19, 2026
oshadmi
approved these changes
Mar 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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.4 - 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.4-specific adaptations
QUERY_ENOPROPKEY/QUERY_ENOPROPVALinstead ofQUERY_ERROR_CODE_*(8.4 naming){0}initialization for QueryError (noQueryError_Default()in 8.4)eval->err->code == QUERY_OKinstead ofQueryError_IsOk()(not available in 8.4)e->lookupObj->namedirectly instead ofRLookupKey_GetName()(not available in 8.4)Note
Medium Risk
Touches core expression evaluation and changes how missing fields/type mismatches are handled during indexing, which can affect what documents get indexed. Scope is contained to
FILTER/expression evaluation and is covered by new C++/Python regression tests.Overview
Release note: Fixes a bug where
FT.CREATE ... FILTERcould incorrectly include/exclude documents during indexing depending on the order of properties in the filter expression when some fields were missing or had type mismatches.This introduces an explicit
EvalMode(EVAL_MODE_INDEXvsEVAL_MODE_QUERY) and updates predicate/property evaluation so index-time filters treat missing properties as a non-match (false) without raising errors, while query-time/FT.ADD IFevaluation remains strict. Adds regression coverage for AND/OR ordering, missing-field comparisons, and numeric type-mismatch behavior.Written by Cursor Bugbot for commit 8b426b1. This will update automatically on new commits. Configure here.