refactor: address comments

Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>

Author: BagritsevichStepan

src/server/search/doc_index.cc

@@ -94,6 +94,24 @@ std::pair<std::vector<FieldReference>, SortIndiciesFieldsList> PreprocessAggrega
   return {std::move(fields), {sort_indicies_aliases.begin(), sort_indicies_aliases.end()}};
 }
 
+/* Separate fields into basic and sortable. The second vector contains flags indicating
+   whether the field at the same index in the first vector is sortable or not. */
+std::pair<std::vector<FieldReference>, std::vector<bool>> GetBasicFields(
+    absl::Span<const std::string_view> fields, const search::Schema& schema) {
+  const size_t fields_count = fields.size();
+  std::vector<bool> is_sortable_field(fields_count);
+  std::vector<FieldReference> basic_fields;
+  basic_fields.reserve(fields_count);
+  for (size_t i = 0; i < fields_count; ++i) {
+    bool is_sortable = IsSortableField(fields[i], schema);
+    is_sortable_field[i] = is_sortable;
+    if (!is_sortable) {
+      basic_fields.emplace_back(fields[i]);
+    }
+  }
+  return {std::move(basic_fields), std::move(is_sortable_field)};
+}
+
 }  // namespace
 
 bool FieldReference::IsJsonPath(std::string_view name) {
@@ -497,19 +515,8 @@ join::Vector<join::OwnedEntry> ShardDocIndex::PreagregateDataForJoin(
     search::SearchAlgorithm* search_algo) const {
   auto search_results = search_algo->Search(&*indices_);
 
-  // First filter out sortable and non-sortable fields
-  // We will load them in different ways
   const size_t fields_count = join_fields.size();
-  std::vector<bool> is_sortable_field(fields_count);
-  std::vector<FieldReference> basic_fields;
-  basic_fields.reserve(fields_count);
-  for (size_t i = 0; i < fields_count; ++i) {
-    bool is_sortable = IsSortableField(join_fields[i], base_->schema);
-    is_sortable_field[i] = is_sortable;
-    if (!is_sortable) {
-      basic_fields.emplace_back(join_fields[i]);
-    }
-  }
+  const auto [basic_fields, is_sortable_field] = GetBasicFields(join_fields, base_->schema);
 
   join::Vector<join::OwnedEntry> result;
   result.reserve(search_results.ids.size());
@@ -561,16 +568,7 @@ ShardDocIndex::FieldsValuesPerDocId ShardDocIndex::LoadKeysData(
     const OpArgs& op_args, const absl::flat_hash_set<search::DocId>& doc_ids,
     absl::Span<const std::string_view> fields_to_load) const {
   const size_t fields_count = fields_to_load.size();
-  std::vector<bool> is_sortable_field(fields_count);
-  std::vector<FieldReference> basic_fields;
-  basic_fields.reserve(fields_count);
-  for (size_t i = 0; i < fields_count; ++i) {
-    bool is_sortable = IsSortableField(fields_to_load[i], base_->schema);
-    is_sortable_field[i] = is_sortable;
-    if (!is_sortable) {
-      basic_fields.emplace_back(fields_to_load[i]);
-    }
-  }
+  const auto [basic_fields, is_sortable_field] = GetBasicFields(fields_to_load, base_->schema);
 
   FieldsValuesPerDocId result;
   result.reserve(doc_ids.size());

src/server/search/search_family.cc

@@ -9,6 +9,8 @@
 #include <absl/strings/ascii.h>
 #include <absl/strings/match.h>
 #include <absl/strings/str_format.h>
+#include <absl/strings/str_split.h>
+#include <absl/strings/string_view.h>
 
 #include <atomic>
 #include <variant>
@@ -443,12 +445,11 @@ ParseResult<aggregate::SortParams> ParseAggregatorSortParams(CmdArgParser* parse
 }
 
 std::pair<std::string_view, std::string_view> Split(std::string_view s, char delim) {
-  auto pos = s.find(delim);
-  if (pos == std::string_view::npos)
-    return {s, std::string_view{}};
-  return {s.substr(0, pos), s.substr(pos + 1)};
+  return absl::StrSplit(s, absl::MaxSplits(absl::ByChar(delim), 1));
 }
 
+// Example: LOAD_FROM index AS alias num_conditions condition [condition ...] [QUERY query]
+// condition is in the form index.field=foreign_index.field or foreign_index.field=index.field
 ParseResult<AggregateParams::JoinParams> ParseAggregatorJoinParams(
     CmdArgParser* parser, absl::flat_hash_set<std::string>* known_indexes) {
   AggregateParams::JoinParams join_params;
@@ -459,11 +460,17 @@ ParseResult<AggregateParams::JoinParams> ParseAggregatorJoinParams(
     join_params.index_alias = join_params.index;
   }
 
+  if (known_indexes->contains(join_params.index_alias)) {
+    return CreateSyntaxError(
+        absl::StrCat("Duplicate index alias in LOAD_FROM: '", join_params.index_alias, "'"));
+  }
+
   // Validate index name
   known_indexes->insert(join_params.index_alias);
 
   size_t num_fields = parser->Next<size_t>();
   join_params.conditions.reserve(num_fields);
+  // Conditions are in the form index.field=foreign_index.field or foreign_index.field=index.field
   while (parser->HasNext() && num_fields > 0) {
     auto [left, right] = Split(parser->Next(), '=');
     auto [l_index, l_field] = Split(left, '.');
@@ -729,14 +736,8 @@ join::Vector<join::Vector<join::Entry>> MergePreaggregatedShardJoinData(
         join::Vector<join::JoinableValue> field_values;
         field_values.reserve(entry.second.size());
 
-        // We need to convert std::string to std::string_view
         auto insert_copy = [&field_values](const auto& field_value) {
-          using T = std::decay_t<decltype(field_value)>;
-          if constexpr (std::is_same_v<T, std::string>) {
-            field_values.emplace_back(std::string_view{field_value});
-          } else {
-            field_values.emplace_back(field_value);
-          }
+          field_values.emplace_back(field_value);
         };
 
         for (const auto& field_value : entry.second) {

src/server/search/search_family_test.cc

@@ -3091,6 +3091,12 @@ TEST_F(SearchFamilyTest, AggregateWithLoadFromSyntaxErrors) {
   EXPECT_THAT(Run({"ft.aggregate", "idx1", "*", "LOAD", "4", "idx1.num1", "idx1.str1", "idx2.num2",
                    "idx2.str2", "LOAD_FROM", "idx2", "AS", "alias", "1", "alias.num2=idx1.num1"}),
               ErrArg("Unknown index alias 'idx2' in the LOAD option. Field: 'num2'"));
+
+  // Test same index used multiple times
+  EXPECT_THAT(Run({"ft.aggregate", "idx1", "*", "LOAD", "4", "idx1.num1", "idx1.str1", "idx2.num2",
+                   "idx2.str2", "LOAD_FROM", "idx2", "1", "idx2.num2=idx1.num1", "LOAD_FROM",
+                   "idx2", "1", "idx2.str2=idx1.str1"}),
+              ErrArg("Duplicate index alias in LOAD_FROM: 'idx2'"));
 }
 
 }  // namespace dfly