Skip to content

Remove filters from LookupJoins when they're provably not required. #1888

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Oct 18, 2023
Merged

Remove filters from LookupJoins when they're provably not required. #1888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3b54dbe
Fix bug: A lookup with a non-null key for a NullSafeEq inadvertantly …
nicktobey Jul 21, 2023
50ce792
Remove filters from LookupJoin if we can prove that candidate rows wi…
nicktobey Jul 21, 2023
48e8777
Recost LookUp joins: We assumed that an index which used every column…
nicktobey Jul 21, 2023
2b384a3
Update Query Plan Tests.
nicktobey Jul 21, 2023
55b6b05
Merge branch 'main' of github.com:dolthub/go-mysql-server into nickto…
nicktobey Sep 11, 2023
d4fba66
Update query plans after merge.
nicktobey Sep 11, 2023
e26df6c
Add list of key expressions to IndexedTableAccess's debug string.
nicktobey Sep 11, 2023
c0030d7
Remove noisy do-nothing filters from described plans.
nicktobey Sep 11, 2023
6fa77f6
Match scalar expr ids instead of scalars.
nicktobey Sep 11, 2023
52fdad0
Match scalar expr ids instead of scalars.
nicktobey Sep 11, 2023
f7f0426
Revert "Match scalar expr ids instead of scalars."
nicktobey Sep 20, 2023
8063b37
Revert "Match scalar expr ids instead of scalars."
nicktobey Sep 20, 2023
1c849f6
Merge branch 'main' into nicktobey/remove-filters
nicktobey Sep 29, 2023
15654ac
Merge remote-tracking branch 'origin' into nicktobey/remove-filters
nicktobey Oct 17, 2023
d2a5654
Update query plans to include keys for indexed table accesses and rem…
nicktobey Oct 18, 2023
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
84 changes: 21 additions & 63 deletions enginetest/queries/imdb_plans.go
View file
Open in desktop

Large diffs are not rendered by default.

315 changes: 159 additions & 156 deletions enginetest/queries/integration_plans.go
View file
Open in desktop

Large diffs are not rendered by default.

505 changes: 166 additions & 339 deletions enginetest/queries/query_plans.go
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions enginetest/queries/tpcc_plans.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

270 changes: 97 additions & 173 deletions enginetest/queries/tpch_plans.go
View file
Open in desktop

Large diffs are not rendered by default.

