From ef67968d2711648759bb2104c7e47600637af54a Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 21 Jan 2025 10:40:18 +0100 Subject: [PATCH 1/6] :bug: Return 0 on empty buckets if null flag is disabled --- .../plugins/shared/data/common/search/aggs/metrics/count.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts index 232b79266834b..4a71fe456eca9 100644 --- a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts +++ b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts @@ -48,6 +48,9 @@ export const getCountMetricAgg = () => if (value === 0 && agg.params.emptyAsNull) { return null; } + if (value == null && !agg.params.emptyAsNull) { + return 0; + } return value; }, isScalable() { From a85cdc56e027ed1c88551c66b9edf581e98fed34 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 21 Jan 2025 16:17:29 +0100 Subject: [PATCH 2/6] :bug: Improve fix --- .../plugins/shared/data/common/search/aggs/metrics/count.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts index 4a71fe456eca9..c8dd4f9ecbd44 100644 --- a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts +++ b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts @@ -48,8 +48,9 @@ export const getCountMetricAgg = () => if (value === 0 && agg.params.emptyAsNull) { return null; } - if (value == null && !agg.params.emptyAsNull) { - return 0; + if (value == null) { + // if the value is undefined, respect the emptyAsNull flag + return agg.params.emptyAsNull ? null : 0; } return value; }, From a87d8bba76209e9d14081994e546f52bd0a2226c Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 21 Jan 2025 16:17:39 +0100 Subject: [PATCH 3/6] :white_check_mark: Add unit tests --- .../common/search/aggs/metrics/count.test.ts | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts diff --git a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts new file mode 100644 index 0000000000000..4d5346e160ec4 --- /dev/null +++ b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts @@ -0,0 +1,87 @@ +/* + * 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", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import moment from 'moment'; +import { IMetricAggConfig } from './metric_agg_type'; +import { getCountMetricAgg } from './count'; + +function makeAgg(emptyAsNull: boolean = false, timeShift?: moment.Duration): IMetricAggConfig { + return { + getTimeShift() { + return timeShift; + }, + params: { + emptyAsNull, + }, + } as IMetricAggConfig; +} + +function getBucket(value: number | undefined, timeShift?: moment.Duration) { + const suffix = timeShift ? `_${timeShift.asMilliseconds()}` : ''; + return { ['doc_count' + suffix]: value }; +} + +describe('Count', () => { + it('should return the value for a non-shifted bucket', () => { + const agg = getCountMetricAgg(); + expect(agg.getValue(makeAgg(), getBucket(1000))).toBe(1000); + }); + + it('should return the value for a shifted bucket', () => { + const agg = getCountMetricAgg(); + const shift = moment.duration(1800000); + expect(agg.getValue(makeAgg(false, shift), getBucket(1000, shift))).toBe(1000); + }); + + describe('emptyAsNull', () => { + it('should return null if the value is 0 and the flag is enabled', () => { + const agg = getCountMetricAgg(); + expect(agg.getValue(makeAgg(true), getBucket(0))).toBe(null); + }); + + it('should return null if the value is undefined and the flag is enabled', () => { + const agg = getCountMetricAgg(); + expect(agg.getValue(makeAgg(true), getBucket(0))).toBe(null); + }); + + it('should return null if the value is 0 and the flag is enabled on a shifted count', () => { + const agg = getCountMetricAgg(); + const shift = moment.duration(1800000); + expect(agg.getValue(makeAgg(true, shift), getBucket(0, shift))).toBe(null); + }); + + it('should return null if the value is undefined and the flag is enabled on a shifted count', () => { + const agg = getCountMetricAgg(); + const shift = moment.duration(1800000); + expect(agg.getValue(makeAgg(true, shift), getBucket(undefined, shift))).toBe(null); + }); + + it('should return 0 if the value is 0 and the flag is disabled', () => { + const agg = getCountMetricAgg(); + expect(agg.getValue(makeAgg(false), getBucket(0))).toBe(0); + }); + + it('should return 0 if the value is undefined and the flag is disabled', () => { + const agg = getCountMetricAgg(); + expect(agg.getValue(makeAgg(false), getBucket(undefined))).toBe(0); + }); + + it('should return 0 if the value is 0 and the flag is disabled on a shifted count', () => { + const agg = getCountMetricAgg(); + const shift = moment.duration(1800000); + expect(agg.getValue(makeAgg(false, shift), getBucket(undefined, shift))).toBe(0); + }); + + it('should return 0 if the value is undefined and the flag is disabled on a shifted count', () => { + const agg = getCountMetricAgg(); + const shift = moment.duration(1800000); + expect(agg.getValue(makeAgg(false, shift), getBucket(undefined, shift))).toBe(0); + }); + }); +}); From a5f7fd598b7a54b7f7f5f9f08354faa2f184f154 Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Wed, 29 Jan 2025 11:25:11 +0100 Subject: [PATCH 4/6] Update src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts Co-authored-by: Marco Vettorello --- .../shared/data/common/search/aggs/metrics/count.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts index 4d5346e160ec4..1f9c1e57c462a 100644 --- a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts +++ b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.test.ts @@ -47,7 +47,7 @@ describe('Count', () => { it('should return null if the value is undefined and the flag is enabled', () => { const agg = getCountMetricAgg(); - expect(agg.getValue(makeAgg(true), getBucket(0))).toBe(null); + expect(agg.getValue(makeAgg(true), getBucket(undefined))).toBe(null); }); it('should return null if the value is 0 and the flag is enabled on a shifted count', () => { From e66661df5a8c869669c999e997f1328d4c64a272 Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Thu, 30 Jan 2025 12:19:09 +0100 Subject: [PATCH 5/6] Update src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts Co-authored-by: Peter Pisljar --- .../plugins/shared/data/common/search/aggs/metrics/count.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts index c8dd4f9ecbd44..255b43013e18b 100644 --- a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts +++ b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts @@ -48,7 +48,7 @@ export const getCountMetricAgg = () => if (value === 0 && agg.params.emptyAsNull) { return null; } - if (value == null) { + if (value == null && !agg.params.emptyAsNull) return 0; // if the value is undefined, respect the emptyAsNull flag return agg.params.emptyAsNull ? null : 0; } From 8994c12b0c00a20378482c5d6f26c6a569952fdd Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 30 Jan 2025 12:54:08 +0100 Subject: [PATCH 6/6] :recycle: Restore old code --- .../plugins/shared/data/common/search/aggs/metrics/count.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts index 255b43013e18b..c8dd4f9ecbd44 100644 --- a/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts +++ b/src/platform/plugins/shared/data/common/search/aggs/metrics/count.ts @@ -48,7 +48,7 @@ export const getCountMetricAgg = () => if (value === 0 && agg.params.emptyAsNull) { return null; } - if (value == null && !agg.params.emptyAsNull) return 0; + if (value == null) { // if the value is undefined, respect the emptyAsNull flag return agg.params.emptyAsNull ? null : 0; }