add missing aggregation part 2 #2149
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #2149 +/- ##
==========================================
+ Coverage 94.32% 94.45% +0.12%
==========================================
Files 320 321 +1
Lines 62032 62424 +392
==========================================
+ Hits 58514 58961 +447
+ Misses 3518 3463 -55
|
| @@ -54,38 +59,68 @@ impl AggregationWithAccessor { | |||
| reader: &SegmentReader, | |||
| limits: AggregationLimits, | |||
| ) -> crate::Result<Vec<AggregationWithAccessor>> { | |||
| let mut missing_value_term_agg = None; | |||
| let mut str_dict_column = None; | |||
| let get_agg_with_accessor = |aggs: &mut Vec<AggregationWithAccessor>, | |||
There was a problem hiding this comment.
does this have to be inlined? can it be a proper funciton?
There was a problem hiding this comment.
nitpick:
I suggest we try to stick to a rule for argument lists: the mut stuff at the end of the list of arguments.
This is arbitrary. (that's the rule enforced at Google)
There was a problem hiding this comment.
It sounds like it could have been a function with a clearer prototype:
Fn(.., .., .., .., ..) -> crate::Result<Option<AggregationWithAccessor>> {}
There was a problem hiding this comment.
The closure has access to the parameters and is mainly there to remove code duplication, moving it to its own function has too many parameters imo
fn add_agg_with_accessor(
aggs: &mut Vec<AggregationWithAccessor>,
accessor: Column<u64>,
column_type: ColumnType,
str_dict_column: Option<StrColumn>,
missing_value_term_agg: Option<Key>,
agg: &Aggregation,
sub_aggregation: &Aggregations,
reader: &SegmentReader,
limits: AggregationLimits,
) -> crate::Result<()> {I refactored it a little bit and added types to the closure instead.
nitpick:
I suggest we try to stick to a rule for argument lists: the mut stuff at the end of the list of arguments.
This is arbitrary. (that's the rule enforced at Google)
I'm not really in favor of that, since this would cause churn when refactoring something to mut for very little gain (at least for how I read code).
I changed it in this case though.
Add missing support for: - Mixed types columns - Key of type string on numerical fields The special aggregation is slower than the integrated one in TermsAggregation and therefore not chosen by default, although it can cover all use cases.
Add missing support for:
The special aggregation is slower than the integrated one in TermsAggregation and therefore not
chosen by default, although it can cover all use cases.
closes #1789