[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics #169834

mbondyra · 2023-10-25T14:11:32Z

Summary

When checking my PR here #169258 @stratoula noticed that the column.meta.type is not set properly for last value aggregation (it always defaults to 'number', same with all the filtered metrics too!). When I dug deeper, I noticed that happens because we calculate it as:

   type:
              column.aggConfig.type.valueType
              column.aggConfig.params.field?.type ||
              'number',

and some of the AggConfigs don't have the static valueType property nor field, specifically the one with nested aggregations, like top_hits, top_metrics and filtered_metric. instead of a static valueType, I've decided to change it to a method getValueType where we can pass AggConfigs and get the type from different places internally. This way top_hits, top_metrics and filtered_metric get the type of the field from customMetric.
I also changed the values for min and max aggregation - they were set on number, but they can also be a date.

elasticmachine · 2023-10-25T16:02:04Z

Pinging @elastic/kibana-visualizations (Team:Visualizations)

mbondyra · 2023-10-25T20:18:54Z

x-pack/plugins/lens/common/expressions/datatable/utils.ts

@@ -16,8 +16,7 @@ export function isNumericFieldForDatatable(currentData: Datatable | undefined, a
  const column = currentData?.columns.find(
    (col) => col.id === accessor || getOriginalId(col.id) === accessor
  );
-  // min and max aggs are reporting as number but are actually dates - work around this by checking for the date formatter until this is fixed at the source


now that the min and max aggs were fixed, I think we can safely remove it.

I think we could also remove the code in line 24, but I am leaving it just in case if there's something I miss!

mbondyra · 2023-10-25T20:25:17Z

x-pack/plugins/lens/public/visualizations/datatable/components/table_basic.tsx

-  const isNumericMap: Record<string, boolean> = useMemo(() => {
-    const numericMap: Record<string, boolean> = {};
-    for (const column of firstLocalTable.columns) {
-      // filtered metrics result as "number" type, but have no field


now when the bug on aggs is fixed, filtered metrics will always have the value of the nested metric so we don't have to do the check here. I tested it with creating a datatable with a few metrics and seeing if they align properly:

I see that a test is failing but I think is fine, we are now actually fixing the last value alignmentws

mbondyra · 2023-10-26T07:46:22Z

x-pack/plugins/lens/public/visualizations/datatable/components/table_basic.test.tsx

@@ -545,53 +545,6 @@ describe('DatatableComponent', () => {
    });
  });

-  test('it detect last_value filtered metric type', () => {


🎉 This is test is not needed anymore as it was added to guard against the buggy response from aggs that we were getting. Now, as we get a proper response (number type is always number, string is always a string) we don't have to test this.

dej611

Tested locally and could not find issues with changes. LGTM 👍

stratoula

I don't find any bugs, on the contrary I think that we are improving our code a lot and we are possibly fixing bugs we don't know they exist :) Thanx Marta for working on this!

stratoula · 2023-10-26T07:52:52Z

src/plugins/data/common/search/aggs/param_types/base.ts

@@ -28,8 +28,7 @@ export class BaseParamType<TAggConfig extends IAggConfig = IAggConfig> {
  deserialize: (value: any, aggConfig?: TAggConfig) => any;
  toExpressionAst?: (value: any) => ExpressionAstExpression[] | ExpressionAstExpression | undefined;
  options: any[];
-  valueType?: any;
-
+  getValueType: (aggConfig: IAggConfig) => any;


I wonder is instead of any we give the DatatableColumnType will we have TS failures?

This breaks because of the weird line I don't understand but without it the expression throws an error:
this.getValueType = () => AggConfig; in file: src/plugins/data/common/search/aggs/param_types/agg.ts

Not sure how we could fix it, I'll talk to Peter about it once he's back to understand things better.

Yes with so many anys it makes sense! Thanx for looking into it!

stratoula · 2023-10-26T08:21:48Z

x-pack/plugins/lens/public/visualizations/datatable/components/table_basic.tsx

-  const isNumericMap: Record<string, boolean> = useMemo(() => {
-    const numericMap: Record<string, boolean> = {};
-    for (const column of firstLocalTable.columns) {
-      // filtered metrics result as "number" type, but have no field


I see that a test is failing but I think is fine, we are now actually fixing the last value alignmentws

ppisljar

lgtm

kibana-ci · 2023-10-26T14:10:01Z

💚 Build Succeeded

Buildkite Build
Commit: 8f71729

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	2537	2539	+2

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`data`	33	32	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.4MB	1.4MB	-581.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	409.5KB	410.1KB	+532.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3186	3188	+2

History

💔 Build #171138 failed d360fc3
💔 Build #171081 failed b3c18a9
💔 Build #171003 failed cb35516
💛 Build #170968 was flaky b7e644b
💔 Build #170911 failed bd81585

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stratoula

…metrics (elastic#169834) ## Summary When checking my PR here elastic#169258 @stratoula noticed that the `column.meta.type` is not set properly for last value aggregation (it always defaults to 'number', same with all the filtered metrics too!). When I dug deeper, I noticed that happens because we calculate it as: ``` type: column.aggConfig.type.valueType column.aggConfig.params.field?.type || 'number', ``` and some of the `AggConfigs` don't have the static `valueType` property nor field, specifically the one with nested aggregations, like top_hits, top_metrics and filtered_metric. instead of a static `valueType`, I've decided to change it to a method `getValueType` where we can pass AggConfigs and get the type from different places internally. This way `top_hits`, `top_metrics` and `filtered_metric` get the type of the field from `customMetric`. I also changed the values for `min` and `max` aggregation - they were set on `number`, but they can also be a `date`.

@stratoula

…metrics (elastic#169834) ## Summary When checking my PR here elastic#169258 @stratoula noticed that the `column.meta.type` is not set properly for last value aggregation (it always defaults to 'number', same with all the filtered metrics too!). When I dug deeper, I noticed that happens because we calculate it as: ``` type: column.aggConfig.type.valueType column.aggConfig.params.field?.type || 'number', ``` and some of the `AggConfigs` don't have the static `valueType` property nor field, specifically the one with nested aggregations, like top_hits, top_metrics and filtered_metric. instead of a static `valueType`, I've decided to change it to a method `getValueType` where we can pass AggConfigs and get the type from different places internally. This way `top_hits`, `top_metrics` and `filtered_metric` get the type of the field from `customMetric`. I also changed the values for `min` and `max` aggregation - they were set on `number`, but they can also be a `date`.

mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v8.12.0 labels Oct 25, 2023

mbondyra force-pushed the aggs/pass_correct_valueType branch from 93fc94d to 74c0d21 Compare October 25, 2023 14:13

mbondyra marked this pull request as ready for review October 25, 2023 16:02

mbondyra requested review from a team as code owners October 25, 2023 16:02

mbondyra force-pushed the aggs/pass_correct_valueType branch from b7e644b to b502baf Compare October 25, 2023 20:17

mbondyra commented Oct 25, 2023

View reviewed changes

mbondyra force-pushed the aggs/pass_correct_valueType branch from b502baf to cb35516 Compare October 25, 2023 20:23

mbondyra commented Oct 25, 2023

View reviewed changes

mbondyra commented Oct 26, 2023

View reviewed changes

dej611 approved these changes Oct 26, 2023

View reviewed changes

stratoula approved these changes Oct 26, 2023

View reviewed changes

ppisljar approved these changes Oct 26, 2023

View reviewed changes

mbondyra changed the title ~~[Aggs] Fix valueType for top hit, top metric and all filtered metrics~~ [Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics Oct 26, 2023

mbondyra added 6 commits October 26, 2023 14:43

[aggs] fix valueType for top hit, top metric and filtered metrics

c37db53

tests

a19a68b

simplify lens cases

0c95ef0

slight refactor

d1840bc

remove the test that is not needed anymore

c93d3f8

remove unnecessary check

8f71729

mbondyra force-pushed the aggs/pass_correct_valueType branch from d360fc3 to 8f71729 Compare October 26, 2023 12:43

mbondyra merged commit cadbaee into elastic:main Oct 26, 2023
29 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Oct 26, 2023

mbondyra deleted the aggs/pass_correct_valueType branch October 26, 2023 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics #169834

[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics #169834

mbondyra commented Oct 25, 2023 •

edited

elasticmachine commented Oct 25, 2023

mbondyra Oct 25, 2023

mbondyra Oct 25, 2023

mbondyra Oct 25, 2023

stratoula Oct 26, 2023

mbondyra Oct 26, 2023 •

edited

dej611 left a comment

stratoula left a comment

stratoula Oct 26, 2023

mbondyra Oct 26, 2023 •

edited

stratoula Oct 26, 2023

stratoula Oct 26, 2023

ppisljar left a comment

kibana-ci commented Oct 26, 2023

API count

[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics #169834

[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics #169834

Conversation

mbondyra commented Oct 25, 2023 • edited

Summary

elasticmachine commented Oct 25, 2023

mbondyra Oct 25, 2023

Choose a reason for hiding this comment

mbondyra Oct 25, 2023

Choose a reason for hiding this comment

mbondyra Oct 25, 2023

Choose a reason for hiding this comment

stratoula Oct 26, 2023

Choose a reason for hiding this comment

mbondyra Oct 26, 2023 • edited

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

stratoula Oct 26, 2023

Choose a reason for hiding this comment

mbondyra Oct 26, 2023 • edited

Choose a reason for hiding this comment

stratoula Oct 26, 2023

Choose a reason for hiding this comment

stratoula Oct 26, 2023

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

kibana-ci commented Oct 26, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Any counts in public APIs

Async chunks

Page load bundle

API count

History

mbondyra commented Oct 25, 2023 •

edited

mbondyra Oct 26, 2023 •

edited

mbondyra Oct 26, 2023 •

edited