Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: auto add quotes for table/columns #1109

Merged
merged 4 commits into from
Dec 22, 2022
Merged

feat: auto add quotes for table/columns #1109

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
854234f
feat: auto add quotes for table/columns
czgu Dec 21, 2022
de51e1b
fix wrong sql hint being registered
czgu Dec 22, 2022
7a20a84
update according to comments
czgu Dec 22, 2022
4e81e05
remove autocompleter from hooks ret
czgu Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions querybook/webapp/components/QueryEditor/QueryEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export const QueryEditor: React.FC<
language,
query: value,
});
const autoCompleter = useAutoComplete(
const { autoCompleter, autoCompleterRef } = useAutoComplete(
czgu marked this conversation as resolved.
Show resolved Hide resolved
metastoreId,
autoCompleteType,
language,
Expand Down Expand Up @@ -559,13 +559,16 @@ export const QueryEditor: React.FC<

const handleOnFocus = useCallback(
(editor: CodeMirror.Editor, event) => {
autoCompleter.registerHelper();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it's not needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it is needed, otherwise query hint wouldn't work. I changed back the code and added more comments


// This is needed because we could have multiple QueryEditor
// instances on the same page
// Note that we are using ref here because ReactCodeMirror doesn't
// use the new handleOnFocus - it only uses the one on mount
autoCompleterRef.current.registerHelper();
if (onFocus) {
onFocus(editor, event);
}
},
[onFocus, autoCompleter]
[onFocus, autoCompleterRef]
);

const handleOnKeyUp = useCallback(
Expand Down
25 changes: 13 additions & 12 deletions querybook/webapp/hooks/queryEditor/useAutoComplete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import CodeMirror from 'lib/codemirror';
import { useEffect, useMemo } from 'react';
import { useEffect, useMemo, useRef } from 'react';
import {
AutoCompleteType,
SqlAutoCompleter,
Expand All @@ -12,20 +12,21 @@ export function useAutoComplete(
language: string,
codeAnalysis: ICodeAnalysis
) {
const autoCompleter = useMemo(
() =>
new SqlAutoCompleter(
CodeMirror,
language,
metastoreId,
autoCompleteType
),
[language, metastoreId, autoCompleteType]
);
const autoCompleterRef = useRef<SqlAutoCompleter>();
const autoCompleter = useMemo(() => {
const completer = new SqlAutoCompleter(
CodeMirror,
language,
metastoreId,
autoCompleteType
);
autoCompleterRef.current = completer;
return completer;
}, [language, metastoreId, autoCompleteType]);

useEffect(() => {
autoCompleter.updateCodeAnalysis(codeAnalysis);
}, [codeAnalysis, autoCompleter]);

return autoCompleter;
return { autoCompleter, autoCompleterRef };
}
94 changes: 78 additions & 16 deletions querybook/webapp/lib/sql-helper/sql-autocompleter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getLanguageSetting } from './sql-setting';
import { getLanguageSetting, ILanguageSetting } from './sql-setting';

import CodeMirror from 'lib/codemirror';
import { ICodeAnalysis, TableToken } from 'lib/sql-helper/sql-lexer';
Expand Down Expand Up @@ -30,10 +30,17 @@ interface ICompletionRow {
score: number;
}

/**
* Flat: we treat the whole string as a single entity like table name or column name
* Hierarchical: we treat the '.' as a separator of entities
* null: disable quoting
*/
type QuoteType = 'flat' | 'hierarchical' | null;
type Formatter = (
word: string,
context?: string,
label?: string
label?: string,
quoteType?: QuoteType
) => ICompletionRow;

export type AutoCompleteType = 'none' | 'schema' | 'all';
Expand Down Expand Up @@ -103,6 +110,9 @@ function findLast(arr: Array<[number, any]>, num: number) {
return arr[index];
}

function checkNameNeedsEscape(name: string) {
return !name.match(/^\w+$/);
}
interface IAutoCompleteResult {
list: ICompletionRow[];
from: CodeMirror.Position;
Expand All @@ -118,6 +128,7 @@ export class SqlAutoCompleter {
private codeAnalysis?: ICodeAnalysis;
private metastoreId?: number;
private language: string;
private languageSetting: ILanguageSetting;
private keywords?: string[];
private type: AutoCompleteType;

Expand All @@ -129,12 +140,14 @@ export class SqlAutoCompleter {
) {
this.codeMirrorInstance = codeMirrorInstance;
this.metastoreId = metastoreId;
this.language = language;
this.type = type;

this.Pos = this.codeMirrorInstance.Pos;
this.codeAnalysis = null;

this.language = language;
this.languageSetting = getLanguageSetting(this.language);

this.registerHelper();
}

Expand All @@ -143,10 +156,33 @@ export class SqlAutoCompleter {
this.codeAnalysis = codeAnalysis;
}

@bind
private flatQuotationFormatter(name: string) {
if (!this.languageSetting.quoteChars) {
return name;
}

if (!checkNameNeedsEscape(name)) {
return name;
}

const [quoteStart, quoteEnd] = this.languageSetting.quoteChars;
return quoteStart + name + quoteEnd;
}

@bind
private quotationFormatter(name: string, quoteType: QuoteType) {
if (quoteType == null) {
return name;
} else if (quoteType === 'hierarchical') {
return name.split('.').map(this.flatQuotationFormatter).join('.');
}
return this.flatQuotationFormatter(name);
}

public getKeywords() {
if (!this.keywords) {
const languageSetting = getLanguageSetting(this.language);
this.keywords = [...languageSetting.keywords];
this.keywords = [...this.languageSetting.keywords];
}
return this.keywords;
}
Expand Down Expand Up @@ -233,7 +269,7 @@ export class SqlAutoCompleter {
(id) => dataSources.dataColumnsById[id].name
);
const filteredColumnNames = columnNames.filter((name) =>
name.startsWith(prefix)
name.toLowerCase().startsWith(prefix)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is prefix also lowercased?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, in this line

searchStr = token.string.toLowerCase();

);

return filteredColumnNames;
Expand All @@ -248,7 +284,13 @@ export class SqlAutoCompleter {
let results: ICompletionRow[] = [];
if (lineAnalysis.context === 'table') {
results = (await this.getTableNamesFromPrefix(searchStr)).map(
formatter.bind(null, lineAnalysis.context)
(tableName) =>
formatter(
tableName,
lineAnalysis.context,
undefined,
'hierarchical'
)
);
} else if (
lineAnalysis.context === 'column' &&
Expand All @@ -257,7 +299,14 @@ export class SqlAutoCompleter {
results = this.getColumnsFromPrefix(
searchStr,
lineAnalysis.reference
).map(formatter.bind(null, lineAnalysis.context));
).map((columnName) =>
formatter(
columnName,
lineAnalysis.context,
undefined,
'flat'
)
);
}
resolve(results);
});
Expand Down Expand Up @@ -287,10 +336,16 @@ export class SqlAutoCompleter {
const tableNames = await this.getTableNamesFromPrefix(prefix);

for (const tableName of tableNames) {
const match = tableName.match(/^(\w+)\.(\w+)$/);
if (match) {
const schemaTableNames = tableName.split('.');

if (schemaTableNames.length === 2) {
Comment on lines +339 to +341
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I saw this similar code piece multiple times, might be good to have tiny util function for it? can return an object with parsed schema name and table name, so that

const {schemaName, tableName} = parseFullTableName(..)
if (schemaName && tableName) {
   ...
   this.flatQuotationFormatter(tableName)
   ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm it is just .split('.') and used in 2 places, i think we can make a function for it if it is more common in future

results.push(
formatter(lineAnalysis.context, match[2], tableName)
formatter(
schemaTableNames[1],
lineAnalysis.context,
tableName,
'flat'
)
);
}
}
Expand All @@ -317,7 +372,8 @@ export class SqlAutoCompleter {

const prefix = context[context.length - 1];
results = this.getColumnsFromPrefix(prefix, tableNames).map(
(column) => formatter(lineAnalysis.context, column, column)
(column) =>
formatter(column, lineAnalysis.context, column, 'flat')
);
}

Expand Down Expand Up @@ -380,11 +436,12 @@ export class SqlAutoCompleter {
};
};

const flatFormatter: Formatter = (context, word) => {
const flatFormatter: Formatter = (word, context, _, quoteType) => {
const score = 0;

return {
originalText: text,
text: word,
text: this.quotationFormatter(word, quoteType),
label: word,
tooltip: context,
render: this.autoSuggestionRenderer,
Expand All @@ -393,11 +450,16 @@ export class SqlAutoCompleter {
};
};

const hierarchicalFormatter: Formatter = (context, word, label) => {
const hierarchicalFormatter: Formatter = (
word,
context,
label,
quoteType
) => {
const score = 0;
return {
originalText: text,
text: word,
text: this.quotationFormatter(word, quoteType),
label,
tooltip: context,
render: this.autoSuggestionRenderer,
Expand Down
7 changes: 6 additions & 1 deletion querybook/webapp/lib/sql-helper/sql-setting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// https://github.com/codemirror/CodeMirror/blob/master/mode/sql/sql.js
// Language Setting
interface ILanguageSetting {
export interface ILanguageSetting {
keywords: Set<string>;
type: Set<string>;
bool: Set<string>;
operatorChars: RegExp;
punctuationChars: RegExp;
placeholderVariable?: RegExp;
quoteChars?: [quoteStart: string, quoteEnd: string];
}

const SQL_KEYWORDS =
Expand All @@ -30,6 +31,7 @@ const SettingsByLanguage: Record<string, ILanguageSetting> = {
operatorChars: /^[*+\-%<>!=&|^~]/,
punctuationChars: /^[/:.]/,
placeholderVariable: /^\${.*?}/,
quoteChars: ['`', '`'],
// These are code mirror specific
// dateSQL: new Set("date timestamp".split(" ")),
// support: new Set("ODBCdotTable doubleQuote binaryNumber hexNumber".split(" "))
Expand All @@ -49,6 +51,7 @@ const SettingsByLanguage: Record<string, ILanguageSetting> = {
bool: new Set('false true null'.split(' ')),
operatorChars: /^[*+\-%<>!=|]/,
punctuationChars: /^[/:.]/,
quoteChars: ['"', '"'],
},
mysql: {
keywords: new Set(
Expand All @@ -65,6 +68,7 @@ const SettingsByLanguage: Record<string, ILanguageSetting> = {
bool: new Set('false true null'.split(' ')),
operatorChars: /^[*+\-%<>!=&|^~]/,
punctuationChars: /^[/:.]/,
quoteChars: ['`', '`'],
},
trino: {
keywords: new Set(
Expand All @@ -81,6 +85,7 @@ const SettingsByLanguage: Record<string, ILanguageSetting> = {
bool: new Set('false true null'.split(' ')),
operatorChars: /^[*+\-%<>!=|]/,
punctuationChars: /^[/:.]/,
quoteChars: ['"', '"'],
},
};
SettingsByLanguage['sparksql'] = SettingsByLanguage['hive'];
Expand Down