From 7709ae7c8f45a087677865972444c87f9b714565 Mon Sep 17 00:00:00 2001 From: Simon Sotak Date: Fri, 24 Mar 2023 11:30:47 +0100 Subject: [PATCH 1/5] Remove non-letter and 1-letter suggestions from empty select clause --- packages/server/src/complete/complete.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/server/src/complete/complete.ts b/packages/server/src/complete/complete.ts index b536c747..84da735f 100644 --- a/packages/server/src/complete/complete.ts +++ b/packages/server/src/complete/complete.ts @@ -244,7 +244,8 @@ class Completer { this.addCandidatesForSelectStar(fromNodes, schemaAndSubqueries) const expectedLiteralNodes = e.expected?.filter( - (v): v is ExpectedLiteralNode => v.type === 'literal' + (v): v is ExpectedLiteralNode => + v.type === 'literal' && hasAtLeastTwoLetters(v.text) ) || [] this.addCandidatesForExpectedLiterals(expectedLiteralNodes) this.addCandidatesForFunctions() @@ -419,3 +420,7 @@ export function complete( console.timeEnd('complete') return { candidates: candidates, error: completer.error } } + +function hasAtLeastTwoLetters(value: string): boolean { + return /[a-zA-Z].*[a-zA-Z]/.test(value) +} From bec9efdfce378ada8f2784ecb102423ceff62707 Mon Sep 17 00:00:00 2001 From: Simon Sotak Date: Fri, 24 Mar 2023 11:29:31 +0100 Subject: [PATCH 2/5] For empty select statement, suggest unscoped columns But only if no scoped columns were suggested --- packages/server/src/complete/complete.ts | 16 ++++++++++++++-- packages/server/test/complete.test.ts | 14 ++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/server/src/complete/complete.ts b/packages/server/src/complete/complete.ts index 84da735f..3125e823 100644 --- a/packages/server/src/complete/complete.ts +++ b/packages/server/src/complete/complete.ts @@ -249,7 +249,13 @@ class Completer { ) || [] this.addCandidatesForExpectedLiterals(expectedLiteralNodes) this.addCandidatesForFunctions() - this.addCandidatesForScopedColumns(fromNodes, schemaAndSubqueries) + const { addedSome } = this.addCandidatesForScopedColumns( + fromNodes, + schemaAndSubqueries + ) + if (!addedSome) { + this.addCandidatesForUnscopedColumns(fromNodes, schemaAndSubqueries) + } this.addCandidatesForAliases(fromNodes) this.addCandidatesForTables(schemaAndSubqueries, true) if (logger.isDebugEnabled()) @@ -379,14 +385,20 @@ class Completer { console.timeEnd('addCandidatesForSelectStar') } - addCandidatesForScopedColumns(fromNodes: FromTableNode[], tables: Table[]) { + addCandidatesForScopedColumns( + fromNodes: FromTableNode[], + tables: Table[] + ): { addedSome: boolean } { console.time('addCandidatesForScopedColumns') + let addedSome = false createCandidatesForScopedColumns(fromNodes, tables, this.lastToken).forEach( (v) => { + addedSome = true this.addCandidate(v) } ) console.timeEnd('addCandidatesForScopedColumns') + return { addedSome } } addCandidatesForUnscopedColumns(fromNodes: FromTableNode[], tables: Table[]) { diff --git a/packages/server/test/complete.test.ts b/packages/server/test/complete.test.ts index 637089b1..f64220c3 100644 --- a/packages/server/test/complete.test.ts +++ b/packages/server/test/complete.test.ts @@ -174,7 +174,6 @@ describe('on blank space', () => { test('complete inside SELECT', () => { const result = complete('SELECT ', { line: 0, column: 7 }, SIMPLE_SCHEMA) - expect(result.candidates.length).toEqual(12) // TODO whare are they? const expected = [ expect.objectContaining({ label: 'array_concat()' }), expect.objectContaining({ label: 'array_contains()' }), @@ -450,7 +449,6 @@ describe('Fully qualified table names', () => { { line: 0, column: 31 }, SIMPLE_NESTED_SCHEMA ) - expect(result.candidates.length).toEqual(1) const expected = [expect.objectContaining({ label: 'table3' })] expect(result.candidates).toEqual(expect.arrayContaining(expected)) }) @@ -880,18 +878,18 @@ test('complete aliased column inside function', () => { expect(result.candidates[0].label).toEqual('department_id') }) -test('complete column inside function', () => { - const sql = `SELECT TO_CHAR(empl, 'MM/DD/YYYY') FROM employees x` +test('complete table inside function', () => { + const sql = `SELECT TO_CHAR(empl, 'MM/DD/YYYY') FROM employees` const result = complete(sql, { line: 0, column: 19 }, COMPLEX_SCHEMA) - expect(result.candidates.length).toEqual(1) - expect(result.candidates[0].label).toEqual('employees') + const expected = [expect.objectContaining({ label: 'employees' })] + expect(result.candidates).toEqual(expect.arrayContaining(expected)) }) test('complete an alias inside function', () => { const sql = `SELECT TO_CHAR(an_ali, 'MM/DD/YYYY') FROM employees an_alias` const result = complete(sql, { line: 0, column: 21 }, COMPLEX_SCHEMA) - expect(result.candidates.length).toEqual(1) - expect(result.candidates[0].label).toEqual('an_alias') + const expected = [expect.objectContaining({ label: 'an_alias' })] + expect(result.candidates).toEqual(expect.arrayContaining(expected)) }) describe('From clause subquery', () => { From 8d2124cfc6e261f36b42eeed80ff94625ccd9167 Mon Sep 17 00:00:00 2001 From: Simon Sotak Date: Fri, 24 Mar 2023 13:07:39 +0100 Subject: [PATCH 3/5] Only add table candidates if we are inside a FROM clause This will avoid suggesting tables when cursor is between SELECT and FROM. See new unit test --- packages/server/src/complete/complete.ts | 12 +++++++++++- packages/server/test/complete.test.ts | 11 +++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/server/src/complete/complete.ts b/packages/server/src/complete/complete.ts index 3125e823..b8434c84 100644 --- a/packages/server/src/complete/complete.ts +++ b/packages/server/src/complete/complete.ts @@ -256,8 +256,18 @@ class Completer { if (!addedSome) { this.addCandidatesForUnscopedColumns(fromNodes, schemaAndSubqueries) } + this.addCandidatesForAliases(fromNodes) - this.addCandidatesForTables(schemaAndSubqueries, true) + + const fromNodesContainingCursor = fromNodes.filter((tableNode) => + isPosInLocation(tableNode.location, this.pos) + ) + const isCursorInsideFromClause = fromNodesContainingCursor.length > 0 + if (isCursorInsideFromClause) { + // only add table candidates if the cursor is inside a FROM clause or JOIN clause, etc. + this.addCandidatesForTables(schemaAndSubqueries, true) + } + if (logger.isDebugEnabled()) logger.debug( `candidates for error returns: ${JSON.stringify(this.candidates)}` diff --git a/packages/server/test/complete.test.ts b/packages/server/test/complete.test.ts index f64220c3..c1dc219e 100644 --- a/packages/server/test/complete.test.ts +++ b/packages/server/test/complete.test.ts @@ -181,6 +181,17 @@ describe('on blank space', () => { expect(result.candidates).toEqual(expect.arrayContaining(expected)) }) + test('complete after SELECT FROM schema2.table2', () => { + const result = complete( + 'SELECT FROM schema2.table2', + { line: 0, column: 8 }, + SIMPLE_NESTED_SCHEMA + ) + expect(result.candidates).not.toContainEqual( + expect.objectContaining({ label: 'TABLE1' }) + ) + }) + test('complete function inside WHERE select star', () => { const result = complete( 'SELECT * FROM tab1 WHERE arr', From 327ec036c2dd319e0556ecdb68bc549e6073f39a Mon Sep 17 00:00:00 2001 From: Simon Sotak Date: Fri, 24 Mar 2023 13:08:09 +0100 Subject: [PATCH 4/5] Rename addedSome at the call site --- packages/server/src/complete/complete.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/server/src/complete/complete.ts b/packages/server/src/complete/complete.ts index b8434c84..c302a38e 100644 --- a/packages/server/src/complete/complete.ts +++ b/packages/server/src/complete/complete.ts @@ -249,11 +249,10 @@ class Completer { ) || [] this.addCandidatesForExpectedLiterals(expectedLiteralNodes) this.addCandidatesForFunctions() - const { addedSome } = this.addCandidatesForScopedColumns( - fromNodes, - schemaAndSubqueries - ) - if (!addedSome) { + + const { addedSome: addedSomeScopedColumnCandidates } = + this.addCandidatesForScopedColumns(fromNodes, schemaAndSubqueries) + if (!addedSomeScopedColumnCandidates) { this.addCandidatesForUnscopedColumns(fromNodes, schemaAndSubqueries) } From 5569e6025755a85e6b3c289f591b20895f374000 Mon Sep 17 00:00:00 2001 From: Simon Sotak Date: Fri, 24 Mar 2023 13:53:09 +0100 Subject: [PATCH 5/5] Add types for database and catalog and use them --- .../src/complete/CompletionItemUtils.ts | 12 +++-- packages/server/src/complete/Identifier.ts | 39 +++++++++----- .../candidates/createTableCandidates.ts | 52 ++++++++++++------- 3 files changed, 66 insertions(+), 37 deletions(-) diff --git a/packages/server/src/complete/CompletionItemUtils.ts b/packages/server/src/complete/CompletionItemUtils.ts index 98c4c273..981450a0 100644 --- a/packages/server/src/complete/CompletionItemUtils.ts +++ b/packages/server/src/complete/CompletionItemUtils.ts @@ -2,11 +2,13 @@ import { CompletionItem, CompletionItemKind } from 'vscode-languageserver-types' import { DbFunction } from '../database_libs/AbstractClient' export const ICONS = { - KEYWORD: CompletionItemKind.Text, - COLUMN: CompletionItemKind.Interface, - TABLE: CompletionItemKind.Field, - FUNCTION: CompletionItemKind.Property, - ALIAS: CompletionItemKind.Variable, + KEYWORD: CompletionItemKind.Keyword, + COLUMN: CompletionItemKind.Field, + TABLE: CompletionItemKind.Constant, + DATABASE: CompletionItemKind.Enum, + CATALOG: CompletionItemKind.Folder, + FUNCTION: CompletionItemKind.Event, + ALIAS: CompletionItemKind.Constant, UTILITY: CompletionItemKind.Event, } diff --git a/packages/server/src/complete/Identifier.ts b/packages/server/src/complete/Identifier.ts index 43273c7d..9ffdb4fa 100644 --- a/packages/server/src/complete/Identifier.ts +++ b/packages/server/src/complete/Identifier.ts @@ -1,13 +1,5 @@ import { CompletionItem, CompletionItemKind } from 'vscode-languageserver-types' - -export const ICONS = { - KEYWORD: CompletionItemKind.Text, - COLUMN: CompletionItemKind.Interface, - TABLE: CompletionItemKind.Field, - FUNCTION: CompletionItemKind.Property, - ALIAS: CompletionItemKind.Variable, - UTILITY: CompletionItemKind.Event, -} +import { ICONS } from './CompletionItemUtils' type OnClause = 'FROM' | 'ALTER TABLE' | 'OTHERS' export class Identifier { @@ -44,18 +36,37 @@ export class Identifier { toCompletionItem(): CompletionItem { const idx = this.lastToken.lastIndexOf('.') const label = this.identifier.substring(idx + 1) - let kindName: string - if (this.kind === ICONS.TABLE) { + if ( + this.kind === ICONS.TABLE || + this.kind === ICONS.DATABASE || + this.kind === ICONS.CATALOG + ) { let tableName = label const i = tableName.lastIndexOf('.') if (i > 0) { tableName = label.substring(i + 1) } - kindName = 'table' - } else { - kindName = 'column' } + const kindName = (() => { + switch (this.kind) { + case ICONS.TABLE: + return 'table' + case ICONS.DATABASE: + return 'schema' + case ICONS.CATALOG: + return 'database' + case ICONS.FUNCTION: + return 'function' + case ICONS.ALIAS: + return 'table' + case ICONS.COLUMN: + return 'column' + default: + return 'column' + } + })() + const item: CompletionItem = { label: label, detail: `${kindName} ${this.detail}`, diff --git a/packages/server/src/complete/candidates/createTableCandidates.ts b/packages/server/src/complete/candidates/createTableCandidates.ts index 88d33242..58d396d2 100644 --- a/packages/server/src/complete/candidates/createTableCandidates.ts +++ b/packages/server/src/complete/candidates/createTableCandidates.ts @@ -36,19 +36,44 @@ export function createCatalogDatabaseAndTableCandidates( } const qualificationLevelNeeded = qualificationNeeded - qualificationLevel switch (qualificationLevelNeeded) { - case 0: - return [getFullyQualifiedTableName(table)] - case 1: - if (table.catalog && table.database) { - return [table.catalog + '.' + table.database] - } - if (table.database) { - return [table.database] + case 0: { + const tableIdentifier = new Identifier( + lastToken, + getFullyQualifiedTableName(table), + '', + ICONS.TABLE, + onFromClause ? 'FROM' : 'OTHERS' + ) + return [tableIdentifier] + } + case 1: { + const qualifiedDatabaseName = + table.catalog && table.database + ? table.catalog + '.' + table.database + : table.database + + if (qualifiedDatabaseName !== null) { + const databaseIdentifier = new Identifier( + lastToken, + qualifiedDatabaseName, + '', + ICONS.DATABASE, + onFromClause ? 'FROM' : 'OTHERS' + ) + return [databaseIdentifier] } break + } case 2: if (table.catalog) { - return [table.catalog] + const catalogIdentifier = new Identifier( + lastToken, + table.catalog, + '', + ICONS.CATALOG, + onFromClause ? 'FROM' : 'OTHERS' + ) + return [catalogIdentifier] } break } @@ -56,15 +81,6 @@ export function createCatalogDatabaseAndTableCandidates( }) return qualifiedEntities - .map((databaseEntity) => { - return new Identifier( - lastToken, - databaseEntity, - '', - ICONS.TABLE, - onFromClause ? 'FROM' : 'OTHERS' - ) - }) .filter((item) => item.matchesLastToken()) .map((item) => item.toCompletionItem()) }