Enable include/exclude in Terms agg for numeric fields by DianaDerevyankina · Pull Request #59425 · elastic/kibana

DianaDerevyankina · 2020-03-05T14:21:46Z

Closes #4576

Summary

Refactored type function to accept multiple type arguments, so it allows declaring other functions based on this one. For example, isStringOrNumberType accepts both string and number types.
Added a possibility to write an array of numbers in request params.
Added a new Include/Exclude control that can accept either string or multiple numbers value.

Screenshots:

The initial state of the component. Selected terms aggregation, string field type is chosen. The same as before.

Exclude field with some data entered.

Selected terms aggregation, number field type is chosen. Between switching of string and number field type values are kept in fields.

Empty fields are valid, they're not written in request

Fields with invalid values are marked in red, but the "Update" button is still available, as these values aren't written in request.

Dev-Docs

Added a new component `IncludeExcludeParamEditor` that returns string or number list controls depending on the type of `agg`.

isType function in migrate_include_exclude_format.ts now can accept an array of string types that allows checking if a value has a type of any of those passed in the array.
Based on this function, added 2 another:

isNumberType function is used to check if the agg type is number.
isStringOrNumberType function is used in shouldShow field of termsBucketAgg to check if the agg type is number or string.

IncludeExcludeParamEditor has props type of AggParamEditorProps<string | Array<number | undefined>> and returns either EuiFormRow with multiple NumberRows or StringParamEditor depending of isNumberType(props.agg) result.

Closes elastic#4576

elasticmachine · 2020-03-05T14:21:49Z

Pinging @elastic/kibana-app (Team:KibanaApp)

DianaDerevyankina · 2020-03-06T08:20:05Z

@elasticmachine merge upstream

maryia-lapata

LGTM, tested locally in Chrome (Mac).

timroes

Unfortunately that PR does not quiet work yet. The include/exclude fields are shown for numeric types now, but they actually don't work, because their values are never written to the request for numerical types.

This is prevented by the check in migrate_include_exclude_format.ts line 50, which actually only writes them to the output if the field is of type string. Also we cannot simply write out a the string as it is for numerical fields, since Elasticsearch does not accept a regex for numeric fields, but needs an array of numeric values. I'd suggest we just take the value inside the field, and split it by comma into an array, so the function would look like the following:

const isNumberType = isType('number');
// ....
if (isObject(value)) {
  output.params[this.name] = value.pattern;
} else if (value && isStringType(aggConfig)) {
  output.params[this.name] = value;
} else if (value.length > 0 && isNumberType(aggConfig)) {
  output.params[this.name] = value
    .split(',')
    .map(val => Number(val.trim()))
    .filter(val => !Number.isNaN(val));
}

We should also add a couple of tests in terms.test.ts to make sure that the write method works correctly for numeric fields.

maryia-lapata · 2020-03-06T15:04:14Z

@timroes can include/exclude value for number type field be a decimal numeral? If yes, the decimal separator can be a point or a comma depending on the locale. Maybe a space can be a delimiter for number.
In any case, I think we should add a description to the include/exclude field, so that a user can understand how to apply several number values.

timroes · 2020-03-06T16:16:09Z

@maryia-lapata It could potentially be a decimal number, but it would always need to be in the english format, since that is what the ES API takes. But I would also be fine if we're using e.g. semicolon as a separator if that feels better.

