feat(hog): C++ parser

[mariusandra]

Problem

The Python parser is slow

Changes

• Ports the Hog node parsing over to C++
• Also makes parsing {return} work

How did you test this code?

Moved the python-only parser tests into the universar parser tests file.

TODO:

• Release a new hogql_parser and run CI
• Release a new @posthog/hogvm

[posthog-bot]

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.15. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

[posthog-bot]

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.15. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

[mariusandra]

The build is still in progress (couldn't release a new hogql_parser because launchpad.net is down and the ARM deadsnakes python repo with it), but everything else could use some 👀

[Twixes]

Looks good! Only some suggestions around code clarity (and a couple missing DECREFs)

[Twixes]

visitPyListOfObjects() would be simpler, though wouldn't skip the empty statements – not sure how important this is

Suggested change

	PyObject* declarations = PyList_New(0);
	if (!declarations) {
	throw PyInternalError();
	}
	auto declaration_ctxs = ctx->declaration();
	for (auto declaration_ctx : declaration_ctxs) {
	if (!declaration_ctx->statement() || !declaration_ctx->statement()->emptyStmt()) {
	PyObject* statement;
	try {
	statement = visitAsPyObject(declaration_ctx);
	} catch (...) {
	Py_DECREF(declarations);
	throw;
	}
	int append_code = PyList_Append(declarations, statement);
	Py_DECREF(statement);
	if (append_code == -1) {
	Py_DECREF(declarations);
	throw PyInternalError();
	}
	}
	}
	PyObject* declarations = visitPyListOfObjects(ctx->declaration());

[mariusandra]

I definitely want to skip the empty statements in the parser and return a relatively clean tree... 🤔

[Twixes]

I think this would do the same:

Suggested change

	vector<string> identifiers;
	auto identifier_ctxs = ctx->identifier();
	identifiers.reserve(identifier_ctxs.size());
	for (auto identifier_ctx : identifier_ctxs) {
	identifiers.push_back(visitAsString(identifier_ctx));
	}
	return X_PyList_FromStrings(identifiers);
	visitPyListOfObjects(ctx->identifier());

[mariusandra]

I had to revert this. Not sure why, but it failed with Parsing failed due to bad type casting (even after adding the return I forgot the first time). The above works, so I'll 🙈

[Twixes]

Oh, of course, because the identifier rule visitor returns a string. So there need to be two steps (and an error guard):

Suggested change

	vector<string> identifiers;
	auto identifier_ctxs = ctx->identifier();
	identifiers.reserve(identifier_ctxs.size());
	for (auto identifier_ctx : identifier_ctxs) {
	identifiers.push_back(visitAsString(identifier_ctx));
	}
	return X_PyList_FromStrings(identifiers);
	PyObject* ret = X_PyList_FromStrings(visitAsVectorOfStrings(ctx->identifier()));
	if (!ret) {
	throw PyInternalError();
	}
	return ret;

[Twixes]

Though actually this goes against convention No. 3:

Stay out of Python land as long as possible. E.g. avoid using PyObject*s` for bools or strings.

So in this spirit, this visitor should only return the vector of strings, as is conventional. Then the rule that actually builds an AST node using IdentifierList can use X_PyList_FromStrings() itself – so that this method doesn't have to be needlessly concerned with Python internals

[posthog-bot]

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.18. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

[posthog-bot]

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.18. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

[Twixes]

Looks good!
One optional note on a point where my initial suggestion was wrong

[Twixes]

Oh, of course, because the identifier rule visitor returns a string. So there need to be two steps (and an error guard):

Suggested change

	vector<string> identifiers;
	auto identifier_ctxs = ctx->identifier();
	identifiers.reserve(identifier_ctxs.size());
	for (auto identifier_ctx : identifier_ctxs) {
	identifiers.push_back(visitAsString(identifier_ctx));
	}
	return X_PyList_FromStrings(identifiers);
	PyObject* ret = X_PyList_FromStrings(visitAsVectorOfStrings(ctx->identifier()));
	if (!ret) {
	throw PyInternalError();
	}
	return ret;

[posthog-bot]

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.19. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!