GH-4279 FilterOptimizer should not modify the query tree above the node it is currently on #4280
Conversation
…de it is currently on
| public void meet(Filter filter) { | ||
| super.meet(filter); | ||
| if (filter.getParentNode() instanceof Filter && filter.getParentNode().getParentNode() != null) { | ||
| if (filter.getArg() instanceof Filter && filter.getParentNode() != null) { | ||
|
|
||
| Filter parentFilter = (Filter) filter.getParentNode(); | ||
| QueryModelNode grandParent = parentFilter.getParentNode(); | ||
| And merge = new And(filter.getCondition().clone(), parentFilter.getCondition().clone()); | ||
| Filter childFilter = (Filter) filter.getArg(); | ||
| QueryModelNode parent = filter.getParentNode(); | ||
| And merge = new And(childFilter.getCondition().clone(), filter.getCondition().clone()); | ||
|
|
||
| Filter newFilter = new Filter(filter.getArg().clone(), merge); | ||
| grandParent.replaceChildNode(parentFilter, newFilter); | ||
| meet(newFilter); | ||
| Filter newFilter = new Filter(childFilter.getArg().clone(), merge); | ||
| parent.replaceChildNode(filter, newFilter); | ||
| } | ||
| } |
There was a problem hiding this comment.
@pulquero your solution works well. I wasn't able to create a test that reproduces the issue. I found that when I removed the line with meet(newFilter); then I got some failing tests. Your solution made those tests pass again.
I think that meet(newFilter); was masking the issue. We would first recurse down to the innermost filter, then optimise it by swapping out the parent node. When returning back up we would visit the old parent node and if that node also needed to be optimised we would actually end up optimising the child nodes again.
There was a problem hiding this comment.
Test case i think would be join, filter, filter, filter, sp for left arg and sp for right arg. Or the other way round. At the bottom it'll swap join with filter, but as it unwinds a 2nd time, the local filter var will be the old one it had on the way down, not the new one that it has been replaced by.
GitHub issue resolved: #4279
Briefly describe the changes proposed in this PR:
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resourcesto format from the command line)