refactor: address comments

Signed-off-by: Stepan Bagritsevich <[email protected]>
dragonflydb · Nov 3, 2024 · 95d86c2 · 95d86c2
1 parent f1ca477
commit 95d86c2
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 82 deletions.
diff --git a/src/server/search/doc_accessors.cc b/src/server/search/doc_accessors.cc
@@ -79,8 +79,8 @@ search::SortableValue ExtractSortableValueFromJson(const search::Schema& schema,
 
 }  // namespace
 
-SearchDocData BaseAccessor::Serialize(
-    const search::Schema& schema, absl::Span<const SearchField<std::string_view>> fields) const {
+SearchDocData BaseAccessor::Serialize(const search::Schema& schema,
+                                      absl::Span<const SearchField> fields) const {
   SearchDocData out{};
   for (const auto& field : fields) {
     const auto& fident = field.GetIdentifier(schema, false);
@@ -256,12 +256,12 @@ JsonAccessor::JsonPathContainer* JsonAccessor::GetPath(std::string_view field) c
 SearchDocData JsonAccessor::Serialize(const search::Schema& schema) const {
   SearchFieldsList fields{};
   for (const auto& [fname, fident] : schema.field_names)
-    fields.emplace_back(fident, NameType::kIdentifier, fname);
+    fields.emplace_back(StringOrView::FromView(fident), false, StringOrView::FromView(fname));
   return Serialize(schema, fields);
 }
 
-SearchDocData JsonAccessor::Serialize(
-    const search::Schema& schema, absl::Span<const SearchField<std::string_view>> fields) const {
+SearchDocData JsonAccessor::Serialize(const search::Schema& schema,
+                                      absl::Span<const SearchField> fields) const {
   SearchDocData out{};
   for (const auto& field : fields) {
     const auto& ident = field.GetIdentifier(schema, true);

diff --git a/src/server/search/doc_accessors.h b/src/server/search/doc_accessors.h
@@ -29,7 +29,7 @@ struct BaseAccessor : public search::DocumentAccessor {
 
   // Serialize selected fields
   virtual SearchDocData Serialize(const search::Schema& schema,
-                                  absl::Span<const SearchField<std::string_view>> fields) const;
+                                  absl::Span<const SearchField> fields) const;
 
   /*
   Serialize the whole type, the default implementation is to serialize all fields.
@@ -80,7 +80,7 @@ struct JsonAccessor : public BaseAccessor {
 
   // The JsonAccessor works with structured types and not plain strings, so an overload is needed
   SearchDocData Serialize(const search::Schema& schema,
-                          absl::Span<const SearchField<std::string_view>> fields) const override;
+                          absl::Span<const SearchField> fields) const override;
   SearchDocData Serialize(const search::Schema& schema) const override;
   SearchDocData SerializeDocument(const search::Schema& schema) const override;
 

diff --git a/src/server/search/doc_index.cc b/src/server/search/doc_index.cc
@@ -217,7 +217,7 @@ SearchFieldsList ToSV(const search::Schema& schema,
   if (fields) {
     sv_fields.reserve(fields->size());
     for (const auto& field : fields.value()) {
-      sv_fields.emplace_back(field);
+      sv_fields.push_back(field.View());
     }
   }
   return sv_fields;
@@ -289,7 +289,7 @@ vector<SearchDocData> ShardDocIndex::SearchForAggregator(
   absl::flat_hash_map<std::string_view, std::string_view> sort_indicies_aliases;
   if (params.load_fields) {
     for (const auto& field : params.load_fields.value()) {
-      auto ident = field.GetIdentifier(base_->schema);
+      auto ident = field.GetIdentifier(base_->schema, false);
       if (sort_indicies_fields.contains(ident)) {
         sort_indicies_aliases[ident] = field.GetShortName();
       }
@@ -319,21 +319,20 @@ vector<SearchDocData> ShardDocIndex::SearchForAggregator(
 SearchFieldsList ShardDocIndex::GetFieldsToLoad(
     const std::optional<OwnedSearchFieldsList>& load_fields,
     const absl::flat_hash_set<std::string_view>& skip_fields) const {
-  absl::flat_hash_map<std::string_view, SearchField<std::string_view>> unique_fields;
+  absl::flat_hash_map<std::string_view, SearchField> unique_fields;
   unique_fields.reserve(base_->schema.field_names.size());
 
   for (const auto& [fname, fident] : base_->schema.field_names) {
     if (!skip_fields.contains(fident)) {
-      unique_fields[fident] = {std::string_view{fident}, NameType::kIdentifier,
-                               std::string_view{fname}};
+      unique_fields[fident] = {StringOrView::FromView(fident), true, StringOrView::FromView(fname)};
     }
   }
 
   if (load_fields) {
     for (const auto& field : load_fields.value()) {
-      const auto& fident = field.GetIdentifier(base_->schema);
+      const auto& fident = field.GetIdentifier(base_->schema, false);
       if (!skip_fields.contains(fident)) {
-        unique_fields[fident] = field;
+        unique_fields[fident] = field.View();
       }
     }
   }

diff --git a/src/server/search/doc_index.h b/src/server/search/doc_index.h
@@ -52,13 +52,13 @@ struct SearchResult {
   std::optional<facade::ErrorReply> error;
 };
 
-enum class NameType : uint8_t { kIdentifier, kShortName, kUndefined };
-
-template <typename T> class SearchField {
+/* SearchField represents a field that can store combinations of identifiers and aliases in various
+   forms: [identifier and alias], [alias and new_alias], [new identifier and alias] (used for JSON
+   data) This class provides methods to retrieve the actual identifier and alias for a field,
+   handling different naming conventions and resolving names based on the schema. */
+class SearchField {
  private:
-  using SingleName = std::pair<T /*identifier or short name*/, NameType>;
-
-  static bool IsJsonPath(const T& name) {
+  static bool IsJsonPath(std::string_view name) {
     if (name.size() < 2) {
       return false;
     }
@@ -68,89 +68,67 @@ template <typename T> class SearchField {
  public:
   SearchField() = default;
 
-  SearchField(T name, NameType name_type) : name_(std::make_pair(std::move(name), name_type)) {
-  }
-
-  SearchField(T name, NameType name_type, T new_alias)
-      : name_(std::make_pair(std::move(name), name_type)), new_alias_(std::move(new_alias)) {
-  }
-
-  template <typename U, typename = std::enable_if_t<std::is_constructible_v<T, U>>>
-  explicit SearchField(const SearchField<U>& other)
-      : name_(std::make_pair(T{other.name_.first}, other.name_.second)) {
-    if (other.HasNewAlias()) {
-      new_alias_ = T{other.new_alias_.value()};
-    } else {
-      new_alias_.reset();
-    }
-  }
-
-  template <typename U, typename = std::enable_if_t<std::is_constructible_v<T, U>>>
-  SearchField& operator=(const SearchField<U>& other) {
-    name_ = std::make_pair(T{other.name_.first}, other.name_.second);
-    if (other.HasNewAlias()) {
-      new_alias_ = T{other.new_alias_.value()};
-    } else {
-      new_alias_.reset();
-    }
-    return *this;
+  SearchField(StringOrView name, bool is_short_name)
+      : name_(std::move(name)), is_short_name_(is_short_name) {
   }
 
-  ~SearchField() = default;
-
-  std::string_view GetIdentifier(const search::Schema& schema) const {
-    return GetIdentifier(schema, [&](const SingleName& single_name) {
-      return single_name.second == NameType::kIdentifier;
-    });
+  SearchField(StringOrView name, bool is_short_name, StringOrView new_alias)
+      : name_(std::move(name)), is_short_name_(is_short_name), new_alias_(std::move(new_alias)) {
   }
 
   std::string_view GetIdentifier(const search::Schema& schema, bool is_json_field) const {
-    return GetIdentifier(schema, [&](const SingleName& single_name) {
-      return single_name.second == NameType::kIdentifier ||
-             (is_json_field && IsJsonPath(single_name.first));
-    });
+    auto as_view = NameView();
+    if (!is_short_name_ || (is_json_field && IsJsonPath(as_view))) {
+      return as_view;
+    }
+    return schema.LookupAlias(as_view);
   }
 
   std::string_view GetShortName() const {
     if (HasNewAlias()) {
-      return new_alias_.value();
+      return AliasView();
     }
-    return name_.first;
+    return NameView();
   }
 
   std::string_view GetShortName(const search::Schema& schema) const {
     if (HasNewAlias()) {
-      return new_alias_.value();
+      return AliasView();
     }
-
-    if (name_.second == NameType::kShortName) {
-      return name_.first;
-    }
-    return schema.LookupIdentifier(std::string_view{name_.first});
+    return is_short_name_ ? NameView() : schema.LookupIdentifier(NameView());
   }
 
- private:
-  template <typename Callback>
-  std::string_view GetIdentifier(const search::Schema& schema, Callback is_identifier) const {
-    if (is_identifier(name_)) {
-      return name_.first;
+  /* Returns a new SearchField instance with name and alias stored as views to the values in this
+   * SearchField */
+  SearchField View() const {
+    if (HasNewAlias()) {
+      return SearchField{StringOrView::FromView(NameView()), is_short_name_,
+                         StringOrView::FromView(AliasView())};
     }
-    return schema.LookupAlias(std::string_view{name_.first});
+    return SearchField{StringOrView::FromView(NameView()), is_short_name_};
   }
 
+ private:
   bool HasNewAlias() const {
     return new_alias_.has_value();
   }
 
-  template <typename U> friend class SearchField;
+  std::string_view NameView() const {
+    return name_.view();
+  }
+
+  std::string_view AliasView() const {
+    return new_alias_.value().view();
+  }
 
  private:
-  SingleName name_;
-  std::optional<T> new_alias_;
+  StringOrView name_;
+  bool is_short_name_;
+  std::optional<StringOrView> new_alias_;
 };
 
-using SearchFieldsList = std::vector<SearchField<std::string_view>>;
-using OwnedSearchFieldsList = std::vector<SearchField<std::string>>;
+using SearchFieldsList = std::vector<SearchField>;
+using OwnedSearchFieldsList = std::vector<SearchField>;
 
 struct SearchParams {
   // Parameters for "LIMIT offset total": select total amount documents with a specific offset from

diff --git a/src/server/search/search_family.cc b/src/server/search/search_family.cc
@@ -221,11 +221,12 @@ void ParseLoadFields(CmdArgParser* parser, std::optional<OwnedSearchFieldsList>*
       str.remove_prefix(1);  // remove leading @
     }
 
+    StringOrView name = StringOrView::FromString(std::string{str});
     if (parser->Check("AS")) {
-      load_fields->value().emplace_back(std::string{str}, NameType::kShortName,
-                                        std::string{parser->Next()});
+      load_fields->value().emplace_back(name, true,
+                                        StringOrView::FromString(std::string{parser->Next()}));
     } else {
-      load_fields->value().emplace_back(std::string{str}, NameType::kShortName);
+      load_fields->value().emplace_back(name, true);
     }
   }
 }
@@ -267,13 +268,13 @@ optional<SearchParams> ParseSearchParamsOrReply(CmdArgParser parser, SinkReplyBu
       size_t num_fields = parser.Next<size_t>();
       params.return_fields.emplace();
       while (params.return_fields->size() < num_fields) {
-        std::string_view str = parser.Next();
+        StringOrView name = StringOrView::FromString(std::string{parser.Next()});
 
         if (parser.Check("AS")) {
-          params.return_fields->emplace_back(std::string{str}, NameType::kShortName,
-                                             std::string{parser.Next()});
+          params.return_fields->emplace_back(std::move(name), true,
+                                             StringOrView::FromString(std::string{parser.Next()}));
         } else {
-          params.return_fields->emplace_back(std::string{str}, NameType::kShortName);
+          params.return_fields->emplace_back(std::move(name), true);
         }
       }
     } else if (parser.Check("NOCONTENT")) {  // NOCONTENT