Fix panic by rewriting how we move filter conditions up. #1570
Conversation
| return transform.NodeWithCtx(node, func(transform.Context) bool { return true }, func(c transform.Context) (sql.Node, transform.TreeIdentity, error) { | ||
| if c.Node != topJoin { | ||
| return c.Node, transform.SameTree, nil | ||
| resultNode, resultIdentity, err := transform.Node(node, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) { |
There was a problem hiding this comment.
this creates a CROSS_JOIN at the tree root in testing for me. i suspect the filter would be dropped with the new change
> explain select * from xy a cross join xy b on a.x = 2;
+-------------------------------------+
| plan |
+-------------------------------------+
| CrossJoin |
| ├─ Filter |
| │ ├─ (a.x = 2) |
| │ └─ TableAlias(a) |
| │ └─ IndexedTableAccess(xy) |
| │ ├─ index: [xy.x] |
| │ ├─ filters: [{[2, 2]}] |
| │ └─ columns: [x y] |
| └─ Exchange |
| └─ TableAlias(b) |
| └─ Table |
| ├─ name: xy |
| └─ columns: [x y] |
+-------------------------------------+There was a problem hiding this comment.
you could check if node == topJoin before doing the tree search for that case
There was a problem hiding this comment.
you could also git checkout main -- sql/analyzer/optimization_rules_test.go in that case also
There was a problem hiding this comment.
If I add the node == topJoin test, we still get the panic from populateSubgraph, even though this seems like the correct action. Investigating this in more detail, we might be trying to fix the wrong issue.
There was a problem hiding this comment.
you could also
git checkout main -- sql/analyzer/optimization_rules_test.goin that case also
Sorry, not following yet, what does this do?
There was a problem hiding this comment.
if topJoin can be the tree root, the changes the unit tests are unnecessary
There was a problem hiding this comment.
you could check if node == topJoin before doing the tree search for that case
This causes a break in a different portion of the code. I suspect one of the downstream rules makes certain assumption about the query plan, and the node == topJoin approach violates one of those assumption. As discussed offline a bit, I'm keeping the PR as-is and we'll just use this approach of updating the topJoin parent, instead of updating the topJoin itself.
There was a problem hiding this comment.
My concerns are correctness related, I don't mind how it happens but CROSS_JOIN root nodes can't lose filters.
There was a problem hiding this comment.
My concerns are correctness related, I don't mind how it happens but CROSS_JOIN root nodes can't lose filters.
Absolutely makes sense.
Here's the result of the explain select... you mentioned earlier, with this fix in place:
+-------------------------------------+
| plan |
+-------------------------------------+
| CrossJoin |
| ├─ Filter |
| │ ├─ (a.x = 2) |
| │ └─ TableAlias(a) |
| │ └─ IndexedTableAccess(xy) |
| │ ├─ index: [xy.x] |
| │ ├─ filters: [{[2, 2]}] |
| │ └─ columns: [x y] |
| └─ Exchange |
| └─ TableAlias(b) |
| └─ Table |
| ├─ name: xy |
| └─ columns: [x y] |
+-------------------------------------+
The filter isn't being dropped. I can add this particular join query to our tests as well.
sql/analyzer/optimization_rules.go
Outdated
| return n, transform.SameTree, nil | ||
| } | ||
|
|
||
| switch imp := n.(type) { |
There was a problem hiding this comment.
| switch imp := n.(type) { | |
| switch n := n.(type) { |
There was a problem hiding this comment.
Feels really strange to reassign n like this, but the scoping seems to work out, so going with this approach.
sql/analyzer/optimization_rules.go
Outdated
| return plan.NewFilter( | ||
| expression.JoinAnd(append([]sql.Expression{n.Expression}, nonJoinFilters...)...), | ||
| n.Child), transform.NewTree, nil | ||
| newExpression := expression.JoinAnd(append([]sql.Expression{imp.Expression}, nonJoinFilters...)...) |
There was a problem hiding this comment.
this might be a bit shorter
| newExpression := expression.JoinAnd(append([]sql.Expression{imp.Expression}, nonJoinFilters...)...) | |
| newExpression := expression.JoinAnd(append(nonJoinFilters, n.Expression)...) |
There was a problem hiding this comment.
Shorter, and we're also not expanding nonJoinFilters unnecessarily. Changing.
| } | ||
| }) | ||
|
|
||
| return resultNode, resultIdentity, err |
There was a problem hiding this comment.
nit: you don't use these vars, maybe return the transform result
There was a problem hiding this comment.
Yup, this is testing/debug code, removing.
There was a problem hiding this comment.
These are coming back. I'm adding a check after the transform.Node call to ensure that nonJoinFilters (which we've removed from the tree) are actually used and are properly added back to the tree. Wouldn't want to just remove these and drop them on the floor.
- renamed argument in switch - simplified joining of expression to nonJoinFilters - added a panic that gets called if the removed nonJoinFilters are not re-added to the tree
sql/analyzer/optimization_rules.go
Outdated
| } | ||
| }) | ||
|
|
||
| if len(nonJoinFilters) > 0 { | ||
| panic("nonJoinFilters should have been used and cleared out") |
There was a problem hiding this comment.
In the event we forget to apply filters, we should maybe return an error rather than panicking. I have not always been consistent about this in the past. We try to organize errors into sql/errors.go but we are also inconsistent about that in practice.
There was a problem hiding this comment.
Sure thing, will add an entry in sql/errors.go and will return it here.
- add an explicit `node == topJoin` check to handle the case that there is nothing that contains topJoin (if it's literally the top of the tree) - reverted all optimization_rules_test.go changes, which are specifically testing the case where the join is the top of the tree - replaced `panic` with an error
Fix panic by rewriting how we move filter conditions up.
Fixes dolthub/dolt#5214