Skip to content

Commit

Permalink
[Lens] Add support for decimals in percentiles (elastic#165703)
Browse files Browse the repository at this point in the history
## Summary

Fixes elastic#98853

This PR adds support for decimals (2 digits) in percentile operation.


![percentile_decimals_support](https://github.com/elastic/kibana/assets/924948/cd0d2901-ba6f-452e-955c-f9d774a4e27f)

Features:
* ✨ Add decimals support in percentile
  * 🐛 Fixed aggs optimization to work with decimals
* 💄 Show Toast for ranking reset when using decimals in both
percentile and percentile rank
* ✅ Extended `isValidNumber` to support digits check and added unit
tests for it
* ♻️ Added support also to `convert to Lens` feature

Added both unit and functional tests.


![percentile_rank_toast](https://github.com/elastic/kibana/assets/924948/a9be1f9f-a1b1-4f9f-90dc-55e2af8933e1)

When trying to add more digits than what is supported then it will show
the input as invalid:
<img width="347" alt="Screenshot 2023-09-05 at 12 24 03"
src="https://github.com/elastic/kibana/assets/924948/3c38474f-b78f-4144-bca7-3dc192313c09">

Also it works now as custom ranking column:
<img width="264" alt="Screenshot 2023-09-05 at 16 14 25"
src="https://github.com/elastic/kibana/assets/924948/cb7be312-7f7b-4dc1-95a3-d893de344585">
<img width="264" alt="Screenshot 2023-09-05 at 16 14 20"
src="https://github.com/elastic/kibana/assets/924948/1c13f66e-da78-4df6-bb4f-e811d47b66d5">

**Notes**: need to specify exact digits in percentile (2) because the
`any` step is not supported and need to specify a number. I guess
alternatives here are to either extend it to 4 digits or make it a
configurable thing.

### Checklist

Delete any items that are not applicable to this PR.

- [x] 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)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
  • Loading branch information
dej611 and stratoula authored Sep 12, 2023
1 parent 01f4bc2 commit b796f13
Show file tree
Hide file tree
Showing 25 changed files with 521 additions and 108 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ function PercentilesEditor({
id={`visEditorPercentileLabel${agg.id}`}
isInvalid={showValidation ? !isValid : false}
display="rowCompressed"
data-test-subj="visEditorPercentile"
>
<NumberList
labelledbyId={`visEditorPercentileLabel${agg.id}-legend`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@ const mockGetFieldByName = jest.fn();
const mockGetLabel = jest.fn();
const mockGetLabelForPercentile = jest.fn();

jest.mock('../utils', () => ({
getFieldNameFromField: jest.fn(() => mockGetFieldNameFromField()),
getLabel: jest.fn(() => mockGetLabel()),
getLabelForPercentile: jest.fn(() => mockGetLabelForPercentile()),
}));
jest.mock('../utils', () => {
const utils = jest.requireActual('../utils');
return {
...utils,
getFieldNameFromField: jest.fn(() => mockGetFieldNameFromField()),
getLabel: jest.fn(() => mockGetLabel()),
getLabelForPercentile: jest.fn(() => mockGetLabelForPercentile()),
};
});

describe('convertToPercentileColumn', () => {
const visType = 'heatmap';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import { METRIC_TYPES } from '@kbn/data-plugin/common';
import { SchemaConfig } from '../../..';
import { isFieldValid, PercentileParams } from '../..';
import { getFieldNameFromField, getLabelForPercentile } from '../utils';
import { getAggIdAndValue, getFieldNameFromField, getLabelForPercentile } from '../utils';
import { createColumn, getFormat } from './column';
import { PercentileColumn, CommonColumnConverterArgs } from './types';
import { SUPPORTED_METRICS } from './supported_metrics';
Expand Down Expand Up @@ -40,7 +40,7 @@ const getPercent = (

const { percents } = aggParams;

const [, percentStr] = aggId.split('.');
const [, percentStr] = getAggIdAndValue(aggId);

const percent = Number(percentStr);
if (!percents || !percents.length || percentStr === '' || isNaN(percent)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@ const mockIsStdDevAgg = jest.fn();
const mockGetFieldByName = jest.fn();
const originalGetFieldByName = stubLogstashDataView.getFieldByName;

jest.mock('../utils', () => ({
getFieldNameFromField: jest.fn((field) => field),
getMetricFromParentPipelineAgg: jest.fn(() => mockGetMetricFromParentPipelineAgg()),
isPercentileAgg: jest.fn(() => mockIsPercentileAgg()),
isPercentileRankAgg: jest.fn(() => mockIsPercentileRankAgg()),
isPipeline: jest.fn(() => mockIsPipeline()),
isStdDevAgg: jest.fn(() => mockIsStdDevAgg()),
}));
jest.mock('../utils', () => {
const utils = jest.requireActual('../utils');
return {
...utils,
getFieldNameFromField: jest.fn((field) => field),
getMetricFromParentPipelineAgg: jest.fn(() => mockGetMetricFromParentPipelineAgg()),
isPercentileAgg: jest.fn(() => mockIsPercentileAgg()),
isPercentileRankAgg: jest.fn(() => mockIsPercentileRankAgg()),
isPipeline: jest.fn(() => mockIsPipeline()),
isStdDevAgg: jest.fn(() => mockIsStdDevAgg()),
};
});

const dataView = stubLogstashDataView;
const visType = 'heatmap';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { Operations } from '../../constants';
import { isMetricWithField, getStdDeviationFormula, ExtendedColumnConverterArgs } from '../convert';
import { getFormulaFromMetric, SUPPORTED_METRICS } from '../convert/supported_metrics';
import {
getAggIdAndValue,
getFieldNameFromField,
getMetricFromParentPipelineAgg,
isPercentileAgg,
Expand Down Expand Up @@ -125,7 +126,7 @@ const getFormulaForPercentile = (
selector: string,
reducedTimeRange?: string
) => {
const percentile = Number(agg.aggId?.split('.')[1]);
const percentile = Number(getAggIdAndValue(agg.aggId)[1]);
const op = SUPPORTED_METRICS[agg.aggType];
if (!isValidAgg(visType, agg, dataView) || !op) {
return null;
Expand Down
14 changes: 14 additions & 0 deletions src/plugins/visualizations/common/convert_to_lens/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,17 @@ export const getMetricFromParentPipelineAgg = (

return metric as SchemaConfig<METRIC_TYPES>;
};

const aggIdWithDecimalsRegexp = /^(\w)+\['([0-9]+\.[0-9]+)'\]$/;

export const getAggIdAndValue = (aggId?: string) => {
if (!aggId) {
return [];
}
// agg value contains decimals
if (/\['/.test(aggId)) {
const [_, id, value] = aggId.match(aggIdWithDecimalsRegexp) || [];
return [id, value];
}
return aggId.split('.');
};
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,13 @@ jest.mock('../../common/convert_to_lens/lib/buckets', () => ({
convertBucketToColumns: jest.fn(() => mockConvertBucketToColumns()),
}));

jest.mock('../../common/convert_to_lens/lib/utils', () => ({
getCustomBucketsFromSiblingAggs: jest.fn(() => mockGetCutomBucketsFromSiblingAggs()),
}));
jest.mock('../../common/convert_to_lens/lib/utils', () => {
const utils = jest.requireActual('../../common/convert_to_lens/lib/utils');
return {
...utils,
getCustomBucketsFromSiblingAggs: jest.fn(() => mockGetCutomBucketsFromSiblingAggs()),
};
});

jest.mock('../vis_schemas', () => ({
getVisSchemas: jest.fn(() => mockGetVisSchemas()),
Expand Down
10 changes: 7 additions & 3 deletions src/plugins/visualizations/public/convert_to_lens/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import type { DataView } from '@kbn/data-views-plugin/common';
import { IAggConfig, METRIC_TYPES, TimefilterContract } from '@kbn/data-plugin/public';
import { AggBasedColumn, PercentageModeConfig, SchemaConfig } from '../../common';
import { convertMetricToColumns } from '../../common/convert_to_lens/lib/metrics';
import { getCustomBucketsFromSiblingAggs } from '../../common/convert_to_lens/lib/utils';
import {
getAggIdAndValue,
getCustomBucketsFromSiblingAggs,
} from '../../common/convert_to_lens/lib/utils';
import { BucketColumn } from '../../common/convert_to_lens/lib';
import type { Vis } from '../types';
import { getVisSchemas, Schemas } from '../vis_schemas';
Expand Down Expand Up @@ -178,11 +181,12 @@ export const getColumnsFromVis = <T>(

if (series && series.length) {
for (const { metrics: metricAggIds } of series) {
const metricAggIdsLookup = new Set(metricAggIds);
const metrics = aggs.filter(
(agg) => agg.aggId && metricAggIds.includes(agg.aggId.split('.')[0])
(agg) => agg.aggId && metricAggIdsLookup.has(getAggIdAndValue(agg.aggId)[0])
);
const customBucketsForLayer = customBucketsWithMetricIds.filter((c) =>
c.metricIds.some((m) => metricAggIds.includes(m))
c.metricIds.some((m) => metricAggIdsLookup.has(m))
);
const layer = createLayer(
vis.type.name,
Expand Down
11 changes: 11 additions & 0 deletions test/functional/page_objects/visualize_editor_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,21 @@ export class VisualizeEditorPageObject extends FtrService {
const input = await this.find.byCssSelector(
'[data-test-subj="visEditorPercentileRanks"] input'
);
this.log.debug(`Setting percentile rank value of ${newValue}`);
await input.clearValue();
await input.type(newValue);
}

public async setPercentileValue(newValue: string, index: number = 0) {
const correctIndex = index * 2 + 1;
const input = await this.find.byCssSelector(
`[data-test-subj="visEditorPercentile"]>div:nth-child(2)>div:nth-child(${correctIndex}) input`
);
this.log.debug(`Setting percentile value at ${index}th input of ${newValue}`);
await input.clearValueWithKeyboard();
await input.type(newValue, { charByChar: true });
}

public async clickEditorSidebarCollapse() {
await this.testSubjects.click('collapseSideBarButton');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import {
DimensionEditorButtonGroups,
CalloutWarning,
DimensionEditorGroupsOptions,
isLayerChangingDueToDecimalsPercentile,
} from './dimensions_editor_helpers';
import type { TemporaryState } from './dimensions_editor_helpers';
import { FieldInput } from './field_input';
Expand Down Expand Up @@ -124,11 +125,14 @@ export function DimensionEditor(props: DimensionEditorProps) {

const [temporaryState, setTemporaryState] = useState<TemporaryState>('none');
const [isHelpOpen, setIsHelpOpen] = useState(false);

// If a layer has sampling disabled, assume the toast has already fired in the past
const [hasRandomSamplingToastFired, setSamplingToastAsFired] = useState(
!isSamplingValueEnabled(state.layers[layerId])
);

const [hasRankingToastFired, setRankingToastAsFired] = useState(false);

const onHelpClick = () => setIsHelpOpen((prevIsHelpOpen) => !prevIsHelpOpen);
const closeHelp = () => setIsHelpOpen(false);

Expand Down Expand Up @@ -163,6 +167,32 @@ export function DimensionEditor(props: DimensionEditorProps) {
[hasRandomSamplingToastFired, layerId, props.notifications.toasts, state.layers]
);

const fireOrResetRankingToast = useCallback(
(newLayer: FormBasedLayer) => {
if (isLayerChangingDueToDecimalsPercentile(state.layers[layerId], newLayer)) {
props.notifications.toasts.add({
title: i18n.translate('xpack.lens.uiInfo.rankingResetTitle', {
defaultMessage: 'Ranking changed to alphabetical',
}),
text: i18n.translate('xpack.lens.uiInfo.rankingResetToAlphabetical', {
defaultMessage: 'To rank by percentile, use whole numbers only.',
}),
});
}
// reset the flag if the user switches to another supported operation
setRankingToastAsFired(!hasRankingToastFired);
},
[hasRankingToastFired, layerId, props.notifications.toasts, state.layers]
);

const fireOrResetToastChecks = useCallback(
(newLayer: FormBasedLayer) => {
fireOrResetRandomSamplingToast(newLayer);
fireOrResetRankingToast(newLayer);
},
[fireOrResetRandomSamplingToast, fireOrResetRankingToast]
);

const setStateWrapper = useCallback(
(
setter:
Expand Down Expand Up @@ -203,7 +233,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
}
const newLayer = adjustColumnReferencesForChangedColumn(outputLayer, columnId);
// Fire an info toast (eventually) on layer update
fireOrResetRandomSamplingToast(newLayer);
fireOrResetToastChecks(newLayer);

return mergeLayer({
state: prevState,
Expand All @@ -217,7 +247,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
}
);
},
[columnId, fireOrResetRandomSamplingToast, layerId, setState, state.layers]
[columnId, fireOrResetToastChecks, layerId, setState, state.layers]
);

const incompleteInfo = (state.layers[layerId].incompleteColumns ?? {})[columnId];
Expand Down Expand Up @@ -811,7 +841,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
field,
visualizationGroups: dimensionGroups,
});
fireOrResetRandomSamplingToast(newLayer);
fireOrResetToastChecks(newLayer);
updateLayer(newLayer);
}}
onChooseField={(choice: FieldChoiceWithOperationType) => {
Expand Down Expand Up @@ -846,7 +876,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
} else {
newLayer = setter;
}
fireOrResetRandomSamplingToast(newLayer);
fireOrResetToastChecks(newLayer);
return updateLayer(adjustColumnReferencesForChangedColumn(newLayer, referenceId));
}}
validation={validation}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,71 @@ import './dimension_editor.scss';
import React from 'react';
import { i18n } from '@kbn/i18n';
import { EuiCallOut, EuiButtonGroup, EuiFormRow } from '@elastic/eui';
import { operationDefinitionMap } from '../operations';
import { nonNullable } from '../../../utils';
import {
operationDefinitionMap,
type PercentileIndexPatternColumn,
type PercentileRanksIndexPatternColumn,
type TermsIndexPatternColumn,
} from '../operations';
import { isColumnOfType } from '../operations/definitions/helpers';
import { FormBasedLayer } from '../types';

export const formulaOperationName = 'formula';
export const staticValueOperationName = 'static_value';
export const quickFunctionsName = 'quickFunctions';
export const termsOperationName = 'terms';
export const optionallySortableOperationNames = ['percentile', 'percentile_ranks'];
export const nonQuickFunctions = new Set([formulaOperationName, staticValueOperationName]);

export type TemporaryState = typeof quickFunctionsName | typeof staticValueOperationName | 'none';

export function isLayerChangingDueToDecimalsPercentile(
prevLayer: FormBasedLayer,
newLayer: FormBasedLayer
) {
// step 1: find the ranking column in prevState and return its value
const termsRiskyColumns = Object.entries(prevLayer.columns)
.map(([id, column]) => {
if (
isColumnOfType<TermsIndexPatternColumn>('terms', column) &&
column.params?.orderBy.type === 'column' &&
column.params.orderBy.columnId != null
) {
const rankingColumn = prevLayer.columns[column.params.orderBy.columnId];
if (isColumnOfType<PercentileIndexPatternColumn>('percentile', rankingColumn)) {
if (Number.isInteger(rankingColumn.params.percentile)) {
return { id, rankId: column.params.orderBy.columnId };
}
}
if (isColumnOfType<PercentileRanksIndexPatternColumn>('percentile_rank', rankingColumn)) {
if (Number.isInteger(rankingColumn.params.value)) {
return { id, rankId: column.params.orderBy.columnId };
}
}
}
})
.filter(nonNullable);
// now check again the terms risky column in the new layer and verify that at
// least one changed due to decimals
const hasChangedDueToDecimals = termsRiskyColumns.some(({ id, rankId }) => {
const termsColumn = newLayer.columns[id];
if (!isColumnOfType<TermsIndexPatternColumn>('terms', termsColumn)) {
return false;
}
if (termsColumn.params.orderBy.type === 'alphabetical') {
const rankingColumn = newLayer.columns[rankId];
if (isColumnOfType<PercentileIndexPatternColumn>('percentile', rankingColumn)) {
return !Number.isInteger(rankingColumn.params.percentile);
}
if (isColumnOfType<PercentileRanksIndexPatternColumn>('percentile_rank', rankingColumn)) {
return !Number.isInteger(rankingColumn.params.value);
}
}
});
return hasChangedDueToDecimals;
}

export function isQuickFunction(operationType: string) {
return !nonQuickFunctions.has(operationType);
}
Expand Down
Loading

0 comments on commit b796f13

Please sign in to comment.