Skip to content

chore(search): Simplify return field logic #5486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 16, 2025

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Jul 12, 2025

  1. Remove effectively dead code:
  • SearchField never stores generated fields, so string_view is enough instead of StringOrView + View() + etc...
  • SearchField::is_short_name is always true
  • Remove SearchField's dead functions
  • DocumentAccessor::SerializeDocument replaced Serialize(), but the latter was never removed. Remove and swap names back
  1. Add ReturnOptionJson that extensively tests RETURN with json
  2. Remove quotes for returning plain json fields (like RS)

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg force-pushed the search-serialization-simplify branch 2 times, most recently from 0d1c496 to c521e51 Compare July 13, 2025 08:37
@dranikpg dranikpg marked this pull request as ready for review July 13, 2025 08:39
@@ -76,7 +76,7 @@ FieldValue ExtractSortableValueFromJson(const search::Schema& schema, string_vie
if (json.is_null()) {
return std::monostate{};
}
auto json_as_string = json.to_string();
auto json_as_string = json.as_string();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to create a wrapper for the json class to avoid mixing up to_string and as_string, since it’s not clear that they return different string formats (and we also call them in another place). But it’s okay.

@dranikpg dranikpg merged commit 1bf7d9b into main Jul 16, 2025
10 checks passed
@dranikpg dranikpg deleted the search-serialization-simplify branch July 16, 2025 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants