perf: Compilation no longer bounded by recursion #1464
Conversation
product-auto-label
bot
added
size: l
| for node in list(self.iter_nodes_topo()): | ||
| # child nodes have already been transformed | ||
| child_results = tuple(results[child] for child in node.child_nodes) | ||
| result = reduction(node, child_results) |
There was a problem hiding this comment.
The bottom_up and reduce_up are similar but the transformed methods APIs are different. Do we have a way to merge them into one? If not, could you please add docstring to reduce_up
There was a problem hiding this comment.
In theory, bottom_up could be implemented in terms of reduce_up, but will leave separate for now, maybe they'll have somewhat different optimizations or extra arguments later on. Added a brief description
bigframes/core/compile/api.py
Outdated
|
|
||
| class SQLCompiler: | ||
| def __init__(self, strict: bool = True): | ||
| self._compiler = compiler.Compiler(strict=strict) | ||
| # Not used, should maybe remove, if strictness is no longer compile-time |
There was a problem hiding this comment.
If it's not used, any reasons to keep it still?
bigframes/core/compile/compiler.py
Outdated
|
|
||
| @_compile_node.register | ||
| def compile_readtable(node: nodes.ReadTableNode, *args): | ||
| return compile_read_table_unordered(node.source, node.scan_list) |
There was a problem hiding this comment.
nit: compile_read_table_unordered is only used within this compile_readtable function. Would it improve readability to inline its logic here, or is there a reason for keeping it separate?
There was a problem hiding this comment.
I think there was an ordered version at some point, inlined now
bigframes/core/compile/compiler.py
Outdated
| ibis_table[scan_item.source_id].name(scan_item.id.sql) | ||
| for scan_item in scan.items | ||
| ), | ||
| scalar_op_compiler = compile_scalar.ScalarOpCompiler() |
There was a problem hiding this comment.
It's not used in this file. Could we remove it?
bigframes/core/compile/compiler.py
Outdated
| return compile_read_table_unordered(node.source, node.scan_list) | ||
|
|
||
|
|
||
| def read_table_as_unordered_ibis( |
There was a problem hiding this comment.
nit: _read_table_as_unordered_ibis since it's internal used only. Can we move all these helper methods in the bottom of this file?
There was a problem hiding this comment.
function removed per other comment.
There was a problem hiding this comment.
They're different functions. Could you double check?
Main gains are when you would otherwise hit stack overflow. Though, still a few other cases where you can hit stack overflow, so might not be that helpful by itself? |
bigframes/core/compile/compiler.py
Outdated
| return compile_read_table_unordered(node.source, node.scan_list) | ||
|
|
||
|
|
||
| def read_table_as_unordered_ibis( |
There was a problem hiding this comment.
They're different functions. Could you double check?
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> 🦕