Skip to content

Commit

Permalink
fix(search_family): Temporary remove the error when a field name does…
Browse files Browse the repository at this point in the history
… not have the '@' sign at the beginning in the FT.AGGREGATE command (#3956)
  • Loading branch information
BagritsevichStepan authored Oct 21, 2024
1 parent 478a5d4 commit e96a99a
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 5 deletions.
18 changes: 13 additions & 5 deletions src/server/search/search_family.cc
Original file line number Diff line number Diff line change
Expand Up @@ -238,12 +238,15 @@ std::string_view ParseField(CmdArgParser* parser) {
return field;
}

std::optional<std::string_view> ParseFieldWithAtSign(CmdArgParser* parser) {
std::string_view ParseFieldWithAtSign(CmdArgParser* parser) {
std::string_view field = parser->Next();
if (field.front() != '@') {
return std::nullopt; // if we expect @, but it's not there, return nullopt
// Temporary warning until we can throw an error
LOG(WARNING) << "bad arguments: Field name '" << field << "' should start with '@'. '@" << field
<< "' is expected";
} else {
field.remove_prefix(1); // remove leading @
}
field.remove_prefix(1); // remove leading @
return field;
}

Expand Down Expand Up @@ -273,12 +276,17 @@ optional<AggregateParams> ParseAggregatorParamsOrReply(CmdArgParser parser,
vector<string_view> fields(parser.Next<size_t>());
for (string_view& field : fields) {
auto parsed_field = ParseFieldWithAtSign(&parser);

/*
TODO: Throw an error if the field has no '@' sign at the beginning
if (!parsed_field) {
cntx->SendError(absl::StrCat("bad arguments for GROUPBY: Unknown property '", field,
"'. Did you mean '@", field, "`?"));
return nullopt;
}
field = parsed_field.value();
} */

field = parsed_field;
}

vector<aggregate::Reducer> reducers;
Expand Down
16 changes: 16 additions & 0 deletions src/server/search/search_family_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,22 @@ TEST_F(SearchFamilyTest, JsonAggregateGroupBy) {
EXPECT_THAT(resp, IsUnordArrayWithSize(IsMap("avg_price", "20")));
}

TEST_F(SearchFamilyTest, JsonAggregateGroupByWithoutAtSign) {
Run({"HSET", "h1", "group", "first", "value", "1"});
Run({"HSET", "h2", "group", "second", "value", "2"});
Run({"HSET", "h3", "group", "first", "value", "3"});

auto resp =
Run({"FT.CREATE", "index", "ON", "HASH", "SCHEMA", "group", "TAG", "value", "NUMERIC"});
EXPECT_EQ(resp, "OK");

// TODO: Throw an error when no '@' is provided in the GROUPBY option
resp = Run({"FT.AGGREGATE", "index", "*", "GROUPBY", "1", "group", "REDUCE", "COUNT", "0", "AS",
"count"});
EXPECT_THAT(resp, IsUnordArrayWithSize(IsMap("count", "2", "group", "first"),
IsMap("group", "second", "count", "1")));
}

TEST_F(SearchFamilyTest, AggregateGroupByReduceSort) {
for (size_t i = 0; i < 101; i++) { // 51 even, 50 odd
Run({"hset", absl::StrCat("k", i), "even", (i % 2 == 0) ? "true" : "false", "value",
Expand Down

0 comments on commit e96a99a

Please sign in to comment.