Implement SORTBY on FT.SEARCH #1

AlexFilipImproving · 2025-11-21T15:36:26Z

No description provided.

COMMANDS.md

src/query/search.cc

testing/ft_search_parser_test.cc

Jonathan-Improving

LGTM!

allenss-amazon

It's important to handle the case where both SORTBY and LIMIT are specified. In this scenario, rather than a full sort of the neighbors, a heap should be used to retain only the required number of entries.

src/commands/ft_search_parser.cc

src/query/search.cc

allenss-amazon

The structure of the main data path of this code is wrong.

It's on the wrong thread and thus looking at potentially stale data. The data for the sort needs to come out of fetched data, not the indexes.
In cluster mode it's only operating on the local data, not the post-merged global result.

It needs to be placed in the call chain of "SearchCommand::SendReply" after the current contents of the candidate keys has been fetched and is available. This will probably require a logic change in the "NOCONTENT" option (since it's suppressing fetching of per-key fields) as well as ensuring that the content fetch code includes the sortby field.

Perhaps some 1:1 time with me will help here.

src/commands/ft_search_parser.cc

allenss-amazon · 2025-11-27T00:36:21Z

src/query/search.cc

  return results;
 }

+absl::StatusOr<std::deque<indexes::Neighbor>> ApplySorting(


I don't think this can fail. The only possible failure is if the sortby field isn't found, and that's probably a crash since you've already validated it in the command itself long before you get here....

AlexFilipImproving · 2025-11-27T17:09:24Z

The structure of the main data path of this code is wrong.

It's on the wrong thread and thus looking at potentially stale data. The data for the sort needs to come out of fetched data, not the indexes.

In cluster mode it's only operating on the local data, not the post-merged global result.

It needs to be placed in the call chain of "SearchCommand::SendReply" after the current contents of the candidate keys has been fetched and is available. This will probably require a logic change in the "NOCONTENT" option (since it's suppressing fetching of per-key fields) as well as ensuring that the content fetch code includes the sortby field.

Perhaps some 1:1 time with me will help here.

@allenss-amazon I generally understand the idea but yes, I think a 1:1 would be a good idea. I'm looking at the code now and I think I need a bit of guidance on the implementation. How would you like the meeting set up?

allenss-amazon

Looks pretty good. Two things. 1/ I'd like to see a heap-oriented sort for cases where LIMIT is in effect and 2/ need some data oriented actual search cases.

allenss-amazon · 2025-12-02T20:46:24Z

src/commands/ft_search.cc

+          double num_a, num_b;
+          if (!absl::SimpleAtod(val_a, &num_a)) return false;
+          if (!absl::SimpleAtod(val_b, &num_b)) return true;
+          result = num_a < num_b;
+        } else {
+          result = val_a < val_b;
+        }


We want to have these kinds of data comparison centralized so that all of the various ways to compare data get the same answers. I'm not familiar with out SimpleAtoD deals with infinity and NaN, etc. Please take a look at the files in the src/expr/* directory.

allenss-amazon · 2025-12-02T21:08:41Z

src/query/search.h

  int k{0};
  std::optional<unsigned> ef;
  LimitParameter limit;
+  SortByParameter sortby;


I believe this belongs in "struct SearchCommand", not here. This parameter is private to the search command, not something that's shared across general queries. (Sorry the command naming here is legacy and therefore confusing).

I tried doing this and I'm not sure how this would fit into SearchCommand since it needs to be located by the parser and I would have to change the implementation of ConstructSortByParser to take a SearchCommand &, which would propagate to all other parsers in ft.search.
It would also be inconsistent with other parsers, like ft.aggregate.

Legacy issue. Originally, there wasn't a difference between SearchParameters and SearchCommand. When this object hierarchy was introduced it looks like the parsing logic for ft.search didn't get the update. Please update it, it shouldn't require much more than changing the parameters definitions on a few functions in the dedicate ft.search parsing logic.

allenss-amazon · 2025-12-02T21:12:00Z

testing/ft_search_parser_test.cc

+            .sortby_parameters_str = "SORTBY attribute_identifier_2",
+            .sortby_field = "attribute_identifier_2",
+            .sortby_order = query::SortOrder::kAscending,
+            .sortby_enabled = true,


A parser test for a field NOT in the schema might be useful

I added a test where the attribute is not found and found that I actually wasn't testing for this case so that's in here too now.

allenss-amazon · 2025-12-02T21:27:03Z

testing/ft_search_test.cc

+                                     "embedding",
+                                     "$embedding",
+                                     "SORTBY",
+                                     "vector",


This needs some data oriented tests. In theory, the compatibility framework that's under integration/compatibility would be the best place to do this. It used to work for ft.search commands, but I see that those tests have been commented out, which leads me to think that it's not working for those commands. But fixing it should be relatively easy.

I think the latest version should work with ft.search commands. I fixed some bugs and I've been using it.

This would be the place to test numeric conversion errors.

src/commands/ft_search.cc

allenss-amazon

There seem to be several changes in the PR that are either just formatting or perhaps some temporary changes for debugging purposes. Let's get those cleaned up. Otherwise, the basic core functionality looks correct to me.

Also, I'd definitely want to see some test cases in the compatibility test suite

src/commands/ft_search.cc

allenss-amazon · 2026-01-09T15:57:30Z

integration/compatibility_test.py

-            data_set = do_answer(answers[i], data_set)
+            data_set = do_answer(self.server.get_new_client(), answers[i], data_set)

        print(f"Correct answers: {correct_answers} out of {len(answers)}")


Are changes here intentional? If so, can you explain?

This is the part of compatibility tests that is commented out. I am looking into writing those numeric tests that you asked for in this file, but I have been having some trouble with the build environment for a few days.

I reverted this change in the latest commit

allenss-amazon · 2026-01-09T15:58:10Z

src/commands/ft_search.cc

+  // Support non-vector queries
+  if (IsNonVectorQuery()) {
+    query::ProcessNonVectorNeighborsForReply(
+        ctx, index_schema->GetAttributeDataType(), neighbors, *this);
+    ApplySorting(neighbors, *this);
+    SerializeNonVectorNeighbors(ctx, search_result, *this);
+    return;
+  }


Why restrict this to non-vector?

I didn't catch this when resolving a merge conflict. It was supposed to be a single added line to the if statement above. I'll go over the entire change again to make sure nothing else slipped through.

allenss-amazon · 2026-01-09T16:12:36Z

src/commands/ft_search_parser.cc

        << "Error parsing vector similarity parameters: ";
  }
+
+  if (auto searchCommand = dynamic_cast<SearchCommand *>(&parameters);


Anytime I see a dynamic_cast, I wonder if this isn't a band-aid on some incorrect object structure. Here, it looks like the preparse and postparse functions should be virtual, that would eliminate the need for the dynamic cast.

allenss-amazon · 2026-01-09T16:14:57Z

src/commands/ft_search_parser.h

+struct SortByParameter {
+  std::string field;
+  SortOrder order{SortOrder::kAscending};
+  bool enabled{false};


Rather than an explicit enabled flag, perhaps this should be std::optional ?

allenss-amazon · 2026-01-09T16:17:26Z

testing/ft_search_parser_test.cc

+            .k = 0,
+            .ef = 0,
+            .score_as = "",
+            .vector_query = false,
+            .sortby_parameters_str = "SORTBY attribute_identifier_1 DESC",
+            .sortby_field = "attribute_identifier_1",
+            .sortby_order = SortOrder::kDescending,
+            .sortby_enabled = true,
+        },


Let's tweak some of the keywords with one or more lower case letters to ensure that we're getting the case-invariant matching correct.

allenss-amazon · 2026-01-09T16:18:23Z

testing/ft_search_test.cc

+                                     "embedding",
+                                     "$embedding",
+                                     "SORTBY",
+                                     "vector",


This would be the place to test numeric conversion errors.

Signed-off-by: Alexandru Filip <alexandru.filip@improving.com>

AlexFilipImproving marked this pull request as draft November 21, 2025 15:36

jduo reviewed Nov 24, 2025

View reviewed changes

COMMANDS.md Show resolved Hide resolved

jduo approved these changes Nov 24, 2025

View reviewed changes

Jonathan-Improving reviewed Nov 25, 2025

View reviewed changes

src/query/search.cc Outdated Show resolved Hide resolved

Jonathan-Improving reviewed Nov 25, 2025

View reviewed changes

testing/ft_search_parser_test.cc Show resolved Hide resolved

Jonathan-Improving approved these changes Nov 25, 2025

View reviewed changes

allenss-amazon suggested changes Nov 25, 2025

View reviewed changes

src/commands/ft_search_parser.cc Outdated Show resolved Hide resolved

src/query/search.cc Outdated Show resolved Hide resolved

AlexFilipImproving requested a review from allenss-amazon November 26, 2025 19:45

allenss-amazon suggested changes Nov 27, 2025

View reviewed changes

AlexFilipImproving force-pushed the feature/sortby-implementation branch from 0f89d05 to 8e0de57 Compare November 28, 2025 17:12

allenss-amazon suggested changes Dec 2, 2025

View reviewed changes

AlexFilipImproving force-pushed the feature/sortby-implementation branch 2 times, most recently from 021dec9 to d758c86 Compare December 10, 2025 19:49

AlexFilipImproving force-pushed the feature/sortby-implementation branch 2 times, most recently from 7349606 to bff42e1 Compare January 8, 2026 20:47

allenss-amazon suggested changes Jan 9, 2026

View reviewed changes

AlexFilipImproving force-pushed the feature/sortby-implementation branch from edb271a to cd56346 Compare January 12, 2026 18:58

AlexFilipImproving mentioned this pull request Jan 16, 2026

Feature/sortby implementation valkey-io/valkey-search#618

Open

AlexFilipImproving force-pushed the feature/sortby-implementation branch from 3a35723 to 645bb26 Compare January 19, 2026 16:16

AlexFilipImproving force-pushed the main branch from 2694c9b to c2c2491 Compare January 19, 2026 16:50

feat: Implement SORTBY in FT.SEARCH. All previous commits squashed.

62f13e1

Signed-off-by: Alexandru Filip <alexandru.filip@improving.com>

AlexFilipImproving force-pushed the feature/sortby-implementation branch from 8defc22 to 62f13e1 Compare January 28, 2026 20:02

AlexFilipImproving added 3 commits January 28, 2026 15:03

Merge branch 'main' into feature/sortby-implementation

eb99755

Add RequiresCompleteResults override

7ddc6bb

Signed-off-by: Alexandru Filip <alexandru.filip@improving.com>

Generate new answers file

047d3b5

Signed-off-by: Alexandru Filip <alexandru.filip@improving.com>

Implement SORTBY on FT.SEARCH #1

Are you sure you want to change the base?

Implement SORTBY on FT.SEARCH #1

Uh oh!

Conversation

AlexFilipImproving commented Nov 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jonathan-Improving left a comment

Choose a reason for hiding this comment

Uh oh!

allenss-amazon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

allenss-amazon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexFilipImproving commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allenss-amazon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenss-amazon Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

allenss-amazon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

allenss-amazon left a comment •

edited

Loading

AlexFilipImproving commented Nov 27, 2025 •

edited

Loading

allenss-amazon Dec 8, 2025 •

edited

Loading