Add reverse support in aggregation_resolver
#5084
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5084 +/- ##
==========================================
- Coverage 84.86% 82.97% -1.89%
==========================================
Files 332 332
Lines 18304 18324 +20
==========================================
- Hits 15534 15205 -329
- Misses 2770 3119 +349
Help us with your feedback. Take ten seconds to tell us how you rate us. |
|
This seems important, but I wonder if we could just define the equivalent aggregation on the aggregation class itself. Not sure if its better. Just a thoguht. |
Thanks for the suggestion. I am open to this solution as well. |
|
Sorry for the delay in review. |
| base_cls_repr = camel_to_snake(base_cls.__name__) | ||
|
|
||
| if not isinstance(query, base_cls): | ||
| choices = {base_cls_repr} | set( |
There was a problem hiding this comment.
maybe we should have a test case covering this? the logic is not trivial
| @@ -80,14 +122,21 @@ def normalization_resolver(query: Union[Any, str], *args, **kwargs): | |||
| # Aggregation Resolver ######################################################## | |||
|
|
|||
|
|
|||
| def aggregation_resolver(query: Union[Any, str], *args, **kwargs): | |||
| def aggregation_resolver(query: Union[Any, str], *args, reverse: bool = False, | |||
There was a problem hiding this comment.
I'm not certain I like having a 'reverse' flag because it makes the logic for 'query' a bit complicated. What do you think about splitting this into reverse_aggregation_resolver?
This PR added the
reversesupport inaggregation_resolverto resolveAggregationto strings. This will keepmessage_and_aggregatefunctionality intact if an aggr isFUSE_AGGRSbut passed withAggregationinstance.Addresses #4712.