hashjoin indexing #1728
hashjoin indexing #1728
Conversation
There was a problem hiding this comment.
The PR summary is really good. The fixes are painful but maybe the only way to do this short of rewriting indexing. A comment pointing out how this fixes one layer of bugs from a larger general class of problems would be good. And then one more pass making sure we only target a specific edge case and are not causing collateral damage to currently working queries would be good. @zachmu this could use a pass, this is mostly my design and I hate it but I don't know a better way short of rewriting indexing.
| var joinParent *plan.JoinNode | ||
| var selFunc transform.SelectorFunc = func(c transform.Context) bool { | ||
| if jp, ok := c.Node.(*plan.JoinNode); ok { | ||
| joinParent = jp | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
this seems like it could have unpredictable effects. any reason why c.Parent.(*plan.JoinNode) below does not work?
sql/analyzer/resolve_subqueries.go
Outdated
| var newSqa sql.Node | ||
| var same2 transform.TreeIdentity | ||
| var err error | ||
| if sqa.OuterScopeVisibility && joinParent != nil && !joinParent.Op.IsLeftOuter() { |
There was a problem hiding this comment.
!joinParent.Op.IsLeftOuter() seems suspicious, why would any of the indexing be dependent on the parent being a left join? We should probably add a test for left joins.
There was a problem hiding this comment.
Similar to above comment, I think we should make sure sqa is the second child of the parent join. None of this generalizes, there are still holes in the logic, but we have some control over not making new bugs. Test to make sure we don't screw up the same case but with subquery alias on the left would be helpful.
There was a problem hiding this comment.
The joinParent.Op.IsLeftOuter() check might've been added to prevent breaking other queries in a previous iteration, but it looks like it isn't needed now. I'll remove it.
| var err error | ||
| if sqa.OuterScopeVisibility && joinParent != nil && !joinParent.Op.IsLeftOuter() { | ||
| if stripChild, ok := joinParent.Right().(*plan.StripRowNode); ok && stripChild.Child == sqa { | ||
| subScope := scope.newScopeInJoin(joinParent.Children()[0]) |
There was a problem hiding this comment.
We should add a comment that this only fixes 1-degree of join nesting, and that a proper fix would re-index the entire tree at the end of analysis. If the subquery has more than 1 sibling, or a sibling higher in the join tree, we currently do not detect it. We probably don't need tests for those edge cases yet. Just gotta hope no one hits them soon.
| @@ -7522,6 +7522,37 @@ SELECT * FROM my_cte;`, | |||
| {2}, | |||
| }, | |||
| }, | |||
| { | |||
| Query: ` | |||
There was a problem hiding this comment.
a comment tagging the original issue might be worthwhile
sql/analyzer/exec_builder.go
Outdated
| @@ -180,10 +180,18 @@ func (b *ExecBuilder) buildHashJoin(j *hashJoin, input sql.Schema, children ...s | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| oldInJoin := false | |||
| if j.g.m.scope != nil { | |||
There was a problem hiding this comment.
Rather than temporarily monkeying with the scope like this, why not pass a modified copy directly to this function?
sql/analyzer/resolve_subqueries.go
Outdated
|
|
||
| joinlessScopeLen := scopeLen | ||
| if scope.inJoin { | ||
| scope.SetJoin(false) |
There was a problem hiding this comment.
Same comment. Use these as values to compute things you need, rather than temporarily modifying
sql/analyzer/exec_builder.go
Outdated
| outerAttrs, err := b.buildFilters(j.g.m.scope, j.right.relProps.OutputCols(), expression.Tuple(j.outerAttrs)) | ||
| tmpScope := j.g.m.scope | ||
| if tmpScope != nil { | ||
| tmpScope = tmpScope.newScopeInJoin(nil) |
There was a problem hiding this comment.
could we have slightly more expressive methods? like j.g.m.scope.newScopeNoJoin()?
When we have plan of the following format:
The indexes we assign to GetFields during analysis don't align with the indexes of the actual columns in each row during execution time. This is a result of StripNode, PrependNode, and the nested Joins with SubqueryAlias.
This error wasn't caught sooner as the incorrect indexes are too low, so they never threw
IndexOutOfBoundserrors and just returned potentially incorrect results instead.The fix was to correct these indexes at analysis time.
Firstly,
SubqueryAliasnodes withOuterScopeVisibilty = trueinside joins need to see the left sibling node (in addition to the parent nodes). SoScopewas modified to include some new fields, specifically for sibling nodes. Additionally, the filefinalizeSubquerywas changed to track the parent as well, so we could detect when we're analyzing aSubqueryAliason the right side of a join, and add the left child to the scope.Additionally, pushdownFilters was modified to not undo all the changes to the Analyzer for HashLookups.
At runtime, the
PrependRownodes cache the rows outside theInSubquery, while the buildJoinIter forCrossJoinwould include both the outside and the left row. This meant that some parts of the inner HashJoin would receive extra columns while others didn't. The fix here was to alter thescope.InJoindepending on which parts of HashJoin we were building.Lastly, to have these changes not break for PreparedStatements, we just needed to not redo
finalizeUnionsinpostPrepared, as we don't replan joins inpostPrepared, so we don't know if we're in a join or not, and the correct indexes are set inprePrepared.Along the way, we discovered a query that panics, but the cause is different than the purpose of this fix, and it panicked the same way before these changes, so it is left as a skipped test.
Fix for: dolthub/dolt#5714