To be honest in the long term I think the field should actually render as a numerical list (like we use in range aggregations, where you can add and remove input boxes and each just takes one number), but this would require way more changes (basically include/exclude need their own control separate from the string control, which I am not sure if it's worth right now, given that I haven't heard that request very often that people need include/exclude on numerical fields.

In any case, I think we should add a description to the include/exclude field, so that a user can understand how to apply several number values.

I totally agree with you. I think for doing that we already need our own editor control for include/exclude (since we cannot attach a tooltip to a generic string component, and also the AggType should not hold a tooltip text for a parameter, since we're trying to split AggConfigs from the editor concerns). If we anyway need to create an include/exclude component, I'll leave the decision to you, how we'd should build that (going the full UX and make it a numeric list, in case it's a number field, or just with a better tooltip).

maryia-lapata · 2020-03-09T12:25:46Z

@timroes I think since we're touching this Exclude/Include field, it's worth doing it in way it should be (with separate numerical list for number field type), which will be clear for a user.

# Conflicts: # src/legacy/core_plugins/data/public/search/aggs/buckets/terms.ts

maryia-lapata

Great work!

In general LGTM, but it'd be nice to fix the comments below.
I tested on Chrome (Mac) the cases of switching field type, discarding, empty and invalid values (they are not in the request).

src/plugins/vis_default_editor/public/components/controls/include_exclude.tsx

src/plugins/vis_default_editor/public/components/controls/components/simple_number_list.tsx

src/plugins/vis_default_editor/public/components/controls/include_exclude.tsx

# Conflicts: # src/plugins/data/public/public.api.md

timroes · 2020-04-10T10:22:37Z

src/plugins/vis_default_editor/public/components/controls/components/simple_number_list.tsx

+
+const generateId = htmlIdGenerator();
+
+function SimpleNumberList({


@maryia-lapata @DianaDerevyankina Could you please explain shortly what's the different between SimpleNumberList and NumberList, we use in other places in the editor (like percentiles)? Is the difference really large enough that it justifies having a separate component, or were there just minor things, that we could potentially solve adding a property to NumberList?

The difference isn't big:

NumberList creates next value as incremented the last one, SimpleNumberList creates just empty input;

NumberList saves an empty value as undefined to value, SimpleNumberList - as an empty string;

NumberList can influence on the form validity and it uses useValidation hook as well as it validates each input, SimpleNumberList - not;

there is no initial value in SimpleNumberList.

In case we decide to reuse NumberList, it's needed to make it more generic. I think the component is already a bit complex (I wouldn't want to make it a magic component), and I'd leave a separate SimpleNumberList component.

timroes · 2020-04-10T10:27:49Z

src/plugins/data/public/search/aggs/buckets/migrate_include_exclude_format.ts

 */

-import { isString, isObject } from 'lodash';
+import { isString, isObject, isArray } from 'lodash';


🎨 We're trying to not introduce new lodash code if there is an easy vanilla JS equivalent. So I would recommend using Array.isArray instead of using a lodash function.

timroes · 2020-04-10T11:51:16Z

src/plugins/data/public/search/aggs/buckets/migrate_include_exclude_format.ts


-    if (isObject(value)) {
+    if (isArray(value) && value.length > 0 && isNumberType(aggConfig)) {
+      const parsedValue = (value as number[]).filter((val: number) => val || val === 0);


Suggested change

const parsedValue = (value as number[]).filter((val: number) => val || val === 0);

const parsedValue = value.filter((val): val is number => Number.isFinite(val));

This way we don't need to have a cast (as) in here, which always contains the danger of breaking in the future, since we "disabled" the type system in this place.

Using a custom typeguard val is number will tell typescript if that boolean returning function returns true, it is a number, otherwise not. That will lead to the same effect of parsedValue being actually a number[] in the end.

Also I would shorten the call for val || val === 0 to Number.isFinite since it's actually a bit safe, since it will only let numbers through, that are not NaN or Infinite, which is kind of similar to what you wanted to express here (except that val || val === 0 would potentially also let any string through).

timroes · 2020-04-10T11:56:07Z

src/plugins/vis_default_editor/public/components/controls/include_exclude.tsx

+      const numberArray = value
+        .split(',')
+        .map(item => parseFloat(item))
+        .filter(number => !isNaN(number));


I would use Number.isFinite here too, since we also wouldn't want Infinity to pass this check. Currently if you had Infinity in your string include/exclude it would show up as an empty input box after selecting a number field.

timroes · 2020-04-10T11:59:54Z

src/plugins/vis_default_editor/public/components/controls/include_exclude.tsx

+        .filter(number => !isNaN(number));
+      setValue(numberArray.length ? numberArray : ['']);
+    } else if (!isAggOfNumberType && isArray(value) && value !== undefined) {
+      setValue((value as Array<number | ''>).filter(item => item !== '').toString());


The case should not be needed here, TypeScript should know that value can only be Array<number | ''> due to the check in the if.

Also toString() -> join('|') to align with the comment above.

timroes · 2020-04-10T12:00:27Z

src/plugins/vis_default_editor/public/components/controls/include_exclude.tsx

+  useEffect(() => {
+    if (isAggOfNumberType && !isArray(value) && value !== undefined) {
+      const numberArray = value
+        .split(',')


I think we want to use | for splitting here, since in the string version it was an regex and so a separator between different options would have had to be an | and a comma would just be part of the string matched.

…nged symbol of string join and array split

timroes · 2020-04-10T14:38:27Z

@elasticmachine merge upstream

timroes

Tested on Chrome Linux, seems to work and being a really nice experience for numerical fields (way better then just one text field). Code LGTM

lukeelmers

Overall this looks great & is a nice addition! Tested locally on Chrome (macOS), and everything is working as I'd expect.

Reviewing the code, I primarily focused on the changes in data.search.aggs. No major concerns, although I would like to +1 Tim's earlier suggestion to add a few unit tests to terms.test.ts, just to make sure we don't forget this change in the future.

Aside from that, everything LGTM!

…ite method

…to 4576

lukeelmers

Thanks for adding those tests! This LGTM once a green build comes back.

# Conflicts: # src/plugins/data/public/public.api.md

kibanamachine · 2020-04-20T19:01:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 88c7fdb

History

💔 Build #41587 failed 14eda87
💚 Build #40153 succeeded 847e4c3
💔 Build #40124 failed c827bd8
💚 Build #39948 succeeded a774fd2
💚 Build #39664 succeeded 42cac76

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

* master: (30 commits) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) [TSVB] Use default Kibana palette for split series (elastic#62241) Ingest overview page (elastic#63214) ...

* Enable include/exclude in Terms agg for numeric fields Closes elastic#4576 * Added a new component that allows adding multiple values * Added some validation to include/exclude fields * Removed unnecessary comments and accepted API changes * Fixed i18n ID issue * Refactored some code and fixed discard button issue * Added SimpleNumberList component and value parsing in include_exclude.tsx * Fixed merge conflict * Fixed merge conflict * Refactored some code * Got rid of lodash isArray, added Number.isFinite where needed and changed symbol of string join and array split * Added some more test cases to cover migrate_include_exclude_format write method Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…t-node-pipelines * 'master' of github.com:elastic/kibana: (32 commits) Move authz lib out of snapshot restore (#63947) Migrate vis_type_table to kibana/new platform (#63105) Enable include/exclude in Terms agg for numeric fields (#59425) follow conventions for saved object definitions (#63571) [Docs]Adds saved object key setting to load balancing kib instances (#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (#63838) Fix page layouts, clean up unused code (#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (#63859) [Maps] Do not fetch geo_shape from docvalues (#63997) Vega doc changes (#63889) [Metrics UI] Reorganize file layout for Metrics UI (#60049) Add sub-menus to Resolver node (for 75% zoom) (#63476) [FTR] Add test suite metrics tracking/output (#62515) [ML] Fixing single metric viewer page padding (#63839) [Discover] Allow user to generate a report after saving a modified saved search (#63623) [Reporting] Config flag to escape formula CSV values (#63645) [Metrics UI] Remove remaining field filtering (#63398) [Maps] fix date labels (#63909) [TSVB] Use default Kibana palette for split series (#62241) ...

…bana into ingest-node-pipelines/privileges * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits) Move authz lib out of snapshot restore (elastic#63947) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [Ingest pipelines] Delete pipeline (elastic#63635) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) ... # Conflicts: # x-pack/legacy/plugins/uptime/public/components/monitor/ml/index.ts # x-pack/legacy/plugins/uptime/public/components/overview/index.ts # x-pack/plugins/ingest_pipelines/public/application/index.tsx # x-pack/plugins/ingest_pipelines/server/routes/api/index.ts # x-pack/plugins/ingest_pipelines/server/routes/index.ts

* Enable include/exclude in Terms agg for numeric fields Closes #4576 * Added a new component that allows adding multiple values * Added some validation to include/exclude fields * Removed unnecessary comments and accepted API changes * Fixed i18n ID issue * Refactored some code and fixed discard button issue * Added SimpleNumberList component and value parsing in include_exclude.tsx * Fixed merge conflict * Fixed merge conflict * Refactored some code * Got rid of lodash isArray, added Number.isFinite where needed and changed symbol of string join and array split * Added some more test cases to cover migrate_include_exclude_format write method Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits) [Ingest Node Pipelines] Clone Pipeline (elastic#64049) Move authz lib out of snapshot restore (elastic#63947) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [Ingest pipelines] Delete pipeline (elastic#63635) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) ...

Enable include/exclude in Terms agg for numeric fields

4abf48a

Closes elastic#4576

DianaDerevyankina added release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:Vis Editor Visualization editor issues labels Mar 5, 2020

DianaDerevyankina requested review from alexwizp and maryia-lapata March 5, 2020 14:21

DianaDerevyankina self-assigned this Mar 5, 2020

Merge branch 'master' into 4576

1ecb7b2

maryia-lapata added v7.7.0 v8.0.0 labels Mar 6, 2020

DianaDerevyankina marked this pull request as ready for review March 6, 2020 10:08

DianaDerevyankina requested a review from a team as a code owner March 6, 2020 10:08

maryia-lapata approved these changes Mar 6, 2020

View reviewed changes

timroes suggested changes Mar 6, 2020

View reviewed changes

lizozom requested a review from lukeelmers March 8, 2020 14:36

DianaDerevyankina added 9 commits March 10, 2020 13:32

Merge branch 'DziyanaDzeraviankina' into 4576

69afa3f

Merge branch 'DziyanaDzeraviankina' into 4576

0d81475

Merge branch 'DziyanaDzeraviankina' into 4576

28869ba

# Conflicts: # src/legacy/core_plugins/data/public/search/aggs/buckets/terms.ts

Merge branch 'DziyanaDzeraviankina' into 4576

8a84f85

Added a new component that allows adding multiple values

dbc10d4

Merge branch 'DziyanaDzeraviankina' into 4576

825412e

Merge branch 'DziyanaDzeraviankina' into 4576

acef9bd

Merge branch 'DziyanaDzeraviankina' into 4576

d759810

Added some validation to include/exclude fields

d53c9dd

DianaDerevyankina removed the v7.7.0 label Apr 2, 2020

Fixed merge conflict

42cac76

maryia-lapata reviewed Apr 9, 2020

View reviewed changes

DianaDerevyankina added 2 commits April 9, 2020 19:54

Merge branch 'DziyanaDzeraviankina' into 4576

3bd31b9

# Conflicts: # src/plugins/data/public/public.api.md

Refactored some code

a774fd2

timroes reviewed Apr 10, 2020

View reviewed changes

timroes suggested changes Apr 10, 2020

View reviewed changes

Got rid of lodash isArray, added Number.isFinite where needed and cha…

c827bd8

…nged symbol of string join and array split

Merge branch 'master' into 4576

847e4c3

timroes approved these changes Apr 10, 2020

View reviewed changes

timroes removed the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Apr 10, 2020

lukeelmers reviewed Apr 14, 2020

View reviewed changes

DianaDerevyankina added 4 commits April 14, 2020 12:34

Merge branch 'DianaDerevyankina' into 4576

703025d

Merge branch 'DianaDerevyankina' into 4576

52017cb

Added some more test cases to cover migrate_include_exclude_format wr…

83320f3

…ite method

Merge branch '4576' of https://github.com/DianaDerevyankina/kibana in…

14eda87

…to 4576

lukeelmers approved these changes Apr 20, 2020

View reviewed changes

Merge branch 'DianaDerevyankina' into 4576

88c7fdb

# Conflicts: # src/plugins/data/public/public.api.md

alexwizp approved these changes Apr 20, 2020

View reviewed changes

DianaDerevyankina merged commit 46f6f87 into elastic:master Apr 21, 2020

DianaDerevyankina mentioned this pull request Apr 21, 2020

[7.x] Enable include/exclude in Terms agg for numeric fields (#59425) #64044

Merged

timroes mentioned this pull request May 5, 2020

Exclude filter in aggregations only supports syntax for strings #12421

Closed


		const generateId = htmlIdGenerator();

		function SimpleNumberList({

	const parsedValue = (value as number[]).filter((val: number) => val \|\| val === 0);
	const parsedValue = value.filter((val): val is number => Number.isFinite(val));

Conversation

DianaDerevyankina commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dev-Docs

Added a new component IncludeExcludeParamEditor that returns string or number list controls depending on the type of agg.

Uh oh!

elasticmachine commented Mar 5, 2020

Uh oh!

DianaDerevyankina commented Mar 6, 2020

Uh oh!

maryia-lapata left a comment

Choose a reason for hiding this comment

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

maryia-lapata commented Mar 6, 2020

Uh oh!

timroes commented Mar 6, 2020

Uh oh!

maryia-lapata commented Mar 9, 2020

Uh oh!

maryia-lapata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timroes commented Apr 10, 2020

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 20, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DianaDerevyankina commented Mar 5, 2020 •

edited

Loading

Added a new component `IncludeExcludeParamEditor` that returns string or number list controls depending on the type of `agg`.