-
Notifications
You must be signed in to change notification settings - Fork 25
Feat/fed numeric agg resolution #898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1f743da
a088522
e7bc1c9
b4c7230
9412fed
f764bd0
7d1a46b
5046131
c11dd43
68ab587
f0ddc0c
e264b12
da646e3
8160e19
bba55c2
5105765
347b9e5
711b693
2388add
6d665b7
36b7fc6
762beeb
d046aea
e6f8fee
0d8a3ab
aaf9278
22405de
6876ba9
9bfaeeb
bdedb05
8ddf011
6daf0bc
10590f7
958fc6d
301fdd4
ac5b5f7
112fa1a
dc876e0
2836cf8
7f486b0
2dc800a
c108bc2
9727fec
e7d5896
e36b064
cd1b150
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,33 @@ | ||
| import { SUPPORTED_AGGREGATIONS } from '../common'; | ||
| import { Aggregations, Bucket, NumericAggregations } from '../types/aggregations'; | ||
| import { AllAggregations } from '../types/types'; | ||
| import { RequestedFieldsMap } from '../util'; | ||
|
|
||
| type ResolveAggregationInput = { | ||
| aggregationsMap: AllAggregations; | ||
| requestedAggregationFields: string[]; | ||
| accumulator: AllAggregations; | ||
| }; | ||
|
|
||
| type AggregationsTuple = [Aggregations, Aggregations]; | ||
| type NumericAggregationsTuple = [NumericAggregations, NumericAggregations]; | ||
|
|
||
| /** | ||
| * Resolves returned aggregations from network queries into single accumulated aggregation | ||
| * | ||
| * @param | ||
| */ | ||
| const resolveAggregations = ({ | ||
| aggregationsMap, | ||
| requestedAggregationFields, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No more requested fields, just aggregate what's there -- definitely simplifies things I'm just curious why there was a need to 'request specific fields' before and why that is no longer needed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is still a need to request specific fields, it just happens before the aggregation resolving. Example: Before code change if we are iterating on original query,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it so it's all just stream lined handling of the same scenario, just wanted to make sure
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Other relevant comment for reference too #898 (comment) For sure - thanks for checking! |
||
| accumulator, | ||
| }: ResolveAggregationInput) => { | ||
| requestedAggregationFields.forEach((fieldName) => { | ||
| const resolveAggregations = ({ aggregationsMap, accumulator }: ResolveAggregationInput) => { | ||
| Object.keys(aggregationsMap).forEach((fieldName) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iterate over response instead of request. possibility of undefined fields when using response object |
||
| const aggregation = aggregationsMap[fieldName]; | ||
| const aggregationType = aggregation?.__typename || ''; | ||
| const accumulatedFieldAggregation = accumulator[fieldName]; | ||
|
|
||
| if (aggregation && accumulatedFieldAggregation) { | ||
| const resolvedAggregation = resolveToNetworkAggregation(aggregationType, [ | ||
| aggregation, | ||
| accumulatedFieldAggregation, | ||
| ]); | ||
| const accumulatedFieldAggregation = accumulator[fieldName]; | ||
|
|
||
| // mutation - update a single aggregations field in the accumulator | ||
| accumulator[fieldName] = resolvedAggregation; | ||
| } | ||
| // mutation - update a single aggregations field in the accumulator | ||
| // if first aggregation, nothing to resolve yet | ||
| accumulator[fieldName] = !accumulatedFieldAggregation | ||
| ? aggregation | ||
| : resolveToNetworkAggregation(aggregationType, [aggregation, accumulatedFieldAggregation]); | ||
| }); | ||
|
Comment on lines
+27
to
30
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clean up resolution calling. if there's no existing aggregation, nothing to resolve with |
||
|
|
||
| return accumulator; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -46,12 +38,12 @@ const resolveAggregations = ({ | |
| */ | ||
| const resolveToNetworkAggregation = ( | ||
| type: string, | ||
| aggregations: Aggregations[] | NumericAggregations[], | ||
| aggregations: AggregationsTuple | NumericAggregationsTuple, | ||
| ): Aggregations | NumericAggregations => { | ||
| if (type === SUPPORTED_AGGREGATIONS.Aggregations) { | ||
| return resolveAggregation(aggregations as Aggregations[]); | ||
| return resolveAggregation(aggregations as AggregationsTuple); | ||
| } else if (type === SUPPORTED_AGGREGATIONS.NumericAggregations) { | ||
| return resolveNumericAggregation(aggregations as NumericAggregations); | ||
| return resolveNumericAggregation(aggregations as NumericAggregationsTuple); | ||
| } else { | ||
| // no types match | ||
| throw Error('No matching aggregation type'); | ||
|
|
@@ -153,7 +145,7 @@ const updateComputedBuckets = (bucket: Bucket, computedBuckets: Bucket[]) => { | |
| * } | ||
| * ``` | ||
| */ | ||
| export const resolveAggregation = (aggregations: Aggregations[]): Aggregations => { | ||
| export const resolveAggregation = (aggregations: AggregationsTuple): Aggregations => { | ||
| const resolvedAggregation = aggregations.reduce((resolvedAggregation, agg) => { | ||
| const computedBuckets = resolvedAggregation.buckets; | ||
| agg.buckets.forEach((bucket) => updateComputedBuckets(bucket, computedBuckets)); | ||
|
|
@@ -163,34 +155,34 @@ export const resolveAggregation = (aggregations: Aggregations[]): Aggregations = | |
| return resolvedAggregation; | ||
| }; | ||
|
|
||
| const resolveNumericAggregation = (aggregations: NumericAggregations) => { | ||
| // TODO: implement | ||
| throw Error('Not implemented'); | ||
| }; | ||
| const resolveNumericAggregation = (aggregations: NumericAggregationsTuple): NumericAggregations => { | ||
| return aggregations.reduce((resolvedAggregation, agg) => { | ||
| // max | ||
| if (agg.stats.max > resolvedAggregation.stats.max) { | ||
| resolvedAggregation.stats.max = agg.stats.max; | ||
| } | ||
|
Comment on lines
+160
to
+163
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolvedAggregation.stats.max = Math.max(agg.stats.max, resolvedAggregation.stats.max);
resolvedAggregation.stats.min = Math.min(agg.stats.min, resolvedAggregation.stats.min); |
||
| // min | ||
| if (agg.stats.min < resolvedAggregation.stats.min) { | ||
| resolvedAggregation.stats.min = agg.stats.min; | ||
| } | ||
| // count | ||
| resolvedAggregation.stats.count += agg.stats.count; | ||
| // sum | ||
| resolvedAggregation.stats.sum += agg.stats.sum; | ||
| // avg | ||
| resolvedAggregation.stats.avg = resolvedAggregation.stats.sum / resolvedAggregation.stats.count; | ||
|
|
||
|
Comment on lines
+160
to
174
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const emptyAggregation: Aggregations = { bucket_count: 0, buckets: [] }; | ||
| return resolvedAggregation; | ||
| }); | ||
| }; | ||
|
|
||
| export class AggregationAccumulator { | ||
| requestedFields: string[]; | ||
| totalAgg: AllAggregations; | ||
|
|
||
| constructor(requestedFieldsMap: RequestedFieldsMap) { | ||
| const requestedFields = Object.keys(requestedFieldsMap); | ||
| this.requestedFields = requestedFields; | ||
| /* | ||
| * seed accumulator with the requested field keys | ||
| * this will make it easier to add to using key lookup instead of Array.find | ||
| */ | ||
| this.totalAgg = requestedFields.reduce<AllAggregations>((accumulator, field) => { | ||
| return { ...accumulator, [field]: emptyAggregation }; | ||
| }, {}); | ||
| } | ||
|
Comment on lines
-178
to
-187
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed anymore, saves an iteration by checking if aggregation exists while resolving |
||
| totalAgg: AllAggregations = {}; | ||
|
|
||
| resolve(data: AllAggregations) { | ||
| resolveAggregations({ | ||
| accumulator: this.totalAgg, | ||
| aggregationsMap: data, | ||
| requestedAggregationFields: this.requestedFields, | ||
| aggregationsMap: structuredClone(data), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ensure no input data is mutated. shouldn't be an issue due to life time of a response data in real time application but definitely causes issues with test fixtures.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL! |
||
| }); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to tuple instead of loose array. resolving logic takes previous aggregation and current aggregation and returns a single aggregation