[fix] fixes APPLY / SORTBY / GROUPBY / REDUCE order on FT.AGGREGATE s…

[filipecosta90]

This is a draft PR open for discussion, fixing AggregateRequest incorrect order of clauses.

Fixes #40

Overall changes:

• [add] added query builder tests to circleci
• [add] added # Test with apply testcase to testbuilder.py
• [fix] fixed Single field, single reducer, Multiple fields, single reducer, and Multiple fields, multiple reducers testcases on testbuilder.py
• [rem] removed self._groups = [], self._projections = [] and self._sortby = [] in exchange of self._aggregateplan = [] which enforces the order of the clauses to be the order of the method calls.

@stryt2, would appreciate your comments regarding the changes.

[MeirShpilraien]

Looks good, small comments.
In addition I did not see the FILTER implementation, did I missed it?
Last I know it is also currently missing but reducers can also have alias, maybe its a good chance to add it? not a must though, its you chose ...

[filipecosta90]

Looks good, small comments.
In addition I did not see the FILTER implementation, did I missed it?
Last I know it is also currently missing but reducers can also have alias, maybe its a good chance to add it? not a must though, its you chose ...

FILTER was not in-place on redisearch-py. Will add it as well.
Will also accommodate the recommended changes. tks @MeirShpilraien

[filipecosta90]

Looks good, small comments.
In addition I did not see the FILTER implementation, did I missed it?
Last I know it is also currently missing but reducers can also have alias, maybe its a good chance to add it? not a must though, its you chose ...

@MeirShpilraien the latest commit:

• added support for filter expressions on aggregations
• added example for aggregation reducer with alias. ( see test_builder file https://github.com/filipecosta90/redisearch-py/blob/ft.aggregate/test/test_builder.py#L65 )

[stryt2]

Thanks for the quick fix. I think the fix looks good as well.

However, another comment on the FILTER, from the Redisearch Aggregation API Reference, it says

Several filter steps can be added, ...

Disregard efficiency, I think the filter(self, expressions) method should also allow multiple filters being added at different stages of the aggregation pipeline; unless having the FILTERs at different stages down the pipeline never effects the aggregation result.

But otherwise, the fix looks solid. Thanks again.

[filipecosta90]

Thanks for the quick fix. I think the fix looks good as well.

However, another comment on the FILTER, from the Redisearch Aggregation API Reference, it says

Several filter steps can be added, ...

Disregard efficiency, I think the filter(self, expressions) method should also allow multiple filters being added at different stages of the aggregation pipeline; unless having the FILTERs at different stages down the pipeline never effects the aggregation result.

hi there @stryt2 , with regards to the "multiple filters being added at different stages of the aggregation pipeline", you're right. I was enforcing them to be after all other clauses due to my interpretation of The FILTER expressions are evaluated post-query and relate to the current state of the pipeline..
will double-check that the filters vs other clauses order will have different outcomes. If so, will correct it. Thanks @stryt2 =)

[MeirShpilraien]

@filipecosta90 did you fix the multiple filters comment? Also a rebase is required.

[filipecosta90]

@filipecosta90 did you fix the multiple filters comment? Also a rebase is required.

will do it today @MeirShpilraien 👍

[filipecosta90]

@MeirShpilraien @stryt2 the latest commit should address the FILTER clause issue.