38 changes: 24 additions & 14 deletions sql/analyzer/indexed_joins.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func addLookupJoins(m *memo.Memo) error {
for _, on := range conds {
filters := memo.SplitConjunction(on)
for _, idx := range indexes {
keyExprs, nullmask := keyExprsForIndex(tableGrp, idx.Cols(), append(filters, extraFilters...))
keyExprs, _, nullmask := keyExprsForIndex(tableGrp, idx.Cols(), append(filters, extraFilters...))
if keyExprs != nil {
concat = append(concat, &memo.Lookup{
Index: idx,
Expand All @@ -223,7 +223,7 @@ func addLookupJoins(m *memo.Memo) error {
}

for _, idx := range indexes {
keyExprs, nullmask := keyExprsForIndex(tableGrp, idx.Cols(), append(join.Filter, extraFilters...))
keyExprs, matchedFilters, nullmask := keyExprsForIndex(tableGrp, idx.Cols(), append(join.Filter, extraFilters...))
if keyExprs == nil {
continue
}
Expand All @@ -235,6 +235,19 @@ func addLookupJoins(m *memo.Memo) error {
Nullmask: nullmask,
},
}
var filters []memo.ScalarExpr
for _, filter := range rel.Filter {
found := false
for _, matchedFilter := range matchedFilters {
if filter == matchedFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

you might be able to just match with e.ExprId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! That's not only more efficient, but that allows us to detect matches through table aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching to use e.ExprId() appears to have broken some correctness tests. I'm going to merge the previous version.

found = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just do

filters = append(filters, filter)
break

and get rid of the found flag

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, could make matchedFilters a map, and avoid a nested for loop. Doubtful this will really make a difference in runtime.

Copy link
Contributor Author

@nicktobey nicktobey Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The purpose behind the loop is to compute the set difference: we want to fill filters with every filter from rel.Filter that isn't also in matchedFilters. Obnoxiously there's no way to do this in the standard library that I could find.

Something with a map would be asymptotically faster, but a nested loop is likely faster for all real uses cases. (You'd need a lot of filters in your join before the overhead of maintaining a map becomes worth it.)

}
}
if !found {
filters = append(filters, filter)
}
}
rel.Filter = filters
rel.Op = rel.Op.AsLookup()
rel.Lookup.Parent = rel.JoinBase
e.Group().Prepend(rel)
Expand All @@ -246,30 +259,28 @@ func addLookupJoins(m *memo.Memo) error {
// keyExprsForIndex returns a list of expression groups that compute a lookup
// key into the given index. The key fields will either be equality filters
// (from ON conditions) or constants.
func keyExprsForIndex(tableGrp memo.GroupId, idxExprs []sql.ColumnId, filters []memo.ScalarExpr) ([]memo.ScalarExpr, []bool) {
var keyExprs []memo.ScalarExpr
var nullmask []bool
func keyExprsForIndex(tableGrp memo.GroupId, idxExprs []sql.ColumnId, filters []memo.ScalarExpr) (keyExprs, matchedFilters []memo.ScalarExpr, nullmask []bool) {
for _, col := range idxExprs {
key, nullable := keyForExpr(col, tableGrp, filters)
key, filter, nullable := keyForExpr(col, tableGrp, filters)
if key == nil {
break
}
keyExprs = append(keyExprs, key)
matchedFilters = append(matchedFilters, filter)
nullmask = append(nullmask, nullable)
}
if len(keyExprs) == 0 {
return nil, nil
return nil, nil, nil
}
return keyExprs, nullmask
return keyExprs, matchedFilters, nullmask
}

// keyForExpr returns an equivalence or constant value to satisfy the
// lookup index expression.
func keyForExpr(targetCol sql.ColumnId, tableGrp memo.GroupId, filters []memo.ScalarExpr) (memo.ScalarExpr, bool) {
func keyForExpr(targetCol sql.ColumnId, tableGrp memo.GroupId, filters []memo.ScalarExpr) (key memo.ScalarExpr, filter memo.ScalarExpr, nullable bool) {
for _, f := range filters {
var left memo.ScalarExpr
var right memo.ScalarExpr
var nullable bool
switch e := f.Group().Scalar.(type) {
case *memo.Equal:
left = e.Left.Scalar
Expand All @@ -280,7 +291,6 @@ func keyForExpr(targetCol sql.ColumnId, tableGrp memo.GroupId, filters []memo.Sc
right = e.Right.Scalar
default:
}
var key memo.ScalarExpr
if ref, ok := left.(*memo.ColRef); ok && ref.Col == targetCol {
key = right
} else if ref, ok := right.(*memo.ColRef); ok && ref.Col == targetCol {
Expand All @@ -291,10 +301,10 @@ func keyForExpr(targetCol sql.ColumnId, tableGrp memo.GroupId, filters []memo.Sc
// expression key can be arbitrarily complex (or simple), but cannot
// reference the lookup table
if !key.Group().ScalarProps().Tables.Contains(int(memo.TableIdForSource(tableGrp))) {
return key, nullable
return key, f, nullable
}
}
return nil, false
return nil, nil, false
}

// convertSemiToInnerJoin adds inner join alternatives for semi joins.
Expand Down Expand Up @@ -475,7 +485,7 @@ func addRightSemiJoins(m *memo.Memo) error {
continue
}

keyExprs, nullmask := keyExprsForIndex(tableGrp, idx.Cols(), append(semi.Filter, filters...))
keyExprs, _, nullmask := keyExprsForIndex(tableGrp, idx.Cols(), append(semi.Filter, filters...))
if keyExprs == nil {
continue
}
Expand Down
3 changes: 3 additions & 0 deletions sql/memo/exec_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,9 @@ func (b *ExecBuilder) buildScalar(e ScalarExpr, sch sql.Schema) (sql.Expression,

func (b *ExecBuilder) buildFilterConjunction(scope *plan.Scope, s sql.Schema, filters ...ScalarExpr) (sql.Expression, error) {
var ret sql.Expression
if len(filters) == 0 {
return expression.NewLiteral(true, types.Boolean), nil
}
for i := range filters {
filter, err := b.buildScalar(filters[i], s)
if err != nil {
Expand Down
8 changes: 8 additions & 0 deletions sql/plan/indexed_table_access.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,14 @@ func (i *IndexedTableAccess) DebugString() string {
if i.lookup.IsReverse {
children = append(children, fmt.Sprintf("reverse: %v", i.lookup.IsReverse))
}
} else {
var filters []string
for _, e := range i.lb.keyExprs {
filters = append(filters, e.String())
}
if len(filters) > 0 {
children = append(children, fmt.Sprintf(fmt.Sprintf("keys: %v", filters)))
}
}

// TableWrappers may want to print their own debug info
Expand Down
36 changes: 22 additions & 14 deletions sql/plan/join.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,14 +433,18 @@ func (j *JoinNode) String() string {
pr := sql.NewTreePrinter()
var children []string
if j.Filter != nil {
if j.Op.IsMerge() {
filters := expression.SplitConjunction(j.Filter)
children = append(children, fmt.Sprintf("cmp: %s", filters[0]))
if len(filters) > 1 {
children = append(children, fmt.Sprintf("sel: %s", expression.JoinAnd(filters[1:]...)))
// Don't print a filter that's always true, it's just noise.
literal, isLiteral := j.Filter.(*expression.Literal)
if !isLiteral || literal.Value() != true {
if j.Op.IsMerge() {
filters := expression.SplitConjunction(j.Filter)
children = append(children, fmt.Sprintf("cmp: %s", filters[0]))
if len(filters) > 1 {
children = append(children, fmt.Sprintf("sel: %s", expression.JoinAnd(filters[1:]...)))
}
} else {
children = append(children, j.Filter.String())
}
} else {
children = append(children, j.Filter.String())
}
}
children = append(children, j.left.String(), j.right.String())
Expand All @@ -453,14 +457,18 @@ func (j *JoinNode) DebugString() string {
pr := sql.NewTreePrinter()
var children []string
if j.Filter != nil {
if j.Op.IsMerge() {
filters := expression.SplitConjunction(j.Filter)
children = append(children, fmt.Sprintf("cmp: %s", sql.DebugString(filters[0])))
if len(filters) > 1 {
children = append(children, fmt.Sprintf("sel: %s", sql.DebugString(expression.JoinAnd(filters[1:]...))))
// Don't print a filter that's always true, it's just noise.
literal, isLiteral := j.Filter.(*expression.Literal)
if !isLiteral || literal.Value() != true {
if j.Op.IsMerge() {
filters := expression.SplitConjunction(j.Filter)
children = append(children, fmt.Sprintf("cmp: %s", sql.DebugString(filters[0])))
if len(filters) > 1 {
children = append(children, fmt.Sprintf("sel: %s", sql.DebugString(expression.JoinAnd(filters[1:]...))))
}
} else {
children = append(children, sql.DebugString(j.Filter))
}
} else {
children = append(children, sql.DebugString(j.Filter))
}
}
children = append(children, sql.DebugString(j.left), sql.DebugString(j.right))
Expand Down