perf: Automatically condense internal expression representation #516
Conversation
product-auto-label bot added size: s 
TrevorBergeron force-pushed the auto_merge_project branch from aeb22ad to 228b138 Compare product-auto-label bot added size: m TrevorBergeron force-pushed the auto_merge_project branch from 228b138 to 0fbcfe5 Compare 
TrevorBergeron force-pushed the auto_merge_project branch from 0fbcfe5 to 0beb855 Compare TrevorBergeron changed the title TrevorBergeron force-pushed the auto_merge_project branch from 7565d24 to 374b9d1 Compare 
TrevorBergeron requested review from chelsea-lin and removed request for junyazhang 
bigframes/core/__init__.py Outdated
| 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
| 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?
3 participants
… 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> 🦕