[ML] Transforms: Improve percentiles agg validation (#197816)

## Summary

Resolves https://github.com/elastic/kibana/issues/138874

The Transforms percentiles aggregation now uses a ComboBox to define
percentiles.


https://github.com/user-attachments/assets/f30de65e-3555-4643-963b-821877e7b166

a few more validation cases:


https://github.com/user-attachments/assets/79339c4e-36d2-465c-bc93-c47e1f442a87



### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
This commit is contained in:
Robert Jaszczurek 2024-11-21 14:17:00 +01:00 committed by GitHub
parent d3b9a9a050
commit 716375b622
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 335 additions and 55 deletions

View file

@ -100,7 +100,7 @@ describe('getAggConfigFromEsAgg', () => {
field: 'products.base_price',
parentAgg: result,
aggConfig: {
percents: '1,5,25,50,75,95,99',
percents: [1, 5, 25, 50, 75, 95, 99],
},
});

View file

@ -225,6 +225,7 @@ export interface PivotAggsConfigWithExtra<T, ESConfig extends { [key: string]: a
onChange: (arg: Partial<T>) => void;
selectedField: string;
isValid?: boolean;
errorMessages?: string[];
}>;
/** Aggregation specific configuration */
aggConfig: Partial<T>;
@ -238,6 +239,8 @@ export interface PivotAggsConfigWithExtra<T, ESConfig extends { [key: string]: a
getAggName?: () => string | undefined;
/** Helper text for the aggregation reflecting some configuration info */
helperText?: () => string | undefined;
/** Returns validation error messages */
getErrorMessages?: () => string[] | undefined;
}
interface PivotAggsConfigPercentiles extends PivotAggsConfigWithUiBase {

View file

@ -242,6 +242,7 @@ export const PopoverForm: React.FC<Props> = ({ defaultData, otherAggNames, onCha
});
}}
isValid={aggConfigDef.isValid()}
errorMessages={aggConfigDef.getErrorMessages?.()}
/>
) : null}
{isUnsupportedAgg && (

View file

@ -82,7 +82,7 @@ describe('Transform: Define Pivot Common', () => {
aggName: 'the-field.percentiles',
dropDownName: 'percentiles( the-f[i]e>ld )',
AggFormComponent: PercentilesAggForm,
aggConfig: { percents: '1,5,25,50,75,95,99' },
aggConfig: { percents: [1, 5, 25, 50, 75, 95, 99] },
},
'filter( the-f[i]e>ld )': {
agg: 'filter',
@ -222,7 +222,7 @@ describe('Transform: Define Pivot Common', () => {
dropDownName: 'percentiles( the-f[i]e>ld )',
field: ' the-f[i]e>ld ',
AggFormComponent: PercentilesAggForm,
aggConfig: { percents: '1,5,25,50,75,95,99' },
aggConfig: { percents: [1, 5, 25, 50, 75, 95, 99] },
},
'sum( the-f[i]e>ld )': {
agg: 'sum',
@ -292,7 +292,7 @@ describe('Transform: Define Pivot Common', () => {
dropDownName: 'percentiles(rt_bytes_bigger)',
field: 'rt_bytes_bigger',
AggFormComponent: PercentilesAggForm,
aggConfig: { percents: '1,5,25,50,75,95,99' },
aggConfig: { percents: [1, 5, 25, 50, 75, 95, 99] },
},
'sum(rt_bytes_bigger)': {
agg: 'sum',

View file

@ -0,0 +1,146 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { getPercentilesAggConfig } from './config';
import type { IPivotAggsConfigPercentiles } from './types';
import { PERCENTILES_AGG_DEFAULT_PERCENTS } from '../../../../../../common';
describe('percentiles agg config', () => {
let config: IPivotAggsConfigPercentiles;
beforeEach(() => {
config = getPercentilesAggConfig({
agg: 'percentiles',
aggName: 'test-agg',
field: ['test-field'],
dropDownName: 'test-agg',
});
});
describe('#setUiConfigFromEs', () => {
test('sets field and percents from ES config', () => {
// act
config.setUiConfigFromEs({
field: 'test-field',
percents: [10, 20, 30],
});
// assert
expect(config.field).toEqual('test-field');
expect(config.aggConfig).toEqual({ percents: [10, 20, 30] });
});
});
describe('#getEsAggConfig', () => {
test('returns null for invalid config', () => {
// arrange
config.aggConfig.percents = [150]; // invalid percentile value
// act and assert
expect(config.getEsAggConfig()).toBeNull();
});
test('returns valid config', () => {
// arrange
config.field = 'test-field';
config.aggConfig.percents = [10, 20, 30];
// act and assert
expect(config.getEsAggConfig()).toEqual({
field: 'test-field',
percents: [10, 20, 30],
});
});
test('returns default percents if none specified', () => {
// arrange
config.field = 'test-field';
// act and assert
expect(config.getEsAggConfig()).toEqual({
field: 'test-field',
percents: PERCENTILES_AGG_DEFAULT_PERCENTS,
});
});
});
describe('#isValid', () => {
test('returns false for percentiles out of range', () => {
// arrange
config.aggConfig.percents = [150];
// act and assert
expect(config.isValid()).toBeFalsy();
expect(config.aggConfig.errors).toContain('PERCENTILE_OUT_OF_RANGE');
});
test('returns false for invalid number format', () => {
// arrrange
config.aggConfig.pendingPercentileInput = 'invalid';
// act and assert
expect(config.isValid()).toBeFalsy();
expect(config.aggConfig.errors).toContain('INVALID_FORMAT');
});
test('returns true for valid percents', () => {
// arrange
config.aggConfig.percents = [10, 20, 30];
// act and assert
expect(config.isValid()).toBeTruthy();
expect(config.aggConfig.errors).toBeUndefined();
});
test('validates pending input along with existing percents', () => {
// arrange
config.aggConfig.percents = [10, 20, 30];
config.aggConfig.pendingPercentileInput = '50';
// act and assert
expect(config.isValid()).toBeTruthy();
expect(config.aggConfig.errors).toBeUndefined();
});
});
describe('#getErrorMessages', () => {
test('returns undefined when there are no errors', () => {
// arrange
config.aggConfig.errors = undefined;
// act and assert
expect(config.getErrorMessages?.()).toBeUndefined();
});
test('returns undefined when errors array is empty', () => {
// arrange
config.aggConfig.errors = [];
// act and assert
expect(config.getErrorMessages?.()).toBeUndefined();
});
test('returns translated messages for single error', () => {
// arrange
config.aggConfig.errors = ['PERCENTILE_OUT_OF_RANGE'];
// act and assert
expect(config.getErrorMessages?.()).toEqual(['Percentiles must be between 0 and 100']);
});
test('returns translated messages for multiple errors', () => {
// arrange
config.aggConfig.errors = ['PERCENTILE_OUT_OF_RANGE', 'INVALID_FORMAT'];
// act and assert
expect(config.getErrorMessages?.()).toEqual([
'Percentiles must be between 0 and 100',
'Percentile must be a valid number',
]);
});
});
});

View file

@ -5,43 +5,60 @@
* 2.0.
*/
import { i18n } from '@kbn/i18n';
import { PercentilesAggForm } from './percentiles_form_component';
import type { IPivotAggsConfigPercentiles } from './types';
import type {
IPivotAggsConfigPercentiles,
PercentilesAggConfig,
ValidationResult,
ValidationResultErrorType,
} from './types';
import type { PivotAggsConfigBase } from '../../../../../../common';
import {
isPivotAggsConfigWithUiBase,
PERCENTILES_AGG_DEFAULT_PERCENTS,
} from '../../../../../../common';
import type { PivotAggsConfigWithUiBase } from '../../../../../../common/pivot_aggs';
import { MAX_PERCENTILE_PRECISION, MAX_PERCENTILE_VALUE, MIN_PERCENTILE_VALUE } from './constants';
/**
* TODO this callback has been moved.
* The logic of parsing the string should be improved.
*/
function parsePercentsInput(inputValue: string | undefined) {
if (inputValue !== undefined) {
const strVals: string[] = inputValue.split(',');
const percents: number[] = [];
for (const str of strVals) {
if (str.trim().length > 0 && isNaN(str as any) === false) {
const val = Number(str);
if (val >= 0 && val <= 100) {
percents.push(val);
} else {
return [];
}
}
function validatePercentsInput(config: Partial<PercentilesAggConfig>): ValidationResult {
const allValues = [...(config.percents ?? [])];
const errors: ValidationResultErrorType[] = [];
// Combine existing percents with pending input for validation
if (config.pendingPercentileInput) {
// Replace comma with dot before converting to number
const normalizedInput = config.pendingPercentileInput.replace(',', '.');
const pendingValue = Number(normalizedInput);
if (allValues.includes(pendingValue)) {
errors.push('DUPLICATE_VALUE');
}
return percents;
if (normalizedInput.replace('.', '').length > MAX_PERCENTILE_PRECISION) {
errors.push('NUMBER_TOO_PRECISE');
}
allValues.push(pendingValue);
}
return [];
}
if (allValues.length === 0) {
return {
isValid: false,
errors: [],
};
}
// Input string should only include comma separated numbers
function isValidPercentsInput(inputValue: string) {
return /^[0-9]+(,[0-9]+)*$/.test(inputValue);
if (allValues.some((value) => isNaN(value))) {
errors.push('INVALID_FORMAT');
}
if (allValues.some((value) => value < MIN_PERCENTILE_VALUE || value > MAX_PERCENTILE_VALUE)) {
errors.push('PERCENTILE_OUT_OF_RANGE');
}
return {
isValid: errors.length === 0,
errors: errors.length > 0 ? errors : undefined,
};
}
export function getPercentilesAggConfig(
@ -56,13 +73,13 @@ export function getPercentilesAggConfig(
AggFormComponent: PercentilesAggForm,
field,
aggConfig: {
percents: PERCENTILES_AGG_DEFAULT_PERCENTS.toString(),
percents: PERCENTILES_AGG_DEFAULT_PERCENTS,
},
setUiConfigFromEs(esAggDefinition) {
const { field: esField, percents } = esAggDefinition;
this.field = esField;
this.aggConfig.percents = percents.join(',');
this.aggConfig.percents = percents;
},
getEsAggConfig() {
if (!this.isValid()) {
@ -71,13 +88,36 @@ export function getPercentilesAggConfig(
return {
field: this.field as string,
percents: parsePercentsInput(this.aggConfig.percents),
percents: this.aggConfig.percents ?? [],
};
},
isValid() {
return (
typeof this.aggConfig.percents === 'string' && isValidPercentsInput(this.aggConfig.percents)
);
const validationResult = validatePercentsInput(this.aggConfig);
this.aggConfig.errors = validationResult.errors;
return validationResult.isValid;
},
getErrorMessages() {
if (!this.aggConfig.errors?.length) return;
return this.aggConfig.errors.map((error) => ERROR_MESSAGES[error]);
},
};
}
const ERROR_MESSAGES: Record<ValidationResultErrorType, string> = {
INVALID_FORMAT: i18n.translate('xpack.transform.agg.popoverForm.invalidFormatError', {
defaultMessage: 'Percentile must be a valid number',
}),
PERCENTILE_OUT_OF_RANGE: i18n.translate(
'xpack.transform.agg.popoverForm.percentileOutOfRangeError',
{
defaultMessage: 'Percentiles must be between 0 and 100',
}
),
NUMBER_TOO_PRECISE: i18n.translate('xpack.transform.agg.popoverForm.numberTooPreciseError', {
defaultMessage: 'Value is too precise. Use fewer decimal places.',
}),
DUPLICATE_VALUE: i18n.translate('xpack.transform.agg.popoverForm.duplicateValueError', {
defaultMessage: 'Value already exists',
}),
};

View file

@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
export const MAX_PERCENTILE_PRECISION = 17;
export const MAX_PERCENTILE_VALUE = 100;
export const MIN_PERCENTILE_VALUE = 0;

View file

@ -5,38 +5,84 @@
* 2.0.
*/
import React from 'react';
import React, { useCallback, useMemo } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFieldText, EuiFormRow } from '@elastic/eui';
import type { EuiComboBoxOptionOption } from '@elastic/eui';
import { EuiComboBox, EuiFormRow } from '@elastic/eui';
import type { IPivotAggsConfigPercentiles } from './types';
export const PercentilesAggForm: IPivotAggsConfigPercentiles['AggFormComponent'] = ({
aggConfig,
onChange,
isValid,
errorMessages,
}) => {
const selectedOptions = useMemo(
() => aggConfig.percents?.map((p) => ({ label: p.toString() })) ?? [],
[aggConfig.percents]
);
const handleCreateOption = useCallback(
(inputValue: string) => {
if (!isValid) return false;
const newValue = Number(inputValue.replace(',', '.'));
const newOption = {
label: newValue.toString(),
};
const updatedOptions = [...selectedOptions, newOption];
onChange({
percents: updatedOptions.map((option) => Number(option.label)),
});
},
[isValid, onChange, selectedOptions]
);
const handleOptionsChange = useCallback(
(newOptions: Array<EuiComboBoxOptionOption<string>>) => {
onChange({ percents: newOptions.map((option) => Number(option.label)) });
},
[onChange]
);
const handleSearchChange = useCallback(
(searchValue: string) => {
// If we're clearing the input after a valid creation,
// this is the post-creation cleanup
if (searchValue === '' && aggConfig.pendingPercentileInput && isValid) return;
onChange({
...aggConfig,
pendingPercentileInput: searchValue,
});
},
[aggConfig, onChange, isValid]
);
// Get the last error message if there are any
const lastErrorMessage = errorMessages?.length
? errorMessages[errorMessages.length - 1]
: undefined;
return (
<>
<EuiFormRow
label={i18n.translate('xpack.transform.agg.popoverForm.percentsLabel', {
defaultMessage: 'Percents',
})}
error={
!isValid && [
i18n.translate('xpack.transform.groupBy.popoverForm.intervalPercents', {
defaultMessage: 'Enter a comma-separated list of percentiles',
}),
]
}
error={lastErrorMessage}
isInvalid={!isValid}
>
<EuiFieldText
value={aggConfig.percents}
onChange={(e) => {
onChange({
percents: e.target.value,
});
}}
<EuiComboBox
noSuggestions
selectedOptions={selectedOptions}
onCreateOption={handleCreateOption}
onChange={handleOptionsChange}
onSearchChange={handleSearchChange}
isInvalid={!isValid}
data-test-subj="transformPercentilesAggPercentsSelector"
/>
</EuiFormRow>
</>

View file

@ -8,10 +8,23 @@
import type { PivotAggsConfigWithExtra } from '../../../../../../common/pivot_aggs';
export interface PercentilesAggConfig {
/** Comma separated list */
percents: string;
percents: number[];
pendingPercentileInput?: string;
errors?: ValidationResultErrorType[];
}
export type ValidationResultErrorType =
| 'INVALID_FORMAT'
| 'PERCENTILE_OUT_OF_RANGE'
| 'NUMBER_TOO_PRECISE'
| 'DUPLICATE_VALUE';
export type IPivotAggsConfigPercentiles = PivotAggsConfigWithExtra<
PercentilesAggConfig,
{ field: string; percents: number[] }
>;
export interface ValidationResult {
isValid: boolean;
errors?: ValidationResultErrorType[];
}

View file

@ -47488,7 +47488,6 @@
"xpack.transform.groupBy.popoverForm.fieldLabel": "Champ",
"xpack.transform.groupBy.popoverForm.intervalError": "Intervalle non valide.",
"xpack.transform.groupBy.popoverForm.intervalLabel": "Intervalle",
"xpack.transform.groupBy.popoverForm.intervalPercents": "Entrer une liste de centiles séparés par des virgules",
"xpack.transform.groupBy.popoverForm.invalidSizeErrorMessage": "Entrer un nombre positif valide",
"xpack.transform.groupBy.popoverForm.missingBucketCheckboxHelpText": "Cochez cette case pour inclure les documents sans valeur.",
"xpack.transform.groupby.popoverForm.missingBucketCheckboxLabel": "Inclure les compartiments manquants",

View file

@ -47448,7 +47448,6 @@
"xpack.transform.groupBy.popoverForm.fieldLabel": "フィールド",
"xpack.transform.groupBy.popoverForm.intervalError": "無効な間隔。",
"xpack.transform.groupBy.popoverForm.intervalLabel": "間隔",
"xpack.transform.groupBy.popoverForm.intervalPercents": "パーセンタイルをコンマで区切って列記します。",
"xpack.transform.groupBy.popoverForm.invalidSizeErrorMessage": "有効な正の数値を入力してください",
"xpack.transform.groupBy.popoverForm.missingBucketCheckboxHelpText": "選択すると、値がないドキュメントを含めます。",
"xpack.transform.groupby.popoverForm.missingBucketCheckboxLabel": "不足しているバケットを含める",

View file

@ -46722,7 +46722,6 @@
"xpack.transform.groupBy.popoverForm.fieldLabel": "字段",
"xpack.transform.groupBy.popoverForm.intervalError": "时间间隔无效。",
"xpack.transform.groupBy.popoverForm.intervalLabel": "时间间隔",
"xpack.transform.groupBy.popoverForm.intervalPercents": "输入百分位数的逗号分隔列表",
"xpack.transform.groupBy.popoverForm.invalidSizeErrorMessage": "输入有效的正数",
"xpack.transform.groupBy.popoverForm.missingBucketCheckboxHelpText": "选择包括没有值的文档。",
"xpack.transform.groupby.popoverForm.missingBucketCheckboxLabel": "包括缺失的存储桶",

View file

@ -302,6 +302,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
{
identifier: 'percentiles(products.base_price)',
label: 'products.base_price.percentiles',
form: {
transformPercentilesAggPercentsSelector: [1, 25, 50, 75, 100],
},
},
{
identifier: 'filter(customer_phone)',

View file

@ -605,6 +605,12 @@ export function TransformWizardProvider({ getService, getPageObjects }: FtrProvi
expectedLabel: string,
formData: Record<string, any>
) {
const isPopoverFormVisible = await testSubjects.exists(
`transformAggPopoverForm_${expectedLabel}`
);
if (!isPopoverFormVisible) {
await this.openPopoverForm(expectedLabel);
}
await testSubjects.existOrFail(`transformAggPopoverForm_${expectedLabel}`);
for (const [testObj, value] of Object.entries(formData)) {
@ -615,12 +621,19 @@ export function TransformWizardProvider({ getService, getPageObjects }: FtrProvi
case 'transformFilterTermValueSelector':
await this.fillFilterTermValue(value);
break;
case 'transformPercentilesAggPercentsSelector':
await this.fillPercentilesAggPercents(value);
break;
}
}
await testSubjects.clickWhenNotDisabled('transformApplyAggChanges');
await testSubjects.missingOrFail(`transformAggPopoverForm_${expectedLabel}`);
},
async openPopoverForm(expectedLabel: string) {
await testSubjects.click(`transformAggregationEntryEditButton_${expectedLabel}`);
},
async selectFilerAggType(value: string) {
await testSubjects.selectValue('transformFilterAggTypeSelector', value);
},
@ -629,6 +642,14 @@ export function TransformWizardProvider({ getService, getPageObjects }: FtrProvi
await comboBox.set('transformFilterTermValueSelector', value);
},
async fillPercentilesAggPercents(value: number[]) {
await comboBox.clear('transformPercentilesAggPercentsSelector');
for (const val of value) {
// Cast to string since Percentiles are usually passed as numbers
await comboBox.setCustom('transformPercentilesAggPercentsSelector', val.toString());
}
},
async assertAdvancedPivotEditorContent(expectedValue: string[]) {
const wrapper = await testSubjects.find('transformAdvancedPivotEditor');
const editor = await wrapper.findByCssSelector('.monaco-editor .view-lines');