refactor: Reimplement implicit joiner at expression layer #436

TrevorBergeron · 2024-03-13T17:18:46Z

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:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

tswast · 2024-03-15T21:44:43Z

bigframes/core/expression.py

@@ -81,6 +81,12 @@ def output_type(
    ) -> dtypes.ExpressionType:
        ...

+    @abc.abstractmethod
+    def bind_variables(


Could we get a docstring for this, please?

I assume this returns a version of the expression where free variables are replaced with the expressions in the bindings mapping?

What does bind_all control? Looks like it raises a ValueError if there are any UnboundVariables left?

On second look, I see that this is maybe a recursive sort of function where the variables bind themselves? Though maybe the effect is the same where free variables are replaced with expressions.

removed bind_all parameter as it was always set. And yes, this method replaces all the variables in the expression with the corresponding expression given in the bindings input.

tswast · 2024-03-15T21:47:58Z

bigframes/core/ordering.py

+class OrderingExpression:
+    """A more flexible version of OrderingColumnReference."""
+
+    # TODO: Maybe migrate BFET to use this?


Could we add a little more detail to this TODO, please?

extended the todo

tswast · 2024-03-15T21:48:08Z

bigframes/core/ordering.py

@@ -61,6 +63,26 @@ def with_reverse(self):
        )


+@dataclass(frozen=True)
+class OrderingExpression:
+    """A more flexible version of OrderingColumnReference."""


Let's lead in the first line what the purpose is. e.g. "Defines ordering for an expression" or something like that.

Also, more flexible in what way? That could be a note in the docstring body.

added more info to docstring

tswast · 2024-03-15T21:50:42Z

bigframes/core/ordering.py

+    direction: OrderingDirection = OrderingDirection.ASC
+    na_last: bool = True
+
+    def remap_names(self, mapping: dict[str, str]):


This needs a docstring. I'm not 100% certain the purpose.

For type annotations, looks like we can use Self here for this builder pattern.

https://www.turingtaco.com/recursive-types/

wasn't able to get this working. Think we need to be on python 3.11

tswast · 2024-03-15T21:52:33Z

bigframes/core/ordering.py

+            self.scalar_expression.rename(mapping), self.direction, self.na_last
+        )
+
+    def with_reverse(self):


Ditto

Suggested change

def with_reverse(self):

def with_reverse(self) -> Self:

I just put the explicit type in new revision as couldn't get Self working. This would be definitely helpful for abstract base classes though.

tswast · 2024-03-15T21:53:16Z

bigframes/core/rewrite.py

+
+
+@dataclasses.dataclass
+class NormalizedSelect:


Sorry, normalized in what way? Let's do a docstring here too.

Not really normalized so much as it is the result of squashing together projections, filters and orderbys as much as possible

tswast · 2024-03-15T21:55:42Z

bigframes/core/rewrite.py

+        new_ordering = tuple(expr.with_reverse() for expr in self.ordering)
+        return NormalizedSelect(self.root, self.columns, self.predicate, new_ordering)
+
+    def order_with(self, by: Tuple[order.OrderingColumnReference, ...]):


Why not take an OrderingExpression here? Seems a bit odd to add new interfaces that use something that might be deprecated.

We are trying to consume and OrderByNode here, and that still uses OrderingColumnReference. I think we might want to migrate these nodes to use the more general OrderingExpression, but that would substantially expand the scope of this PR.

tswast · 2024-03-15T21:58:16Z

bigframes/core/__init__.py

        )
+        if allow_row_identity_join:
+            as_selection = bigframes.core.rewrite.rewrite_join(join_node)


Why doesn't rewrite_join return join_node if it can't rewrite? Seems like this should be called maybe_join like the lower layer does if it might return None.

tswast · 2024-03-15T22:00:33Z

bigframes/core/rewrite.py

+        if (self.root == right.root) and all(
+            l_expr == r_expr for l_expr, r_expr in zip(l_join_exprs, r_join_exprs)
+        ):


Seems we could reduce some nesting if we did the inverse of this and exit early. go/tott/651

tswast

Thanks!

product-auto-label bot added size: l Pull request size is large. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Mar 13, 2024

TrevorBergeron force-pushed the join_rewrite branch 2 times, most recently from 3d6becb to 5bd7a36 Compare March 13, 2024 22:40

refactor: Reimplement implicit joiner at expression layer

415b6c0

TrevorBergeron force-pushed the join_rewrite branch from 5bd7a36 to 415b6c0 Compare March 13, 2024 22:44

TrevorBergeron marked this pull request as ready for review March 13, 2024 22:46

TrevorBergeron requested review from a team as code owners March 13, 2024 22:46

TrevorBergeron requested review from stevewalker-de and tswast and removed request for stevewalker-de March 13, 2024 22:46

TrevorBergeron and others added 3 commits March 14, 2024 09:26

Merge branch 'main' into join_rewrite

84685ca

prevent rewriting of cross joins

21a6472

Merge branch 'main' into join_rewrite

e796b26

tswast requested changes Mar 15, 2024

View reviewed changes

clean up and docstrings

4b81758

TrevorBergeron requested a review from tswast March 15, 2024 23:43

tswast approved these changes Mar 18, 2024

View reviewed changes

Merge branch 'main' into join_rewrite

e63a517

tswast added the automerge Merge the pull request once unit tests and other checks pass. label Mar 18, 2024

TrevorBergeron added 2 commits March 18, 2024 13:34

Merge branch 'main' into join_rewrite

509d8a3

Merge branch 'main' into join_rewrite

1f5d551

gcf-merge-on-green bot merged commit 6611c28 into main Mar 19, 2024

gcf-merge-on-green bot deleted the join_rewrite branch March 19, 2024 01:50

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Mar 19, 2024



		@dataclasses.dataclass
		class NormalizedSelect:

refactor: Reimplement implicit joiner at expression layer #436

refactor: Reimplement implicit joiner at expression layer #436

Uh oh!

Conversation

TrevorBergeron commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!