refactor: Unify compile paths with ResultNode #1636
Conversation
47c7449 to
886d287
Compare
bigframes/core/compile/compiler.py
Outdated
| node: nodes.BigFrameNode | ||
| sort_rows: bool | ||
| materialize_all_order_keys: bool = False | ||
| peek_rows: typing.Optional[int] = None |
There was a problem hiding this comment.
naming nit: "peek_count" to indicate that is a numeric value?
| distinct: bool = False, | ||
| ) -> Select: | ||
| if isinstance(columns, str): | ||
| columns = [columns] | ||
| self.select_list: typing.List[typing.Union[SelectExpression, SelectAll]] = ( | ||
| [ | ||
| SelectExpression(expression=expr.ColumnExpression(name=column)) | ||
| if isinstance(column, str) |
There was a problem hiding this comment.
This logic is probably complicated enough to be defined in a separate helper function.
bigframes/core/nodes.py
Outdated
| @property | ||
| def row_count(self) -> Optional[int]: | ||
| child_count = self.child.row_count | ||
| if child_count is not None: |
There was a problem hiding this comment.
Shall we unnest the code?
if child_count is None:
return None
if self.limit is None:
return child_count
return min(self.limit, child_count)
bigframes/core/nodes.py
Outdated
| output_names: tuple[str, ...] | ||
| order_by: Tuple[OrderingExpression, ...] = () | ||
| output_cols: tuple[tuple[ex.DerefOp, str], ...] | ||
| is_ordered: bool |
There was a problem hiding this comment.
is is_ordered necessary? Can we rely on the nullness of order_by to decide whether it is ordered?
If not, we might need to document how we are going to handle this confusing case when is_ordered == False and order_by == None
There was a problem hiding this comment.
yeah, not needed, removed
bigframes/core/compile/compiler.py
Outdated
| result_node = rewrites.defer_order( | ||
| result_node, output_hidden_row_keys=request.materialize_all_order_keys | ||
| ) | ||
| if not request.sort_rows: |
There was a problem hiding this comment.
style nit: let's re-write this if-else as:
if request.sort_rows:
...
else:
...
(the "else" indentation might be unnecessary too, because the "if" branch always returns)
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> 🦕