From 3308a0f39f48aafc7cde5511322c188b81883785 Mon Sep 17 00:00:00 2001 From: skarri Date: Thu, 9 Feb 2023 12:05:35 -0600 Subject: [PATCH] feat: support for general message in query validation plugin --- .../validation/base_query_validator.py | 8 ++ .../validators/presto_explain_validator.py | 2 + .../components/QueryEditor/QueryEditor.tsx | 88 +++++++++++++------ querybook/webapp/const/queryExecution.ts | 1 + querybook/webapp/hooks/queryEditor/useLint.ts | 8 +- .../webapp/lib/codemirror/codemirror-lint.ts | 4 +- .../lib/sql-helper/sql-context-free-linter.ts | 2 + .../sql-context-sensitive-linter.ts | 2 + querybook/webapp/lib/sql-helper/sql-lexer.ts | 3 +- 9 files changed, 89 insertions(+), 29 deletions(-) diff --git a/querybook/server/lib/query_analysis/validation/base_query_validator.py b/querybook/server/lib/query_analysis/validation/base_query_validator.py index 9cff84969..fea06f2ad 100644 --- a/querybook/server/lib/query_analysis/validation/base_query_validator.py +++ b/querybook/server/lib/query_analysis/validation/base_query_validator.py @@ -7,6 +7,11 @@ ) +class QueryValidationResultObjectType(Enum): + LINT = "lint" + GENERAL = "general" + + class QueryValidationSeverity(Enum): ERROR = "error" WARNING = "warning" @@ -15,11 +20,13 @@ class QueryValidationSeverity(Enum): class QueryValidationResult(object): def __init__( self, + obj_type: QueryValidationResultObjectType, line: int, # 0 based ch: int, # location of the starting token severity: QueryValidationSeverity, message: str, ): + self.type = obj_type self.line = line self.ch = ch self.severity = severity @@ -27,6 +34,7 @@ def __init__( def to_dict(self): return { + "type": self.type.value, "line": self.line, "ch": self.ch, "severity": self.severity.value, diff --git a/querybook/server/lib/query_analysis/validation/validators/presto_explain_validator.py b/querybook/server/lib/query_analysis/validation/validators/presto_explain_validator.py index 3a0736b9f..9ad117ca5 100644 --- a/querybook/server/lib/query_analysis/validation/validators/presto_explain_validator.py +++ b/querybook/server/lib/query_analysis/validation/validators/presto_explain_validator.py @@ -4,6 +4,7 @@ BaseQueryValidator, QueryValidationResult, QueryValidationSeverity, + QueryValidationResultObjectType, ) from lib.utils.execute_query import ExecuteQuery from lib.query_executor.executors.presto import get_presto_error_dict @@ -55,6 +56,7 @@ def _map_statement_error_to_query( ) return QueryValidationResult( + QueryValidationResultObjectType.LINT, statement_error_line, statement_error_ch, QueryValidationSeverity.ERROR, diff --git a/querybook/webapp/components/QueryEditor/QueryEditor.tsx b/querybook/webapp/components/QueryEditor/QueryEditor.tsx index 86ae84256..0495704e0 100644 --- a/querybook/webapp/components/QueryEditor/QueryEditor.tsx +++ b/querybook/webapp/components/QueryEditor/QueryEditor.tsx @@ -10,6 +10,7 @@ import React, { } from 'react'; import { Controlled as ReactCodeMirror } from 'react-codemirror2'; import toast from 'react-hot-toast'; +import styled from 'styled-components'; import { showTooltipFor } from 'components/CodeMirrorTooltip'; import { ICodeMirrorTooltipProps } from 'components/CodeMirrorTooltip/CodeMirrorTooltip'; @@ -88,6 +89,13 @@ export interface IQueryEditorHandles { getEditorSelection: (editor?: CodeMirror.Editor) => IRange; } +const StyledQueryValidationMsg = styled.span.attrs({ + className: 'flex-row mr8', +})` + color: ${(props) => + props.type === 'info' ? 'var(--color-blue-dark)' : 'var(--color-false)'}; +`; + export const QueryEditor: React.FC< IQueryEditorProps & { ref: React.Ref; @@ -140,15 +148,23 @@ export const QueryEditor: React.FC< const [fullScreen, setFullScreen] = useState(false); - const { getLintAnnotations, lintSummary, isLinting } = useLint({ - query: value, - editorRef, - metastoreId, - codeAnalysis, - getTableByName, - getLintErrors, - onLintCompletion, - }); + const { getLintAnnotations, lintSummary, isLinting, queryAnnotations } = + useLint({ + query: value, + editorRef, + metastoreId, + codeAnalysis, + getTableByName, + getLintErrors, + onLintCompletion, + }); + + const generalAnnotation: null | ILinterWarning = useMemo(() => { + const list = queryAnnotations.filter( + (obj: ILinterWarning) => obj.type === 'general' + ); + return list.length > 0 ? list[0] : null; + }, [queryAnnotations]); const openTableModal = useCallback((tableId: number) => { navigateWithinEnv(`/table/${tableId}/`, { @@ -603,20 +619,7 @@ export const QueryEditor: React.FC< ); /* ---- end of properties ---- */ - const renderLintButtons = () => { - if (value.length === 0) { - return null; - } - - if (isLinting) { - return ( - - - Linting - - ); - } - + const renderLintButton = () => { if (lintSummary.numErrors + lintSummary.numWarnings > 0) { return (
{lintSummary.numErrors > 0 && ( @@ -658,10 +664,40 @@ export const QueryEditor: React.FC< } }; + const renderGeneralValidationMessage = () => { + if (!generalAnnotation) { + return null; + } + return ( + + {generalAnnotation.message} + + ); + }; + + const renderValidationMessages = () => { + if (value.length === 0) { + return null; + } + if (isLinting) { + return ( + + + Linting + + ); + } + return ( + <> + {renderGeneralValidationMessage()} + {renderLintButton()} + + ); + }; + const floatButtons = (
- {renderLintButtons()} - + {renderValidationMessages()} ([]); const lintAnnotations = useMemo( - () => tableAnnotations.concat(queryAnnotations), + () => + tableAnnotations.concat( + queryAnnotations.filter( + (obj: ILinterWarning) => obj.type === 'lint' + ) + ), [tableAnnotations, queryAnnotations] ); @@ -211,5 +216,6 @@ export function useLint({ getLintAnnotations: getCodeMirrorLintAnnotations, isLinting: isLintingQuery || isLintingTable, lintSummary, + queryAnnotations, }; } diff --git a/querybook/webapp/lib/codemirror/codemirror-lint.ts b/querybook/webapp/lib/codemirror/codemirror-lint.ts index c6a490b9d..7af84efc2 100644 --- a/querybook/webapp/lib/codemirror/codemirror-lint.ts +++ b/querybook/webapp/lib/codemirror/codemirror-lint.ts @@ -17,7 +17,7 @@ export function createSQLLinter( ); return validationResults.map((validationError) => { - const { line, ch, severity, message } = validationError; + const { type, line, ch, severity, message } = validationError; const errorToken = cm.getTokenAt({ line, @@ -37,6 +37,7 @@ export function createSQLLinter( }, severity, message, + type, } as ILinterWarning; } else { return { @@ -50,6 +51,7 @@ export function createSQLLinter( }, severity, message, + type, } as ILinterWarning; } }); diff --git a/querybook/webapp/lib/sql-helper/sql-context-free-linter.ts b/querybook/webapp/lib/sql-helper/sql-context-free-linter.ts index 02b2ce6d5..0dbbd7832 100644 --- a/querybook/webapp/lib/sql-helper/sql-context-free-linter.ts +++ b/querybook/webapp/lib/sql-helper/sql-context-free-linter.ts @@ -47,6 +47,7 @@ const contextFreeLinterWarningsByLanguage: Record< message: 'Do a "count(*) and group by" works better in terms of performance.', severity: 'warning', + type: 'lint', from: { line: token.line, ch: token.start, @@ -88,6 +89,7 @@ const contextFreeLinterWarningsByLanguage: Record< warnings.push({ message: 'Please add a LIMIT clause', severity: 'warning', + type: 'lint', from: { line: firstToken.line, ch: firstToken.start, diff --git a/querybook/webapp/lib/sql-helper/sql-context-sensitive-linter.ts b/querybook/webapp/lib/sql-helper/sql-context-sensitive-linter.ts index 72dd14d96..69f5247d0 100644 --- a/querybook/webapp/lib/sql-helper/sql-context-sensitive-linter.ts +++ b/querybook/webapp/lib/sql-helper/sql-context-sensitive-linter.ts @@ -22,6 +22,7 @@ export function getContextSensitiveWarnings( contextSensitiveWarnings.push({ message: `Table ${table.name} is newly created or does not exist`, severity: 'warning', + type: 'lint', from: { line: table.line, ch: implicitSchema @@ -55,6 +56,7 @@ export function getContextSensitiveWarnings( contextSensitiveWarnings.push({ message: warningMessage, + type: 'lint', severity: maxSeverity === DataTableWarningSeverity.ERROR ? 'error' diff --git a/querybook/webapp/lib/sql-helper/sql-lexer.ts b/querybook/webapp/lib/sql-helper/sql-lexer.ts index f0ac69ee9..fa9cff7c7 100644 --- a/querybook/webapp/lib/sql-helper/sql-lexer.ts +++ b/querybook/webapp/lib/sql-helper/sql-lexer.ts @@ -174,8 +174,9 @@ export interface IRange { } export interface ILinterWarning extends IRange { + type: 'lint' | 'general'; message: string; - severity: 'error' | 'warning'; + severity: 'error' | 'warning' | 'info'; } export interface ILineage {