Skip to content

Commit

Permalink
chore(hogql): Full error & NULL handling in C++ parser (1.0.0) (#18240)
Browse files Browse the repository at this point in the history
* chore(hogql): Be defensive against NULLs in the C++ parser

* Clean up on C++ exceptions

* Add to CONTRIBUTING guide

* Revert `AllowShortFunctionsOnASingleLine` change

* Update HogQLX additions too

* Bump version to 1.0.0

* Use new hogql-parser version

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
Twixes and github-actions[bot] authored Oct 27, 2023
1 parent 79b5e90 commit 8f62e85
Show file tree
Hide file tree
Showing 10 changed files with 1,110 additions and 403 deletions.
1 change: 1 addition & 0 deletions hogql_parser/.clang-format
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
BasedOnStyle: Chromium
ColumnLimit: 120
AlignAfterOpenBracket: BlockIndent
AllowShortIfStatementsOnASingleLine: WithoutElse
25 changes: 20 additions & 5 deletions hogql_parser/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,32 @@

## Mandatory reading

If you're new to Python C/C++ extensions, there are some things you must have in your mind.
If you're new to Python C/C++ extensions, there are some things you must have in mind. The [Python/C API Reference Manual](https://docs.python.org/3/c-api/index.html) is worth a read as a whole.

### [Objects, Types and Reference Counts in CPython](https://docs.python.org/3/c-api/intro.html#objects-types-and-reference-counts)
The three pages below are must-reads though. They're key to writing production-ready code:

### [Objects, Types and Reference Counts](https://docs.python.org/3/c-api/intro.html#objects-types-and-reference-counts)

Key takeaways:

1. `Py_INCREF()` and `Py_DECREF()` need to be used accurately, or there'll be memory leaks (or, less likely, segfaults).
1. `Py_INCREF()` and `Py_DECREF()` need to be used accurately, or there'll be memory leaks (or, less likely, segfaults). This also applies to early exits, such as these caused by an error.
1. `Py_None`, `Py_True`, and `Py_False` are singletons, but they still need to be incref'd/decref'd - the best way to do create a new reference to them is wrapping them in `Py_NewRef()`.
1. Pretty much only `PyList_SET_ITEM()` _steals_ references (i.e. assumes ownership of objects passed into it), if you pass an object into any other function and no longer need it after that - remember to `Py_DECREF` it!
1. Pretty much only `PyList_SET_ITEM()` _steals_ references (i.e. assumes ownership of objects passed into it) - if you pass an object into any other function and no longer need it after that, remember to `Py_DECREF` it!

### [Exception Handling](https://docs.python.org/3/c-api/exceptions.html)

Key takeaways:

1. If a Python exception has been raised, the module method that was called from Python must stop execution and return `NULL` immediately.
> In `HogQLParseTreeConverter`, we are able to use C++ exceptions: throwing `SyntaxException`,
> `NotImplementedException`, or `ParsingException` results in the same exception being raised in Python as
> expected. Note that if a `visitAsFoo` call throws an exception and there are `PyObject*`s in scope, we have to
> remember about cleaning up their refcounts. At such call sites, a `try {} catch (...) {}` block is appropriate.
1. For all Python/C API calls returning `PyObject*`, make sure `NULL` wasn't returned - if it was, then something failed and the Python runtime has already set an exception (e.g. a `MemoryError`). The same applies to calls returning `int` - there the error value is `-1`. Exception: in `PyArg_Foo` functions failure is signaled by `0` and success by `1`.
> In `HogQLParseTreeConverter`, these internal Python failures are handled simply by throwing
> `PyInternalException`.
### [Building Values in CPython](https://docs.python.org/3/c-api/arg.html#building-values)
### [Building Values](https://docs.python.org/3/c-api/arg.html#building-values)

Key takeaways:

Expand Down
8 changes: 5 additions & 3 deletions hogql_parser/error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ using namespace std;

EXCEPTION_CLASS_IMPLEMENTATION(HogQLException, runtime_error)

EXCEPTION_CLASS_IMPLEMENTATION(HogQLSyntaxException, HogQLException)
EXCEPTION_CLASS_IMPLEMENTATION(HogQLNotImplementedException, HogQLException)
EXCEPTION_CLASS_IMPLEMENTATION(HogQLParsingException, HogQLException)
EXCEPTION_CLASS_IMPLEMENTATION(SyntaxException, HogQLException)
EXCEPTION_CLASS_IMPLEMENTATION(NotImplementedException, HogQLException)
EXCEPTION_CLASS_IMPLEMENTATION(ParsingException, HogQLException)

PyInternalException::PyInternalException() : exception() {}
12 changes: 9 additions & 3 deletions hogql_parser/error.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@
EXCEPTION_CLASS_DEFINITION(HogQLException, std::runtime_error)

// The input does not conform to HogQL syntax.
EXCEPTION_CLASS_DEFINITION(HogQLSyntaxException, HogQLException)
EXCEPTION_CLASS_DEFINITION(SyntaxException, HogQLException)

// This feature isn't implemented in HogQL (yet).
EXCEPTION_CLASS_DEFINITION(HogQLNotImplementedException, HogQLException)
EXCEPTION_CLASS_DEFINITION(NotImplementedException, HogQLException)

// An internal problem in the parser layer.
EXCEPTION_CLASS_DEFINITION(HogQLParsingException, HogQLException)
EXCEPTION_CLASS_DEFINITION(ParsingException, HogQLException)

// Python runtime errored out somewhere - this means we must use the error it's already raised.
class PyInternalException : public std::exception {
public:
PyInternalException();
};
Loading

0 comments on commit 8f62e85

Please sign in to comment.