perf: Automatically condense internal expression representation #516
Conversation
aeb22ad to
228b138
Compare
228b138 to
0fbcfe5
Compare
0fbcfe5 to
0beb855
Compare
7565d24 to
374b9d1
Compare
bigframes/core/__init__.py
Outdated
| @@ -183,7 +183,7 @@ def project_to_id(self, expression: ex.Expression, output_id: str): | |||
| child=self.node, | |||
| assignments=tuple(exprs), | |||
| ) | |||
| ) | |||
| ).rewrite_projection() | |||
There was a problem hiding this comment.
how to pick up these functions and ask for rewriting the projection? Can we add the rewrite_projection to the ArrayValue constructor? In case other developers who will add new function here and miss the rewrite_projection while missing context.
There was a problem hiding this comment.
moved to constructor in new revision.
There was a problem hiding this comment.
on second thought, this isn't really possible, as ArrayValue is an immutable dataclass, and mutation (by rewriting) is blocked even in the initializer
There was a problem hiding this comment.
I see. Thanks for trying that out! Maybe adding some comments for why we can add the rewrite_projection here? So other developers can have more context when they add something similar?
| if join_type == "right": | ||
| new_ordering = right.ordering | ||
| reverse_root = right.reverse_root | ||
| elif join_type == "outer": |
There was a problem hiding this comment.
do we need to redefine the reverse_root for the outer join case?
There was a problem hiding this comment.
outer join we treat as left order taking precedence, so it inherits reverse_root from the self.
| return OrderedIR( | ||
| self._table, | ||
| columns=self.columns, | ||
| hidden_ordering_columns=[*self._hidden_ordering_columns, *new_baked_cols], |
There was a problem hiding this comment.
Is it the intention to omit the existing _hidden_ordering_columns here?
There was a problem hiding this comment.
yes, we are rewriting the hidden columns here, so we are recreating the set of hidden columns and discarding the old set.
bigframes/core/__init__.py
Outdated
| @@ -183,7 +183,7 @@ def project_to_id(self, expression: ex.Expression, output_id: str): | |||
| child=self.node, | |||
| assignments=tuple(exprs), | |||
| ) | |||
| ) | |||
| ).rewrite_projection() | |||
There was a problem hiding this comment.
I see. Thanks for trying that out! Maybe adding some comments for why we can add the rewrite_projection here? So other developers can have more context when they add something similar?
… schema system.
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> 🦕