Skip to content
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

fix(search_family): Add options test for the FT.AGGREGATE command #4479

Conversation

BagritsevichStepan
Copy link
Contributor

@BagritsevichStepan BagritsevichStepan commented Jan 20, 2025

related to the #4204

  1. Fix some small issues during parsing of the several SEARCH methods
  2. Remove using builder in Parse* methods.
    It was a bad decision to use it because, in some cases, we were returning an error with builder->SendError(...). However, in those cases, we were not handling parser errors with parser->Error(). As a result, debug builds were crashing because of this check:
CmdArgParser::~CmdArgParser() {
  DCHECK(!error_.has_value()) << "Parsing error occured but not checked";
}

Now, we ensure that parsing errors are processed in all cases.
3. Add options test for the FT.AGGREGATE command

@BagritsevichStepan BagritsevichStepan force-pushed the search/ft-aggregate-add-option-tests branch from 3570612 to 4926c1d Compare February 9, 2025 12:43
@BagritsevichStepan BagritsevichStepan force-pushed the search/ft-aggregate-add-option-tests branch from 4926c1d to a047d47 Compare February 9, 2025 12:59
@BagritsevichStepan BagritsevichStepan force-pushed the search/ft-aggregate-add-option-tests branch from a047d47 to 9dea4fd Compare February 11, 2025 10:20
Comment on lines +265 to +267
if (!parse_result.value()) {
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?
For ParseSchema, we return false. It is the last option that should be parsed.

Copy link
Contributor

@BorysTheDev BorysTheDev Feb 11, 2025

Choose a reason for hiding this comment

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

you already have a check if (!parse_result) so parse_result.value() is always exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_result is io::Error<bool>, so when I prove if (!parse_result), I prove that no error occurred. When I check if (!parse_result.value()), I prove that the returned result is false.

@BagritsevichStepan BagritsevichStepan merged commit 6032358 into dragonflydb:main Feb 11, 2025
10 checks passed
@BagritsevichStepan BagritsevichStepan deleted the search/ft-aggregate-add-option-tests branch February 11, 2025 13:12
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