Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hashjoin indexing #1728

Merged
merged 31 commits into from
Apr 27, 2023
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions enginetest/queries/queries.go
Original file line number Diff line number Diff line change
Expand Up @@ -7522,6 +7522,37 @@ SELECT * FROM my_cte;`,
{2},
},
},
{
Query: `
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a comment tagging the original issue might be worthwhile


SELECT COUNT(*)
FROM keyless
WHERE keyless.c0 IN (

WITH RECURSIVE cte(depth, i, j) AS (
SELECT 0, T1.c0, T1.c1
FROM keyless T1
WHERE T1.c0 = 0

UNION ALL

SELECT cte.depth + 1, cte.i, T2.c1 + 1
FROM cte, keyless T2
WHERE cte.depth = T2.c0
)

SELECT U0.c0
FROM keyless U0, cte
WHERE cte.j = keyless.c0

)
ORDER BY c0;
;`,

Expected: []sql.Row{
{4},
},
},
}

var KeylessQueries = []QueryTest{
Expand Down Expand Up @@ -7946,6 +7977,29 @@ var BrokenQueries = []QueryTest{
Query: "select 2000.0 / 250000000.0 * (24.0 * 6.0 * 6.25 * 10.0);",
Expected: []sql.Row{{"0.0720000000"}},
},
{
// This panics
// The non-recursive part of the UNION ALL returns too many rows, causing index out of bounds errors
// Without the join on mytable and cte, this error is caught
Query: `
WITH RECURSIVE cte(i, j) AS (
SELECT 0, 1, 2
FROM mytable

UNION ALL

SELECT *
FROM mytable, cte
WHERE cte.i = mytable.i
)
SELECT *
FROM mytable;`,
Expected: []sql.Row{
{1, "first row"},
{2, "second row"},
{3, "third row"},
},
},
}

var VersionedQueries = []QueryTest{
Expand Down
78 changes: 78 additions & 0 deletions enginetest/queries/query_plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -7694,6 +7694,84 @@ With c as (
" └─ columns: [i]\n" +
"",
},
{
Query: `
SELECT COUNT(*)
FROM keyless
WHERE keyless.c0 IN (
WITH RECURSIVE cte(depth, i, j) AS (
SELECT 0, T1.c0, T1.c1
FROM keyless T1
WHERE T1.c0 = 0

UNION ALL

SELECT cte.depth + 1, cte.i, T2.c1 + 1
FROM cte, keyless T2
WHERE cte.depth = T2.c0
)

SELECT U0.c0
FROM keyless U0, cte
WHERE cte.j = keyless.c0
);`,
ExpectedPlan: "Project\n" +
" ├─ columns: [COUNT(1):0!null as COUNT(*)]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint))\n" +
" ├─ group: \n" +
" └─ Filter\n" +
" ├─ InSubquery\n" +
" │ ├─ left: keyless.c0:0\n" +
" │ └─ right: Subquery\n" +
" │ ├─ cacheable: false\n" +
" │ └─ Project\n" +
" │ ├─ columns: [U0.c0:2]\n" +
" │ └─ Filter\n" +
" │ ├─ Eq\n" +
" │ │ ├─ cte.j:5\n" +
" │ │ └─ keyless.c0:0\n" +
" │ └─ CrossJoin\n" +
" │ ├─ TableAlias(U0)\n" +
" │ │ └─ Table\n" +
" │ │ ├─ name: keyless\n" +
" │ │ └─ columns: [c0]\n" +
" │ └─ SubqueryAlias\n" +
" │ ├─ name: cte\n" +
" │ ├─ outerVisibility: true\n" +
" │ ├─ cacheable: true\n" +
" │ └─ RecursiveCTE\n" +
" │ └─ Union all\n" +
" │ ├─ Project\n" +
" │ │ ├─ columns: [0 (tinyint), T1.c0:2, T1.c1:3]\n" +
" │ │ └─ Filter\n" +
" │ │ ├─ Eq\n" +
" │ │ │ ├─ T1.c0:2\n" +
" │ │ │ └─ 0 (tinyint)\n" +
" │ │ └─ TableAlias(T1)\n" +
" │ │ └─ Table\n" +
" │ │ ├─ name: keyless\n" +
" │ │ └─ columns: [c0 c1]\n" +
" │ └─ Project\n" +
" │ ├─ columns: [(cte.depth:3!null + 1 (tinyint)), cte.i:4, (T2.c1:7 + 1 (tinyint))]\n" +
" │ └─ HashJoin\n" +
" │ ├─ Eq\n" +
" │ │ ├─ cte.depth:3!null\n" +
" │ │ └─ T2.c0:6\n" +
" │ ├─ RecursiveTable(cte)\n" +
" │ └─ HashLookup\n" +
" │ ├─ source: TUPLE(cte.depth:3!null)\n" +
" │ ├─ target: TUPLE(T2.c0:2)\n" +
" │ └─ CachedResults\n" +
" │ └─ TableAlias(T2)\n" +
" │ └─ Table\n" +
" │ ├─ name: keyless\n" +
" │ └─ columns: [c0 c1]\n" +
" └─ Table\n" +
" ├─ name: keyless\n" +
" └─ columns: [c0 c1]\n" +
"",
},
}

// QueryPlanTODOs are queries where the query planner produces a correct (results) but suboptimal plan.
Expand Down
1 change: 0 additions & 1 deletion sql/analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,6 @@ func postPrepareRuleSelector(id RuleId) bool {
stripTableNameInDefaultsId,
resolvePreparedInsertId,
finalizeSubqueriesId,
finalizeUnionsId,

// DefaultValidationRules
validateResolvedId,
Expand Down
8 changes: 8 additions & 0 deletions sql/analyzer/exec_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than temporarily monkeying with the scope like this, why not pass a modified copy directly to this function?

oldInJoin = j.g.m.scope.inJoin
j.g.m.scope.SetJoin(false)
}
outerAttrs, err := b.buildFilters(j.g.m.scope, j.right.relProps.OutputCols(), expression.Tuple(j.outerAttrs))
if err != nil {
return nil, err
}
if j.g.m.scope != nil {
j.g.m.scope.SetJoin(oldInJoin)
}
filters, err := b.buildFilters(j.g.m.scope, input, j.filter...)
if err != nil {
return nil, err
Expand Down
1 change: 1 addition & 0 deletions sql/analyzer/indexed_joins.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ func inOrderReplanJoin(

// two different base cases, depending on whether we reorder or not
if reorder {
scope.SetJoin(true)
ret, err := replanJoin(ctx, j, a, scope)
if err != nil {
return nil, transform.SameTree, fmt.Errorf("failed to replan join: %w", err)
Expand Down
3 changes: 2 additions & 1 deletion sql/analyzer/pushdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,8 @@ func convertIsNullForIndexes(ctx *sql.Context, e sql.Expression) sql.Expression
// pushdownFixIndices fixes field indices for non-join expressions (replanJoin
// is responsible for join filters and conditions.)
func pushdownFixIndices(a *Analyzer, n sql.Node, scope *Scope) (sql.Node, transform.TreeIdentity, error) {
if _, ok := n.(*plan.JoinNode); ok {
switch n := n.(type) {
case *plan.JoinNode, *plan.HashLookup:
return n, transform.SameTree, nil
}
return FixFieldIndexesForExpressions(a, n, scope)
Expand Down
96 changes: 68 additions & 28 deletions sql/analyzer/resolve_subqueries.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,31 @@ func finalizeSubqueries(ctx *sql.Context, a *Analyzer, n sql.Node, scope *Scope,
// finalizeSubqueriesHelper finalizes all subqueries and subquery expressions,
// fixing parent scopes before recursing into child nodes.
func finalizeSubqueriesHelper(ctx *sql.Context, a *Analyzer, node sql.Node, scope *Scope, sel RuleSelector) (sql.Node, transform.TreeIdentity, error) {
return transform.Node(node, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) {
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
}
Comment on lines +53 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like it could have unpredictable effects. any reason why c.Parent.(*plan.JoinNode) below does not work?


var conFunc transform.CtxFunc = func(c transform.Context) (sql.Node, transform.TreeIdentity, error) {
n := c.Node
if sqa, ok := n.(*plan.SubqueryAlias); ok {
newSqa, same2, err := analyzeSubqueryAlias(ctx, a, sqa, scope, sel, true)
var newSqa sql.Node
var same2 transform.TreeIdentity
var err error
if sqa.OuterScopeVisibility && joinParent != nil && !joinParent.Op.IsLeftOuter() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if stripChild, ok := joinParent.Right().(*plan.StripRowNode); ok && stripChild.Child == sqa {
subScope := scope.newScopeInJoin(joinParent.Children()[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

newSqa, same2, err = analyzeSubqueryAlias(ctx, a, sqa, subScope, sel, true)
} else {
newSqa, same2, err = analyzeSubqueryAlias(ctx, a, sqa, scope, sel, true)
}
} else {
newSqa, same2, err = analyzeSubqueryAlias(ctx, a, sqa, scope, sel, true)
}

if err != nil {
return n, transform.SameTree, err
}
Expand All @@ -68,37 +90,38 @@ func finalizeSubqueriesHelper(ctx *sql.Context, a *Analyzer, node sql.Node, scop
newNode, err = newSqa.WithChildren(newNode)
return newNode, transform.NewTree, err
}
} else {
return transform.OneNodeExprsWithNode(n, func(node sql.Node, e sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
if sq, ok := e.(*plan.Subquery); ok {
newSq, same2, err := analyzeSubqueryExpression(ctx, a, node, sq, scope, sel, true)
if err != nil {
if analyzererrors.ErrValidationResolved.Is(err) {
// if a parent is unresolved, we want to dig deeper to find the unresolved
// child dependency
_, _, err := finalizeSubqueriesHelper(ctx, a, sq.Query, scope.newScopeFromSubqueryExpression(node), sel)
if err != nil {
return e, transform.SameTree, err
}
}
return transform.OneNodeExprsWithNode(n, func(node sql.Node, e sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
if sq, ok := e.(*plan.Subquery); ok {
newSq, same2, err := analyzeSubqueryExpression(ctx, a, node, sq, scope, sel, true)
if err != nil {
if analyzererrors.ErrValidationResolved.Is(err) {
// if a parent is unresolved, we want to dig deeper to find the unresolved
// child dependency
_, _, err := finalizeSubqueriesHelper(ctx, a, sq.Query, scope.newScopeFromSubqueryExpression(node), sel)
if err != nil {
return e, transform.SameTree, err
}
return e, transform.SameTree, err
}
newExpr, same1, err := finalizeSubqueriesHelper(ctx, a, newSq.(*plan.Subquery).Query, scope.newScopeFromSubqueryExpression(node), sel)
if err != nil {
return e, transform.SameTree, err
}
return e, transform.SameTree, err
}
newExpr, same1, err := finalizeSubqueriesHelper(ctx, a, newSq.(*plan.Subquery).Query, scope.newScopeFromSubqueryExpression(node), sel)
if err != nil {
return e, transform.SameTree, err
}

if same1 && same2 {
return e, transform.SameTree, nil
} else {
return newSq.(*plan.Subquery).WithQuery(newExpr), transform.NewTree, nil
}
} else {
if same1 && same2 {
return e, transform.SameTree, nil
} else {
return newSq.(*plan.Subquery).WithQuery(newExpr), transform.NewTree, nil
}
})
}
})
} else {
return e, transform.SameTree, nil
}
})
}

return transform.NodeWithCtx(node, selFunc, conFunc)
}

func resolveSubqueriesHelper(ctx *sql.Context, a *Analyzer, node sql.Node, scope *Scope, sel RuleSelector, finalize bool) (sql.Node, transform.TreeIdentity, error) {
Expand Down Expand Up @@ -420,10 +443,27 @@ func setJoinScopeLen(ctx *sql.Context, a *Analyzer, n sql.Node, scope *Scope, se
if scopeLen == 0 {
return n, transform.SameTree, nil
}

joinlessScopeLen := scopeLen
if scope.inJoin {
scope.SetJoin(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment. Use these as values to compute things you need, rather than temporarily modifying

joinlessScopeLen = len(scope.Schema())
scope.SetJoin(true)
}
return transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) {
if j, ok := n.(*plan.JoinNode); ok {
nj := j.WithScopeLen(scopeLen)
if _, ok := nj.Left().(*plan.StripRowNode); !ok {
if _, ok := nj.Right().(*plan.HashLookup); ok {
nnj, err := nj.WithChildren(
plan.NewStripRowNode(nj.Left(), joinlessScopeLen),
plan.NewStripRowNode(nj.Right(), joinlessScopeLen),
)
if err != nil {
return nil, transform.SameTree, err
}
return nnj, transform.NewTree, nil
}
nj, err := nj.WithChildren(
plan.NewStripRowNode(nj.Left(), scopeLen),
plan.NewStripRowNode(nj.Right(), scopeLen),
Expand Down
5 changes: 5 additions & 0 deletions sql/analyzer/resolve_unions.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,22 @@ func finalizeUnions(ctx *sql.Context, a *Analyzer, n sql.Node, scope *Scope, sel
subqueryCtx, cancelFunc := ctx.NewSubContext()
defer cancelFunc()

scope.SetJoin(false)
// TODO we could detect tree modifications here, skip rebuilding
left, _, err := a.analyzeStartingAtBatch(subqueryCtx, u.Left(), scope, "default-rules", NewFinalizeUnionSel(sel))
if err != nil {
return nil, transform.SameTree, err
}

scope.SetJoin(false)

right, _, err := a.analyzeStartingAtBatch(subqueryCtx, u.Right(), scope, "default-rules", NewFinalizeUnionSel(sel))
if err != nil {
return nil, transform.SameTree, err
}

scope.SetJoin(false)

newn, err := n.WithChildren(StripPassthroughNodes(left), StripPassthroughNodes(right))
if err != nil {
return nil, transform.SameTree, err
Expand Down
Loading