alternative mixed field aggregation collection

[PSeitz]

instead of having multiple accessor in one AggregationWithAccessor split it into
multiple independent AggregationWithAccessor

Performance seems unchanged

Before
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg                            ... bench: 173,939,311 ns/iter (+/- 35,629,524)
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg_multi                      ... bench: 227,312,130 ns/iter (+/- 34,251,990)
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg_opt                        ... bench: 216,645,922 ns/iter (+/- 72,184,100)
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg_sparse                     ... bench:  43,224,161 ns/iter (+/- 11,300,519)

After
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg                            ... bench: 171,519,343 ns/iter (+/- 76,513,404)
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg_multi                      ... bench: 228,347,835 ns/iter (+/- 12,738,742)
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg_opt                        ... bench: 218,119,685 ns/iter (+/- 72,979,994)
test aggregation::agg_bench::bench::bench_aggregation_terms_many_json_mixed_type_with_sub_agg_sparse                     ... bench:  41,439,166 ns/iter (+/- 10,048,723)

[adamreichold]

I think the return type fixes the types here, so that let aggs: ..., Ok(aggs) and the ? are unnecessary, i.e. it could just end in }).collect().

[PSeitz]

I don't understand, we need to collect to Result to forward the error

[adamreichold]

Certainly, but the extra binding and type hints seem unnecessary because the type that is collected already matches the return type, does it not?

So in think the function could just end in

        acc_field_types
            .into_iter()
            .map(|(accessor, field_type)| {
                Ok(AggregationWithAccessor {
                    accessor,
                    field_type,
                    sub_aggregation: get_aggs_with_segment_accessor_and_validate(
                        sub_aggregation,
                        reader,
                        &limits,
                    )?,
                    agg: agg.clone(),
                    str_dict_column: str_dict_column.clone(),
                    limits: limits.new_guard(),
                    column_block_accessor: Default::default(),
                })
            })
            .collect()

[PSeitz]

we return a Result<Vec> not Vec<Result>

[adamreichold]

Yes, which is what you are collecting by giving the type hint ::<crate::Result<_>> but that type hint just matches the return type. I am not suggesting to change any types, just to drop the unnecessary temporary binding and error forwarding because the collection already produces exactly the required type.

(Turning Iterator<Item=Result<S>> into Result<T> where T: FromIterator<Item=S> is the usual FromIterator impl which you invoke either way.)

[PSeitz]

I think the binding with type assignment increases readability

[adamreichold]

You can still spell out the full return type if desired in the turbo-fish if you want to, e.g.

.collect::<crate::Result<Vec<AggregationWithAccessor>>>()

but the extra binding, error forwarding and ok wrapping are unnecessary.

(There is even a Clippy lint for unnecessary let bindings but IIRC it does not firing when ok wrapping is involved because this is sometimes necessary to change the error type via the From invocation hidding in ?.)

[PSeitz]

I don't agree, the binding and type serves the purpose of increased readability by giving it a name and a type, which is important if the transformation gets nested

[fulmicoton]

I agree with @PSeitz . It is more readable when we put the type on the variable side.

[fulmicoton]

Thanks for putting the type!