From 2869d721af7e51653b992779990c9c490aa77b07 Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Mon, 28 Oct 2024 09:31:48 -0600 Subject: [PATCH] [ES|QL] detect the type of `COUNT(*)` (#197914) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary We weren't properly detecting the type of the expression `COUNT(*)`. Now we are! Before: Screenshot 2024-10-25 at 4 38 08 PM After: Screenshot 2024-10-25 at 4 35 44 PM ### Checklist - [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 (cherry picked from commit a5f0c1916e6348d82306e14c772c66f195ae781e) --- .../src/shared/helpers.test.ts | 76 +++++++------------ .../src/shared/helpers.ts | 32 ++++++-- 2 files changed, 53 insertions(+), 55 deletions(-) diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.test.ts b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.test.ts index 0078e0fac119c..e2e6397005e22 100644 --- a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.test.ts +++ b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.test.ts @@ -225,79 +225,47 @@ describe('getExpressionType', () => { }); it('detects the return type of a function', () => { - expect( - getExpressionType(getASTForExpression('returns_keyword()'), new Map(), new Map()) - ).toBe('keyword'); + expect(getExpressionType(getASTForExpression('returns_keyword()'))).toBe('keyword'); }); it('selects the correct signature based on the arguments', () => { - expect(getExpressionType(getASTForExpression('test("foo")'), new Map(), new Map())).toBe( - 'keyword' - ); - expect(getExpressionType(getASTForExpression('test(1.)'), new Map(), new Map())).toBe( - 'double' - ); - expect(getExpressionType(getASTForExpression('test(1., "foo")'), new Map(), new Map())).toBe( - 'long' - ); + expect(getExpressionType(getASTForExpression('test("foo")'))).toBe('keyword'); + expect(getExpressionType(getASTForExpression('test(1.)'))).toBe('double'); + expect(getExpressionType(getASTForExpression('test(1., "foo")'))).toBe('long'); }); it('supports nested functions', () => { expect( - getExpressionType( - getASTForExpression('test(1., test(test(test(returns_keyword()))))'), - new Map(), - new Map() - ) + getExpressionType(getASTForExpression('test(1., test(test(test(returns_keyword()))))')) ).toBe('long'); }); it('supports functions with casted results', () => { - expect( - getExpressionType(getASTForExpression('test(1.)::keyword'), new Map(), new Map()) - ).toBe('keyword'); + expect(getExpressionType(getASTForExpression('test(1.)::keyword'))).toBe('keyword'); }); it('handles nulls and string-date casting', () => { - expect(getExpressionType(getASTForExpression('test(NULL)'), new Map(), new Map())).toBe( - 'null' - ); - expect(getExpressionType(getASTForExpression('test(NULL, NULL)'), new Map(), new Map())).toBe( - 'null' - ); - expect( - getExpressionType(getASTForExpression('accepts_dates("", "")'), new Map(), new Map()) - ).toBe('keyword'); + expect(getExpressionType(getASTForExpression('test(NULL)'))).toBe('null'); + expect(getExpressionType(getASTForExpression('test(NULL, NULL)'))).toBe('null'); + expect(getExpressionType(getASTForExpression('accepts_dates("", "")'))).toBe('keyword'); }); it('deals with functions that do not exist', () => { - expect(getExpressionType(getASTForExpression('does_not_exist()'), new Map(), new Map())).toBe( - 'unknown' - ); + expect(getExpressionType(getASTForExpression('does_not_exist()'))).toBe('unknown'); }); it('deals with bad function invocations', () => { - expect( - getExpressionType(getASTForExpression('test(1., "foo", "bar")'), new Map(), new Map()) - ).toBe('unknown'); + expect(getExpressionType(getASTForExpression('test(1., "foo", "bar")'))).toBe('unknown'); - expect(getExpressionType(getASTForExpression('test()'), new Map(), new Map())).toBe( - 'unknown' - ); + expect(getExpressionType(getASTForExpression('test()'))).toBe('unknown'); - expect(getExpressionType(getASTForExpression('test("foo", 1.)'), new Map(), new Map())).toBe( - 'unknown' - ); + expect(getExpressionType(getASTForExpression('test("foo", 1.)'))).toBe('unknown'); }); it('deals with the CASE function', () => { - expect(getExpressionType(getASTForExpression('CASE(true, 1, 2)'), new Map(), new Map())).toBe( - 'integer' - ); + expect(getExpressionType(getASTForExpression('CASE(true, 1, 2)'))).toBe('integer'); - expect( - getExpressionType(getASTForExpression('CASE(true, 1., true, 1., 2.)'), new Map(), new Map()) - ).toBe('double'); + expect(getExpressionType(getASTForExpression('CASE(true, 1., true, 1., 2.)'))).toBe('double'); expect( getExpressionType( @@ -306,6 +274,20 @@ describe('getExpressionType', () => { new Map() ) ).toBe('keyword'); + + expect( + getExpressionType( + getASTForExpression('CASE(true, "", true, "", keywordVar)'), + new Map(), + new Map([ + [`keywordVar`, [{ name: 'keywordVar', type: 'keyword', location: { min: 0, max: 0 } }]], + ]) + ) + ).toBe('keyword'); + }); + + it('supports COUNT(*)', () => { + expect(getExpressionType(getASTForExpression('COUNT(*)'))).toBe('long'); }); }); diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts index 2392a44814997..18d6ae6faa246 100644 --- a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts +++ b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts @@ -815,15 +815,31 @@ export function getExpressionType( return 'unknown'; } + /** + * Special case for COUNT(*) because + * the "*" column doesn't match any + * of COUNT's function definitions + */ + if ( + fnDefinition.name === 'count' && + root.args[0] && + isColumnItem(root.args[0]) && + root.args[0].name === '*' + ) { + return 'long'; + } + if (fnDefinition.name === 'case' && root.args.length) { - // The CASE function doesn't fit our system of function definitions - // and needs special handling. This is imperfect, but it's a start because - // at least we know that the final argument to case will never be a conditional - // expression, always a result expression. - // - // One problem with this is that if a false case is not provided, the return type - // will be null, which we aren't detecting. But this is ok because we consider - // variables and fields to be nullable anyways and account for that during validation. + /** + * The CASE function doesn't fit our system of function definitions + * and needs special handling. This is imperfect, but it's a start because + * at least we know that the final argument to case will never be a conditional + * expression, always a result expression. + * + * One problem with this is that if a false case is not provided, the return type + * will be null, which we aren't detecting. But this is ok because we consider + * variables and fields to be nullable anyways and account for that during validation. + */ return getExpressionType(root.args[root.args.length - 1], fields, variables); }