MOD-10047: Prepare Hybrid Parsing For Hybrid Cursors by kei-nan · Pull Request #6736 · RediSearch/RediSearch

kei-nan · 2025-08-27T13:25:58Z

Reorganize the parsing of a hybrid request so a hybrid request can live without a pipeline.

A clear and concise description of what the PR is solving, including:

Current: Various subquery pipeline related code is done in hybrid merger construction
Change: Since we might not have a merger we need the pipeline changes to happen in the parsing level, this also meant heavilty changing the cpp tests that relied on the hybrid merger pipeline construction.(we use the parse function instead)
Outcome: Cleaner code.

Main objects this PR modified

Hybrid Request

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

codecov · 2025-08-28T11:38:23Z

Codecov Report

❌ Patch coverage is 70.55215% with 48 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feature-RRF@cd47772). Learn more about missing BASE report.

⚠️ Current head 84e3107 differs from pull request most recent head b09b749

Please upload reports for the commit b09b749 to get more accurate results.

Files with missing lines	Patch %	Lines
src/hybrid/hybrid_exec.c	45.33%	41 Missing ⚠️
src/hybrid/hybrid_request.c	92.10%	3 Missing ⚠️
src/module.c	60.00%	2 Missing ⚠️
src/aggregate/aggregate_request.c	83.33%	1 Missing ⚠️
src/hybrid/parse_hybrid.c	97.29%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff               @@
##             feature-RRF    #6736   +/-   ##
==============================================
  Coverage               ?   88.69%           
==============================================
  Files                  ?      261           
  Lines                  ?    42760           
  Branches               ?     3630           
==============================================
  Hits                   ?    37925           
  Misses                 ?     4786           
  Partials               ?       49

Flag	Coverage Δ
flow	`80.91% <63.80%> (?)`
unit	`49.08% <46.62%> (?)`

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.

Copilot

Pull Request Overview

This PR refactors hybrid request parsing to move pipeline-related code from the merger construction phase to the parsing level, enabling hybrid requests to exist without requiring a merger. The primary motivation is to prepare for future cursor support in hybrid search by decoupling request construction from pipeline building.

Key changes include:

Reorganized parsing function to return structured command context rather than complete HybridRequest
Moved LOAD step processing and implicit sorting logic from merger to parser
Updated test suite to use new parsing interface instead of merger-based construction

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`tests/cpptests/test_cpp_parsed_hybrid_pipeline.cpp`	New test file using parse-then-build pattern instead of merger construction
`tests/cpptests/test_cpp_parse_hybrid.cpp`	Updated to use new ParseHybridCommandCtx structure and parsing interface
`tests/cpptests/test_cpp_hybrid_defaults.cpp`	Modified to work with restructured parsing context and parameter handling
`tests/cpptests/test_cpp_hybrid.cpp`	Simplified to basic HybridRequest creation test, removing pipeline tests
`src/pipeline/pipeline_construction.h`	Removed processLoadStep function declaration
`src/pipeline/pipeline_construction.c`	Updated comment about scorer creation conditions
`src/module.c`	Split hybrid command handler into internal/external variants
`src/hybrid/parse_hybrid.h`	Added ParseHybridCommandCtx structure and updated function signature
`src/hybrid/parse_hybrid.c`	Major refactor moving pipeline logic to parsing phase
`src/hybrid/hybrid_request.h`	Updated HybridRequest structure and added new pipeline building functions
`src/hybrid/hybrid_request.c`	Split pipeline building into depletion and merge phases
`src/hybrid/hybrid_exec.h`	Updated function signatures and added cursor support
`src/hybrid/hybrid_exec.c`	Refactored execution flow to work with new parsing structure
`src/aggregate/aggregate_request.c`	Enhanced SORTBY 0 handling for hybrid tail queries
`src/aggregate/aggregate.h`	Added QEXEC_F_NO_SORT flag for explicit no-sorting behavior

