Enable include/exclude in Terms agg for numeric fields

[DianaDerevyankina]

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:

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

2. Exclude field with some data entered.

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

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

5. 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:

1. isNumberType function is used to check if the agg type is number.
2. 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.

[elasticmachine]

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

[DianaDerevyankina]

@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]

@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]

@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]

@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.

[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).

[timroes]

@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?

[maryia-lapata]

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]

🎨 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]

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]

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]

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

[timroes]

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

[timroes]

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.

[timroes]

@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!

[lukeelmers]

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

[kibanamachine]

💚 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