Bug fix: added deterministic sorting for sortby

[JasonLi314]

Fixes #1353 item 8.

FT.SEARCH SORTBY ties (documents with equal sort values) have no tie-break, so their relative order depends on incidental retrieval order rather than the data. Two consequences: repeating the same query can return ties in a different order, and because the full-result path uses std::stable_sort while the LIMIT path uses std::partial_sort, a LIMITed reply is not a prefix of the unlimited one — paginating over ties yields duplicate or missing rows.

This adds a direction-following tie-break on the document key, making the comparator a total order so the result is a pure function of the data and query, and a LIMITed reply is a true prefix of the unlimited one.

Not a Redis-compatibility fix: RediSearch breaks ties differently. This is a determinism improvement, so it is ungated.

[github-actions]

Reviewers for this PR

• First Pass Reviewer: @Aksha1812 — Please do your best to do a detailed review on the PR and get a response on your feedback. Once the first pass is done, notify the maintainer assigned to this PR to follow up on the final review and getting the PR merged. You can reach out to the people owning the relevant code paths for more help on the review.
• Maintainer Reviewer: @KarthikSubbarao — Once the first review is done, please follow up with a final review and help to merge the change in.

_{Assigned automatically to the least-assigned members of the reviewer pools in .github/reviewer-pools.json. Use /reviewer or /remove-reviewer to adjust.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 0f8d7286-adf7-43d6-9740-7f80dcbacf61

📥 Commits

Reviewing files that changed from the base of the PR and between d88224e and 8782aa4.

📒 Files selected for processing (2)

• integration/test_sortby_vector.py
• src/query/search.cc

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The change makes FT.SEARCH SORTBY ordering deterministic for equal values and documents missing the sort field. It updates stored-field retrieval for vector queries and adds integration coverage for full and limited results.

Changes

SORTBY ordering

Layer / File(s)	Summary
Sorting comparator behavior src/commands/ft_search.cc	The comparator uses direction-aware document-key tie-breaking. Missing sort fields sort after present fields. Equal vector and non-vector values use the same tie-breaker.
Stored sort-field retrieval src/query/search.cc	Indexed content is not used when SORTBY targets a stored field. Score-based sorting remains eligible for indexed content materialization.
Integration coverage integration/test_non_vector.py, integration/test_sortby_vector.py	Tests cover equal values, missing fields, stored-field sorting without RETURN access, distance aliases, both sort directions, and limited-result prefixes.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant FTSEARCH
  participant MaybeAddIndexedContent
  participant ApplySorting
  Client->>FTSEARCH: Execute FT.SEARCH with SORTBY
  FTSEARCH->>MaybeAddIndexedContent: Resolve content for sort target
  MaybeAddIndexedContent-->>FTSEARCH: Return stored-field or score content
  FTSEARCH->>ApplySorting: Sort matching documents
  ApplySorting-->>FTSEARCH: Return direction-aware ordered results
  FTSEARCH-->>Client: Return full or limited reply

Loading

Priority: ➖ Normal

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 8782a

No merge-blocking regression is identified in the changed SORTBY behavior.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The MaybeAddIndexedContent change and integration/test_sortby_vector.py add behavior for KNN queries that sort by stored fields omitted from RETURN. This behavior is distinct from issue #1353 it…	Remove the unrelated KNN stored-field change and its test, or link this work to a coding requirement that explicitly includes KNN SORTBY on stored fields.
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: deterministic ordering for SORTBY results. It is concise and related to the changeset.
Description check	✅ Passed	The description directly explains the SORTBY tie-breaking problem, the document-key solution, LIMIT behavior, and scope of the fix.
Linked Issues check	✅ Passed	For issue #1353 item 8, ApplySorting now uses external_id as a direction-aware tie-breaker for vector-score ties, equal sort values, and documents with missing sort values. The comparator therefor…

Full details: Out of Scope Changes check

Explanation

The MaybeAddIndexedContent change and integration/test_sortby_vector.py add behavior for KNN queries that sort by stored fields omitted from RETURN. This behavior is distinct from issue #1353 item 8, which requires deterministic ordering for SORTBY ties and consistent LIMIT prefixes. The change is not needed for the tie-breaker implementation.

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

Safe to merge.

Findings

1.  Preserve vector sort fields ▶

Summary

This update makes FT.SEARCH SORTBY ordering deterministic with document-key tie-breaking, keeps documents missing the sort field last, and ensures vector searches sorting by stored fields resolve the sort field even when it is omitted from RETURN.

_{Reviews (2) · Last reviewed commit: "fix SORTBY on stored fields for KNN quer..."}

[Aksha1812]

lets trim AI generated comments.

[Aksha1812]

Fix looks correct . but this might cause performance to degrade for sort by queries and would force a main-thread GetContent per neighbor where it used to be fully index-served. Wondering if adding the sortby fields to the attributes list is the more efficient choice here.