feat(bigframes): UDF transpiler handles some control flow#17558
feat(bigframes): UDF transpiler handles some control flow#17558TrevorBergeron wants to merge 13 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces control flow graph (CFG) construction and topological sorting to transpile Python bytecode with conditional branches and jumps into BigFrames expressions, supported by extensive unit tests. The review feedback highlights several critical improvements, including handling the JUMP_BACKWARD_NO_INTERRUPT instruction to prevent loops from being silently compiled, optimizing CFG building by precomputing instruction offsets to avoid O(N^2) sorting overhead, removing dead code for STORE_FAST, and correcting a misleading stack underflow error message.
tswast
left a comment
tswast
left a comment
There was a problem hiding this comment.
One more case statement that I worry about and a nit, but otherwise looking good, thanks!
| local_vars.get(var2, expression.UnboundVariableExpression(var2)) | ||
| ) | ||
|
|
||
| case name if name.startswith("LOAD_FAST"): |
There was a problem hiding this comment.
Given that we've observed at least two LOAD_FAST instructions that need to append two variables to the statck instead of 1, I worry about this "startswith" too. I'd rather fail for an unknown instruction than accidentally miss a value / generate incorrect code.
There was a problem hiding this comment.
Fixed, and yeah, best to just be entirely explicit everywhere about the exact instruction
| stack.pop() | ||
|
|
||
| case name if name in _ALL_JUMP_OPNAMES: | ||
| if opname in _UNCONDITIONAL_JUMP_OPNAMES: |
There was a problem hiding this comment.
Nit: Why are we nesting if statements inside the case statement instead of introducing more specific case statements? Other than the "jumped = True" line, these cases appear pretty independent. IMO at least that top level if/elif/else should be broken up into separate cases to reduce a bit of the nesting.
Alternatively, maybe we could break the jump handling into a separate function to reduce a bit of nesting that way.
There was a problem hiding this comment.
Flattened in new revision. I think there is some room to factor things, but will leave for later. Numba for instance dispatches methods based on op name and mutates an object representing VM state: https://github.com/numba/numba/blob/main/numba/core/byteflow.py
2 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