opt: fix computation of apply join outer columns#35171
opt: fix computation of apply join outer columns#35171craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
999ea8f to
88bbdf7
Compare
88bbdf7 to
bafd909
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, @justinj, @RaduBerinde, and @rytaft)
pkg/sql/opt/memo/logical_props_builder.go, line 410 at r1 (raw file):
// right input. rightOuterCols := join.Child(1).(RelExpr).Relational().OuterCols boundOuterCols := rightOuterCols.Intersection(join.Child(0).(RelExpr).Relational().OutputCols)
We already did DifferenceWith(inputCols) above which includes all the left child OutputCols.. So maybe we can remove this entire block?
pkg/sql/opt/norm/testdata/rules/reject_nulls, line 620 at r1 (raw file):
# them. exprnorm (Root
[nit] I don't think you need the Root part as long as you don't execbuild it.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status:
pkg/sql/opt/testutils/opttester/opt_tester.go, line 591 at r1 (raw file):
} ot.seenRules.Add(int(ruleName)) return true
This seems like it's doing full exploration -- not just normalization (I think you want to do what OptNorm is doing above).
Previously, we would incorrectly compute the outer columns of an apply-join, because we would not consider that some of the right input's outer columns actually came from higher up in the tree, not from the left input. By fixing this, a new problem was uncovered that required fixing. The following series of events led to an infinite loop of norm rules applying: * An outer join requests null rejection on some set of columns, * that request travels up through an apply join, * it eventually reaches a GroupBy that can provide null rejection via RejectNullsGroupBy, * RejectNullsGroupBy creates a new filter and pushes it down, * this new filter is unable to make it past the apply join that the request traveled through, * RejectNullsGroupBy triggers again, since the null rejection request persists. The fix was to stop null rejection requests if the join doing the propagation wouldn't be able to push down a filter (that is, if the input has outer cols). Unfortunately, we can't (easily) fix this by allowing filters to travel through apply joins (via PushFilterIntoJoinRight), since that then cycles with TryDecorrelateSelect (which is effectively the inverse of that). This also prevents decorrelation of an existing test case, which requires further investigation. Release note: None
bafd909 to
0c183cd
Compare
justinj
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/memo/logical_props_builder.go, line 410 at r1 (raw file):
Previously, RaduBerinde wrote…
We already did
DifferenceWith(inputCols)above which includes all the left child OutputCols.. So maybe we can remove this entire block?
Oh, you're totally right! Deleted it.
pkg/sql/opt/norm/testdata/rules/reject_nulls, line 620 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] I don't think you need the
Rootpart as long as you don't execbuild it.
Done.
pkg/sql/opt/testutils/opttester/opt_tester.go, line 591 at r1 (raw file):
Previously, rytaft wrote…
This seems like it's doing full exploration -- not just normalization (I think you want to do what OptNorm is doing above).
It's not actually, exprgen.Build doesn't run exploration over the built expression. Added a comment explaining.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/sql/opt/testutils/opttester/opt_tester.go, line 591 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
It's not actually,
exprgen.Builddoesn't run exploration over the built expression. Added a comment explaining.
Cool - thanks!
andy-kimball
left a comment
There was a problem hiding this comment.
Seems we need to bring back cycle detection. I removed it thinking the Rule props were a good enough tool to prevent cycles, but this is the second known case where that unable to decorrelate due to not having the cycle detection.
Reviewable status:
|
bors r+ |
35171: opt: fix computation of apply join outer columns r=justinj a=justinj Previously, we would incorrectly compute the outer columns of an apply-join, because we would not consider that some of the right input's outer columns actually came from higher up in the tree, not from the left input. By fixing this, a new problem was uncovered that required fixing. The following series of events led to an infinite loop of norm rules applying: * An outer join requests null rejection on some set of columns, * that request travels up through an apply join, * it eventually reaches a GroupBy that can provide null rejection via RejectNullsGroupBy, * RejectNullsGroupBy creates a new filter and pushes it down, * this new filter is unable to make it past the apply join that the request traveled through, * RejectNullsGroupBy triggers again, since the null rejection request persists. The fix was to stop null rejection requests if the join doing the propagation wouldn't be able to push down a filter (that is, if the input has outer cols). Unfortunately, we can't (easily) fix this by allowing filters to travel through apply joins (via PushFilterIntoJoinRight), since that then cycles with TryDecorrelateSelect (which is effectively the inverse of that). This also prevents decorrelation of an existing test case, which requires further investigation. Release note: None Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
Build succeeded |
Previously, we would incorrectly compute the outer columns of an
apply-join, because we would not consider that some of the right input's
outer columns actually came from higher up in the tree, not from the
left input.
By fixing this, a new problem was uncovered that required fixing. The
following series of events led to an infinite loop of norm rules
applying:
RejectNullsGroupBy,
request traveled through,
persists.
The fix was to stop null rejection requests if the join doing the
propagation wouldn't be able to push down a filter (that is, if the
input has outer cols).
Unfortunately, we can't (easily) fix this by allowing filters to travel
through apply joins (via PushFilterIntoJoinRight), since that then
cycles with TryDecorrelateSelect (which is effectively the inverse of
that).
This also prevents decorrelation of an existing test case, which
requires further investigation.
Release note: None