feat(not-defining): Hog #22336
feat(not-defining): Hog #22336
Conversation
|
It looks like the code of |
|
@benjackwhite ready for a look 👀 |
There was a problem hiding this comment.
99% trusting the actual hog implementation itself. Some comments about the current implemented functions.
I'm definitely on the "lets go" camp now but I still think the danger things that we already know are a bit riskier make sense to perhaps leave out now and add "properly" (i.e. sending http calls via our webhook delivery service or something, console logs stored in some state instead of being actually written to the console)
| raise HogVMException(f"Invalid bytecode. Must start with '{HOGQL_BYTECODE_IDENTIFIER}'") | ||
|
|
||
| while (symbol := next(iterator, None)) is not None: | ||
| def check_timeout(): |
| expect(await executeHogQLBytecode(['_h', op.INTEGER, 0, op.INTEGER, 1, op.AND, 2], fields)).toBe(false) | ||
| expect(await executeHogQLBytecode(['_h', op.INTEGER, 1, op.INTEGER, 0, op.INTEGER, 1, op.OR, 3], fields)).toBe( | ||
| true | ||
| expect(await exec(['_h'], fields)).toBe(null) |
There was a problem hiding this comment.
Long term - we should make this run both python and nodejs in parallel against the same test cases if possible
There was a problem hiding this comment.
Absolutely. Especially as we're talking about adding a third target, Rust, as well.
hogvm/python/stl.py
Outdated
| return args[0][::-1] | ||
|
|
||
|
|
||
| def httpGet(name: str, args: list[Any], team: Optional[Team], stdout: Optional[list[str]], timeout: int): |
There was a problem hiding this comment.
tbh this is the only bit im not super in favour of having already as its the main place where we will have "dangerzone" thoughts in terms of long term running operations, DNS spoofing etc. If there's no rush, could we not add this later properly instead?
There was a problem hiding this comment.
(╯°□°)╯︵ ┻━┻
It's gone
hogvm/typescript/src/stl.ts
Outdated
| httpGet: (args, _, timeout) => { | ||
| async function httpGet(url: string, timeout: number = 5): Promise<string | null> { | ||
| const timeoutPromise = new Promise<null>((_, reject) => { | ||
| setTimeout(() => { | ||
| reject(new Error(`Request timed out after ${timeout} seconds`)) | ||
| }, timeout * 1000) | ||
| }) | ||
|
|
||
| try { | ||
| const response = await Promise.race([fetch(url), timeoutPromise]) | ||
| if (!response || !response.ok) { | ||
| throw new Error('Network response was not ok.') | ||
| } | ||
| return await response.text() | ||
| } catch (error) { | ||
| throw new Error(`Failed to fetch: ${error.message}`) | ||
| } | ||
| } | ||
|
|
||
| return httpGet(args[0], timeout) | ||
| }, | ||
| print: (args) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(...args) | ||
| }, |
There was a problem hiding this comment.
Both of these are the potential "danger" functions - one does arbitrary http calls and the other write to console and therefor IO. Likely we would want an execution wrapper that actually takes these calls and performs them carefully. Happy to approve and move forward but feels like something that could for sure be pushed back?
There was a problem hiding this comment.
Agreed. httpGet is gone. I kept the print for now, but will rework it in the next PR to append to a log array just like the Python version does. I don't see a big danger as the JS VM is not used anywhere now.
| class TestParserPython(parser_test_factory("python")): | ||
| pass | ||
| def _program(self, program: str, placeholders: dict[str, ast.Expr] | None = None) -> ast.Program: | ||
| return parse_program(program, placeholders=placeholders, start=None) | ||
|
|
||
| def test_program_variable_declarations(self): |
There was a problem hiding this comment.
Let's not make TestParserPython diverge from TestPythonCpp. They should inherit all the same stuff from parser_test_factory(), so that the implementations stay in sync. Ofc we can move faster with the Python one, but once in master, we should converge ASAP @mariusandra
There was a problem hiding this comment.
Absolutely. Adding C++ parsing support for Hog and fixing this is somewhere high on my list.
Changes
Adds a
"hog"feature flag which enables:/debug:The feature
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
See the PR
Out of scope / next steps