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.

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(hogql): errors for large full select queries #15957

Merged
merged 7 commits into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 17 additions & 6 deletions frontend/src/queries/nodes/HogQLQuery/HogQLQueryEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import { useActions, useValues } from 'kea'
import { HogQLQuery } from '~/queries/schema'
import { useEffect, useRef, useState } from 'react'
import { hogQLQueryEditorLogic } from './hogQLQueryEditorLogic'
import MonacoEditor from '@monaco-editor/react'
import MonacoEditor, { Monaco } from '@monaco-editor/react'
import { AutoSizer } from 'react-virtualized/dist/es/AutoSizer'
import { LemonButton } from 'lib/lemon-ui/LemonButton'
import { Spinner } from 'lib/lemon-ui/Spinner'
import { Link } from 'lib/lemon-ui/Link'
import { urls } from 'scenes/urls'
import { IDisposable } from 'monaco-editor'
import { IDisposable, editor as importedEditor } from 'monaco-editor'

export interface HogQLQueryEditorProps {
query: HogQLQuery
Expand All @@ -18,8 +18,12 @@ export interface HogQLQueryEditorProps {
let uniqueNode = 0
export function HogQLQueryEditor(props: HogQLQueryEditorProps): JSX.Element {
const [key] = useState(() => uniqueNode++)
const hogQLQueryEditorLogicProps = { query: props.query, setQuery: props.setQuery, key }
const { queryInput } = useValues(hogQLQueryEditorLogic(hogQLQueryEditorLogicProps))
const [monacoAndEditor, setMonacoAndEditor] = useState(
null as [Monaco, importedEditor.IStandaloneCodeEditor] | null
)
const [monaco, editor] = monacoAndEditor ?? []
const hogQLQueryEditorLogicProps = { query: props.query, setQuery: props.setQuery, key, editor, monaco }
const { queryInput, hasErrors, error } = useValues(hogQLQueryEditorLogic(hogQLQueryEditorLogicProps))
const { setQueryInput, saveQuery } = useActions(hogQLQueryEditorLogic(hogQLQueryEditorLogicProps))

// Using useRef, not useState, as we don't want to reload the component when this changes.
Expand All @@ -42,7 +46,7 @@ export function HogQLQueryEditor(props: HogQLQueryEditorProps): JSX.Element {
</div>
<div
data-attr="hogql-query-editor"
className={'flex flex-col p-2 bg-border space-y-2 resize-y overflow-auto h-80 w-full'}
className={'flex flex-col p-2 bg-border space-y-2 resize-y h-80 w-full'}
>
<div className="flex-1">
<AutoSizer disableWidth>
Expand All @@ -63,6 +67,7 @@ export function HogQLQueryEditor(props: HogQLQueryEditorProps): JSX.Element {
run: () => saveQuery(),
})
)
setMonacoAndEditor([monaco, editor])
}}
options={{
minimap: {
Expand All @@ -79,7 +84,13 @@ export function HogQLQueryEditor(props: HogQLQueryEditorProps): JSX.Element {
onClick={saveQuery}
type="primary"
status={'muted-alt'}
disabledReason={!props.setQuery ? 'No permission to update' : undefined}
disabledReason={
!props.setQuery
? 'No permission to update'
: hasErrors
? error ?? 'Query has errors'
: undefined
}
fullWidth
center
>
Expand Down
63 changes: 61 additions & 2 deletions frontend/src/queries/nodes/HogQLQuery/hogQLQueryEditorLogic.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import { actions, kea, key, listeners, path, props, propsChanged, reducers } from 'kea'
import { HogQLQuery } from '~/queries/schema'
import { actions, kea, key, listeners, path, props, propsChanged, reducers, selectors } from 'kea'
import { HogQLMetadata, HogQLQuery, NodeKind } from '~/queries/schema'

import type { hogQLQueryEditorLogicType } from './hogQLQueryEditorLogicType'
import { editor, MarkerSeverity } from 'monaco-editor'
import { query } from '~/queries/query'
import { Monaco } from '@monaco-editor/react'

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface ModelMarker extends editor.IMarkerData {}

export interface HogQLQueryEditorLogicProps {
key: number
query: HogQLQuery
setQuery?: (query: HogQLQuery) => void
monaco?: Monaco | null
editor?: editor.IStandaloneCodeEditor | null
}

export const hogQLQueryEditorLogic = kea<hogQLQueryEditorLogicType>([
Expand All @@ -21,15 +29,66 @@ export const hogQLQueryEditorLogic = kea<hogQLQueryEditorLogicType>([
actions({
saveQuery: true,
setQueryInput: (queryInput: string) => ({ queryInput }),
setModelMarkers: (markers: ModelMarker[]) => ({ markers }),
}),
reducers(({ props }) => ({
queryInput: [props.query.query, { setQueryInput: (_, { queryInput }) => queryInput }],
modelMarkers: [[] as ModelMarker[], { setModelMarkers: (_, { markers }) => markers }],
})),
selectors({
hasErrors: [(s) => [s.modelMarkers], (modelMarkers) => !!modelMarkers?.length],
error: [
(s) => [s.hasErrors, s.modelMarkers],
(hasErrors, modelMarkers) =>
hasErrors && modelMarkers[0]
? `Error on line ${modelMarkers[0].startLineNumber}, column ${modelMarkers[0].startColumn}`
: null,
],
}),
listeners(({ actions, props, values }) => ({
saveQuery: () => {
const query = values.queryInput
actions.setQueryInput(query)
props.setQuery?.({ ...props.query, query })
},
setQueryInput: async (_, breakpoint) => {
if (!props.editor || !props.monaco) {
return
}
const model = props.editor?.getModel()
if (!model) {
return
}
await breakpoint(300)
const { queryInput } = values
const response = await query<HogQLMetadata>({
kind: NodeKind.HogQLMetadata,
select: queryInput,
})
breakpoint()
if (!response?.isValid) {
const start = model.getPositionAt(response?.errorStart ?? 0)
const end = model.getPositionAt(response?.errorEnd ?? queryInput.length)
const markers: ModelMarker[] = [
{
startLineNumber: start.lineNumber,
startColumn: start.column,
endLineNumber: end.lineNumber,
endColumn: end.column,
message: response?.error ?? 'Unknown error',
severity: MarkerSeverity.Error,
},
]
actions.setModelMarkers(markers)
} else {
actions.setModelMarkers([])
}
},
setModelMarkers: ({ markers }) => {
const model = props.editor?.getModel()
if (model) {
props.monaco?.editor.setModelMarkers(model, 'hogql', markers)
}
},
})),
])
1 change: 1 addition & 0 deletions frontend/utils.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export const commonConfig = {
'process.env.NODE_ENV': isDev ? '"development"' : '"production"',
},
loader: {
'.ttf': 'file',
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got an error when importing from monaco otherwise :/

'.png': 'file',
'.svg': 'file',
'.woff': 'file',
Expand Down
11 changes: 1 addition & 10 deletions posthog/hogql/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,7 @@ def __init__(self, message: str, *, start: Optional[int] = None, end: Optional[i
class SyntaxException(HogQLException):
"""Invalid HogQL syntax."""

line: int
column: int

def __init__(self, message: str, *, line: int, column: int):
super().__init__(message)
self.line = line
self.column = column

def __str__(self):
return f"Syntax error at line {self.line}, column {self.column}: {super().__str__()}"
pass


class QueryException(HogQLException):
Expand Down
21 changes: 19 additions & 2 deletions posthog/hogql/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,30 @@ def get_parser(query: str) -> HogQLParser:
stream = CommonTokenStream(lexer)
parser = HogQLParser(stream)
parser.removeErrorListeners()
parser.addErrorListener(HogQLErrorListener())
parser.addErrorListener(HogQLErrorListener(query))
return parser


class HogQLErrorListener(ErrorListener):
query: str

def __init__(self, query: str = ""):
super().__init__()
self.query = query

def get_position(self, line, column):
lines = self.query.split("\n")
try:
position = sum(len(lines[i]) + 1 for i in range(line - 1)) + column
except IndexError:
return -1
if position > len(self.query):
return -1
return position

def syntaxError(self, recognizer, offendingType, line, column, msg, e):
raise SyntaxException(msg, line=line, column=column)
start = max(self.get_position(line, column), 0)
raise SyntaxException(msg, start=start, end=len(self.query))


class HogQLParseTreeConverter(ParseTreeVisitor):
Expand Down
10 changes: 6 additions & 4 deletions posthog/hogql/test/test_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ def test_metadata_valid_expr_select(self):
"isValid": False,
"inputExpr": "select 1",
"inputSelect": None,
"error": "Syntax error at line 1, column 7: extraneous input '1' expecting <EOF>",
"error": "extraneous input '1' expecting <EOF>",
"errorStart": 7,
"errorEnd": 8,
},
)

Expand Down Expand Up @@ -55,9 +57,9 @@ def test_metadata_valid_expr_select(self):
"isValid": False,
"inputExpr": None,
"inputSelect": "timestamp",
"error": "Syntax error at line 1, column 0: mismatched input 'timestamp' expecting {SELECT, WITH, '('}",
"errorStart": None,
"errorEnd": None,
"error": "mismatched input 'timestamp' expecting {SELECT, WITH, '('}",
"errorStart": 0,
"errorEnd": 9,
},
)

Expand Down
12 changes: 5 additions & 7 deletions posthog/hogql/test/test_printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,12 @@ def test_expr_parse_errors_poe_off(self):
self._assert_expr_error("person", "Can't select a table when a column is expected: person")

def test_expr_syntax_errors(self):
self._assert_expr_error("(", "Syntax error at line 1, column 1: no viable alternative at input '('")
self._assert_expr_error("())", "Syntax error at line 1, column 1: no viable alternative at input '()'")
self._assert_expr_error(
"select query from events", "Syntax error at line 1, column 13: mismatched input 'from' expecting <EOF>"
)
self._assert_expr_error("(", "no viable alternative at input '('")
self._assert_expr_error("())", "no viable alternative at input '()'")
self._assert_expr_error("select query from events", "mismatched input 'from' expecting <EOF>")
self._assert_expr_error("this makes little sense", "Unable to resolve field: this")
self._assert_expr_error("1;2", "Syntax error at line 1, column 1: mismatched input ';' expecting <EOF>")
self._assert_expr_error("b.a(bla)", "Syntax error at line 1, column 3: mismatched input '(' expecting '.'")
self._assert_expr_error("1;2", "mismatched input ';' expecting <EOF>")
self._assert_expr_error("b.a(bla)", "mismatched input '(' expecting '.'")

def test_logic(self):
self.assertEqual(
Expand Down