[8.6] MOD-14065: Fix property order discrepancy in FILTER#8660
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
force-pushed
the
backport-MOD-14065-to-8.6
branch
2 times, most recently
from
35cced7 to
3229aa1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.6 #8660 +/- ##
==========================================
+ Coverage 83.84% 83.87% +0.03%
==========================================
Files 367 367
Lines 55653 55656 +3
Branches 14321 14321
==========================================
+ Hits 46664 46684 +20
+ Misses 8828 8811 -17
Partials 161 161
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:
|
* 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.6
branch
from
3229aa1 to
eec2548
Compare
- Add QueryError_ClearError to properly clean up error messages - Revert accidental submodule updates to match 8.6 branch
- Revert EvalCtx_Eval return codes back to REDISMODULE_ERR (not EXPR_EVAL_ERR) - Remove testEvalCtxEvalExprNullExpr and testEvalCtxEvalExprUnknownProperty tests - Remove QueryError_ClearError from EvalCtx_Destroy (explicit calls remain in callers) - Update QueryError_ClearError calls in rules.c and spec.c
Collaborator
Author
|
Approved by me. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
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).
|
1 task
Itzikvaknin
reviewed
Mar 19, 2026
| } | ||
|
|
||
| ExprEval evaluator = {.err = status, .lookup = &lookup_s, .res = NULL, .srcrow = &row, .root = e}; | ||
| // In query mode missing properties are errors, as expected for `FT.ADD`. |
Itzikvaknin
approved these changes
Mar 19, 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
2 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.6 - 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.6-specific adaptations
QueryError_Default()removal inEvalCtx_Create(8.6 differs slightly from master)Note
Medium Risk
Changes core expression-evaluation semantics for missing fields by introducing mode-dependent behavior, which can affect which documents are indexed under
FILTERrules. Covered by extensive new C++ and Python regression tests, but still touches critical indexing logic.Overview
Fixes a long-standing inconsistency where indexing-time
FILTERexpressions could yield different results depending on property order when some fields are missing.This introduces
EVAL_MODE_INDEXvsEVAL_MODE_QUERYin the expression evaluator so indexing treats missing properties as non-matching (false) without raising errors, while query-time evaluation remains strict and still errors on missing properties where expected.Updates rule/spec filter evaluation to use index mode, keeps
FT.ADD ... IFevaluation in query mode, and adds regression coverage for missing-field AND/OR and comparison/type-mismatch cases (C++ unit tests + Python integration tests).Written by Cursor Bugbot for commit 0caa589. This will update automatically on new commits. Configure here.