feat(hogql): better errors #15001
Merged
feat(hogql): better errors #15001
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Twixes
reviewed
Apr 12, 2023
posthog/hogql/ast.py
Outdated
| @@ -32,12 +33,12 @@ def accept(self, visitor): | |||
| return visit(self) | |||
| if hasattr(visitor, "visit_unknown"): | |||
| return visitor.visit_unknown(self) | |||
| raise ValueError(f"Visitor has no method {method_name}") | |||
| raise HogQLException(f"Visitor has no method {method_name}") | |||
There was a problem hiding this comment.
I suppose this should be a NotImplementedException?
posthog/hogql/errors.py
Outdated
| @@ -0,0 +1,14 @@ | |||
| class HogQLException(ValueError): | |||
There was a problem hiding this comment.
To be somewaht pedantic, I'm not sure if all HogQL exceptions are ValueErrors in the "Raised when an operation or function receives an argument that has the right type but an inappropriate value" sense – e.g. a "NotImplementedException" results not from an inappropriate value, but from our transient lack of support for a value that is appropriate in principle.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Twixes
approved these changes
Apr 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
HogQL gives no errors and you have to play a guessing game to fix your query.
Changes
Instead of nothing, we now get some errors in the events explorer:
And in full SQL insights.
Unfortunately errors for "old insights" are still shown just in the network request:
I'm out of time to add support for nice errors in the interface for old insights, and it feels like something that will require custom handling with "project data exploration" anyway, so hoping @thmsobrmlr or @Twixes can help get this over the line...
While this could be added, this PR should be fine to merge already as is, because these errors will already help a lot of users.
One final point: Sometimes you can write valid HogQL, but still get a ClickHouse error. In that case, we currently show nothing. This will likely need followup as well. We should also allow-list clickhouse error codes and not send any potentially sensitive information. This closed PR implemented support for ClickHouse errors. It can be used as a starting point when implementing that.
How did you test this code?
Quickly tested in the browser (see screencasts).