Comments suppressed due to low confidence (1)

tests/cpptests/test_cpp_parse_hybrid.cpp:1

Same issue as previous test - 'COMBINE' should not appear before 'SORTBY'. The command structure is incorrect. It should be: 'SEARCH', 'hello', 'VSIM', '@vector', TEST_BLOB_DATA, 'SORTBY', '1', '@score'

/*

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tests/cpptests/test_cpp_parse_hybrid.cpp

src/hybrid/hybrid_request.c

Copilot · 2025-08-28T13:19:06Z

src/hybrid/parse_hybrid.c

+  if (hybridParams->scoringCtx) {
    HybridScoringContext_Free(hybridParams->scoringCtx);
-    rm_free(hybridParams);
-  }
-
-  if (tailPipeline) {
-    Pipeline_Clean(tailPipeline);
-    rm_free(tailPipeline);
+    hybridParams->scoringCtx = NULL;


The error handling only frees the scoringCtx but doesn't clean up other resources that may have been allocated during parsing, such as vector query parameters or other contexts that could have been created before the error occurred. This could lead to memory leaks.

src/hybrid/hybrid_request.h

Itzikvaknin · 2025-08-31T11:18:00Z

src/hybrid/hybrid_request.c

            StrongRef_Release(sync_ref);
            array_free(depleters);
-            return rc;
+            return NULL;


Are you sure this won't cause some side-effects? Callee level

pretty sure,
currently it is only called in HybridRequest_BuildPipeline and it handles it correctly.

src/hybrid/hybrid_exec.c

Itzikvaknin · 2025-08-31T11:40:45Z

src/hybrid/hybrid_exec.c

+                                                         RedisModuleCtx *ctx, RedisSearchCtx *sctx, QueryError *status, 
+                                                         bool internal) {
+  // Build the pipeline and execute
+  hreq->reqflags = hybridParams->aggregationParams.common.reqflags;


Consider moving this to HybridRequest_BuildPipeline

Itzikvaknin · 2025-08-31T11:47:44Z

src/module.c

+int RSShardedHybridCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
+  return hybridCommandHandler(ctx, argv, argc, true);
+}
+
+int RSClientHybridCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
+  return hybridCommandHandler(ctx, argv, argc, false);


Consider different names - like internal-external

Itzikvaknin

LGTM, see few comments

nafraf · 2025-09-01T15:48:24Z

src/aggregate/aggregate_request.c

+        AC_Advance(ac);  // Advance without adding SortBy step to the plan
+        *papCtx->reqflags |= QEXEC_F_NO_SORT;
+      } else {
+        QueryError_SetError(status, QUERY_EPARSEARGS, "SORTBY 0 is not supported in this type of query");


Do we have test for this?

Yes, lets have a also a flow test for this

src/hybrid/hybrid_exec.c

src/hybrid/hybrid_request.c

tests/cpptests/test_cpp_hybrid.cpp

tests/cpptests/test_cpp_hybrid_defaults.cpp

hybrid request and hybrid parsing changes

9241473

kei-nan requested review from Itzikvaknin and nafraf August 27, 2025 13:25

kei-nan self-assigned this Aug 27, 2025

github-actions bot added the size:XL label Aug 27, 2025

kei-nan added 4 commits August 28, 2025 11:00

important fixes to tests and code

613cb2d

revert unit-tests file changes

f60b72b

small fixes, zero out hybrid params in test setup

c2d628a

fix memory leaks in unit tests

1e0e2cf

fix memory leaks in flow tests

d8c2128

kei-nan requested a review from Copilot August 28, 2025 13:17

Copilot AI reviewed Aug 28, 2025

View reviewed changes

kei-nan changed the title ~~Prepare Hybrid Parsing For Hybrid Cursors~~ MOD-10047: Prepare Hybrid Parsing For Hybrid Cursors Aug 28, 2025