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

Jump to bottom

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 2 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
4 changes: 1 addition & 3 deletions querybook/webapp/components/QueryEditor/QueryEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,13 +559,11 @@ 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


if (onFocus) {
onFocus(editor, event);
}
},
[onFocus, autoCompleter]
[onFocus]
);

const handleOnKeyUp = useCallback(
Expand Down
65 changes: 50 additions & 15 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 @@ -103,6 +103,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 +121,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 +133,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 +149,28 @@ 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 hierarchicalQuotationFormatter(name: string) {
return name.split('.').map(this.flatQuotationFormatter).join('.');
}

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 +257,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 @@ -247,17 +271,19 @@ export class SqlAutoCompleter {
return new Promise(async (resolve) => {
let results: ICompletionRow[] = [];
if (lineAnalysis.context === 'table') {
results = (await this.getTableNamesFromPrefix(searchStr)).map(
formatter.bind(null, lineAnalysis.context)
);
results = (await this.getTableNamesFromPrefix(searchStr))
.map(this.hierarchicalQuotationFormatter)
.map(formatter.bind(null, lineAnalysis.context));
} else if (
lineAnalysis.context === 'column' &&
lineAnalysis.reference
) {
results = this.getColumnsFromPrefix(
searchStr,
lineAnalysis.reference
).map(formatter.bind(null, lineAnalysis.context));
)
.map(this.flatQuotationFormatter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about just move this .map(this.flatQuotationFormatter) into getColumnsFromPrefix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i moved all of the quotation formatting logic to the formatter level so it would not interfere with display

.map(formatter.bind(null, lineAnalysis.context));
}
resolve(results);
});
Expand Down Expand Up @@ -287,10 +313,17 @@ 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(
lineAnalysis.context,
this.flatQuotationFormatter(
schemaTableNames[1]
),
tableName
)
);
}
}
Expand All @@ -316,9 +349,11 @@ export class SqlAutoCompleter {
}

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

resolve(results);
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