refactor: Define RowJoinNode and defer rewrite #1183
Conversation
product-auto-label
bot
added
size: m
product-auto-label
bot
added
size: l
bigframes/core/blocks.py
Outdated
| @@ -2335,8 +2335,8 @@ def join( | |||
| # Handle null index, which only supports row join | |||
| # This is the canonical way of aligning on null index, so always allow (ignore block_identity_join) | |||
| if self.index.nlevels == other.index.nlevels == 0: | |||
| result = try_legacy_row_join(self, other, how=how) or try_new_row_join( | |||
| self, other | |||
| result = try_new_row_join(self, other) or try_legacy_row_join( | |||
There was a problem hiding this comment.
We should probably rename "try_new_row_join" now or in the future. The reason is that I guess "try_legacy_row_join" will be eventually removed, and it would be very confusing if we have only a "new" version.
We can just call it "try_row_join".
There was a problem hiding this comment.
renamed to try_row_join
| new_exprs = ( | ||
| *root.child.assignments, | ||
| *( | ||
| (expr.bind_refs(mapping, allow_partial_bindings=True), id) |
There was a problem hiding this comment.
This tuple comprehension is very long. Shall we use another local variable "root_assignments" to hold the value?
And
"new_exprs = tuple(root.child.assignments) + root_assignments"
There was a problem hiding this comment.
extracted out to variable as suggested
| # should be impossible, as l_path[-1] == r_path[-1] | ||
| raise ValueError() | ||
| min_height = min(root.height for root in roots) | ||
| to_descend = set(root for root in roots if root.height > min_height) |
There was a problem hiding this comment.
It took me a while to realize that to_descend is a set of root notes.
Let's name it "descend_roots" ?
There was a problem hiding this comment.
renamed to roots_to_descend
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> 🦕