Skip to content
New issue

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

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

Already on GitHub? # to your account

Changing AggregationRequest to Generate Based on Provided Argument Order #259

Merged
merged 11 commits into from
Mar 7, 2024

Conversation

shacharPash
Copy link
Contributor

@shacharPash shacharPash commented Feb 29, 2024

Before this PR, the FT.AGGREGATE arguments would always appear in this order, 0 or 1 times, regardless of the order in which the user defined them:

public void SerializeRedisArgs()
{
    Verbatim();
    Load();
    Timeout();
    Apply();
    GroupBy();
    SortBy();
    Limit();
    Filter();
    Cursor();
    Params();
    Dialect();
}

According to the aggregation Core concepts :

The basic idea of an aggregate query is this:

  • Perform a search query, filtering for records you wish to process.
  • Build a pipeline of operations that transform the results by zero or more sequences of:
    • Group and reduce: grouping by fields in the results, and applying reducer functions to each group.
    • Sort: sort the results based on one or more fields.
    • Apply transformations: apply mathematical and string functions on fields in the pipeline, optionally creating new fields or replacing existing ones.
    • Limit: limit the result, regardless of how the result is sorted.
    • Filter: filter the results (post-query) based on predicates relating to its values.

The pipeline is dynamic and re-entrant, and every operation can be repeated. For example, you can group by property X, sort the top 100 results by group size, then group by property Y and sort the results by some other property, then apply a transformation on the output.

So, after this PR the user can write more than one of each argument and the order is the order the user call the arguments.

Closes #230

@shacharPash shacharPash added the breakingchange API or breaking change label Feb 29, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 98.21429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 94.17%. Comparing base (2ec5430) to head (7c134e5).
Report is 1 commits behind head on master.

Files Patch % Lines
src/NRedisStack/Search/AggregationRequest.cs 97.67% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
- Coverage   94.27%   94.17%   -0.11%     
==========================================
  Files          87       87              
  Lines        5367     5270      -97     
  Branches      503      489      -14     
==========================================
- Hits         5060     4963      -97     
  Misses        181      181              
  Partials      126      126              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shacharPash shacharPash marked this pull request as ready for review March 3, 2024 13:25
@shacharPash shacharPash requested review from chayim, gerzse and a team March 3, 2024 13:26
Copy link
Contributor

@gerzse gerzse left a comment

Choose a reason for hiding this comment

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

LGTM, just some cosmetic comments.

/// <param name="phonetic">Declaring a text attribute as PHONETIC will perform phonetic matching on it in searches by default.</param>
/// <param name="noIndex">Attributes can have the NOINDEX option, which means they will not be indexed.</param>
/// <param name="unf">Set this to true to prevent the indexer from sorting on the normalized form.
/// <param name=SearchArgs.name>The field's name.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

These docstring changes should be reverted, I guess.


private void SortBy()
public AggregationRequest SortBy(int max, params SortedField[] fields) // TODO: check if it should be params
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like no harm to make it params. But then the same can be done for other methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure because this signature allows to send only MAX without any field, although at least one field must be sent, thats why I added the if(fields.Length > 0),
but right now I looked again and this is the syntax of sortby:
[ SORTBY nargs [ property ASC | DESC [ property ASC | DESC ...]] [MAX num]
so sending only MAX without any field should be possible.

@shacharPash shacharPash requested a review from gerzse March 6, 2024 09:37
@shacharPash shacharPash merged commit ee56859 into master Mar 7, 2024
18 checks passed
@shacharPash shacharPash deleted the Issue245/ftAggregateTimeout branch March 7, 2024 11:46
@shacharPash shacharPash changed the title Fix AggregationRequest Builder Changing AggregationRequest to Generate Based on Provided Argument Order Mar 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
breakingchange API or breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong query when aggregating with filter
3 participants