{{ message }}
Repository navigation
[8.8] [MOD-19620] Fix crash on malformed distributed LOAD AS clause - #11951
Closed
redisearch-backport-pull-request[bot] wants to merge 3 commits into
Closed
redisearch-backport-pull-request[bot] wants to merge 3 commits into
redisearch-backport-pull-request[bot] wants to merge 3 commits into
Conversation
![redis-pr-app[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
* Fix malformed distributed LOAD AS handling * Reject LOAD clauses with a dangling AS while parsing, and add tests The distributed planner walks a LOAD step's raw argument slice and assumed any `AS` was followed by an alias, citing validation that ran earlier. No earlier check existed: both LOAD parsers (FT.AGGREGATE and FT.HYBRID) only slice a counted number of tokens, and on a coordinator the planner runs before the pipeline validates the step. So `LOAD 2 @field AS` read past the end of the slice. The planner also over-read on valid input: it generates its own LOAD step from a FILTER expression's field names, so `FILTER "@x == 1 || @as == 1"` produced a slice ending in a plain field named `AS`. Add the missing check to both LOAD parsers via a shared PLNLoadStep_ValidateArgs, reusing the pipeline's existing error message. In the planner, treat `AS` as alias syntax only when a token follows it, which covers the generated steps and replaces the earlier break-on-end guard. Tests: a coordinator cpptest suite drives both triggers through AREQ_Compile and AGGPLN_Distribute, a FT.HYBRID parse case pins the hybrid rejection, and testAggregateBadLoadArgs checks the end-to-end error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Apply suggestion from @Itzikvaknin Co-authored-by: Itzikvaknin <82322982+Itzikvaknin@users.noreply.github.com> * Remove unnecessary comments in dist_plan.cpp Remove comments explaining alias handling in ArgsCursor processing. * Remove comments from PLNLoadStep_ValidateArgs Removed comments explaining the argument validation process. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Itzikvaknin <82322982+Itzikvaknin@users.noreply.github.com> (cherry picked from commit 9a84790)
redisearch-backport-pull-request
Bot
added
size:M
codex
aardvark
auto-backport
1 of 4 tasks
Collaborator
|
/backport-fix Keep everything identical to master commit 9a84790; product code is already faithful, change only the listed test files. In tests/cpptests/coord_tests/test_cpp_distribute_load.cpp: QEXEC_F_IS_COORDINATOR does not exist on 8.8 (compile error at :57). Drop that line; the tests call AGGPLN_Distribute directly. RLOOKUP_FOREACH exists on 8.8, keep it as in master. Change nothing else. |
Contributor
Author
|
🤖 Auto-backport fix declined What I observed: Failed CI run 39441669242 was reported: https://github.com/RediSearch/RediSearch/actions/runs/39441669242. No log excerpts were provided. Failed run: https://github.com/RediSearch/RediSearch/actions/runs/39441669242 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ofiryanai
changed the title
auto-merge was automatically disabled
October 21, 2026 15:57
Pull request was closed
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.
Backport of #11702 to
8.8.Drift from original
Compared line by line against
9a8479009. Product code is identical.tests/cpptests/coord_tests/test_cpp_distribute_load.cpp, forced by the 8.8 API:AREQ_New()+AREQ_Compile(r, ctx, rmArgs, n, false, qerr)andAREQ_DecrRefinstead of master's signatures andAREQ_Free; droppedAREQ_AddRequestFlags(r, QEXEC_F_IS_COORDINATOR)(the flag doesn't exist on 8.8, and the test callsAGGPLN_Distributedirectly).tests/pytests/test_blocked_client_timeout.py(branch-only test, not on master):test_fail_timeout_before_execution_plan_errorusedLOAD 2 @name ASto get an error raised late, at pipeline construction. The backported fix rejects that at parse time, so the test now usesLOAD 3 @name AS AS("Alias for LOAD cannot beAS"), which still passes parsing and fails at pipeline construction. Test intent unchanged.src/aggregate/aggregate_plan.c: kept the branch's includes; the lines added are identical to master.Original PR
9a84790094c33b9e984dd54979fe9aaed021f3b2Cherry-pick result
Conflict Log
src/aggregate/aggregate_plan.ctests/cpptests/coord_tests/test_cpp_distribute_load.cppRelease notes
🤖 Generated by the auto-backport workflow.