Move index costing into join planning phase #2191
Conversation
max-hoffman
changed the title
max-hoffman
force-pushed
the
max/index-cost-refactor
branch
from
d3f4abd
to
f4ef143
Compare
| return grp | ||
| } | ||
|
|
||
| func (m *Memo) MemoizeConcatLookupJoin(grp, left, right *ExprGroup, op plan.JoinType, filter []sql.Expression, lookups []*IndexScan) *ExprGroup { |
There was a problem hiding this comment.
I'm not clear on what a "Concat Lookup Join" is and why this isn't just "MemoizeLookupJoin". I suspect it's a single node that does multiple lookups on multiple indexes, but I'm not 100% sure. Can you leave a docstring?
sql/memo/memo.go
Outdated
| @@ -213,6 +257,21 @@ func (m *Memo) MemoizeProject(grp, child *ExprGroup, projections []sql.Expressio | |||
| return grp | |||
| } | |||
|
|
|||
| func (m *Memo) MemoizeIta(grp *ExprGroup, ita *plan.IndexedTableAccess, alias string, index *Index) *ExprGroup { | |||
There was a problem hiding this comment.
What does ITA stand for? Can you add a docstring?
sql/memo/memo.go
Outdated
| @@ -297,6 +356,7 @@ func (m *Memo) optimizeMemoGroup(grp *ExprGroup) error { | |||
| n = n.Next() | |||
| } | |||
|
|
|||
| grp.fixEnforcers() | |||
There was a problem hiding this comment.
Why is this call needed? A comment would help.
sql/memo/expr_group.go
Outdated
| func (e *ExprGroup) fixEnforcers() { | ||
| switch n := e.Best.(type) { | ||
| case *MergeJoin: | ||
| // todo: no ITA children that aren't the same index as sorting index |
There was a problem hiding this comment.
These comments are unclear. Can you elaborate?
sql/memo/expr_group.go
Outdated
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| // fixEnforcers edits the children of a new best plan to account |
There was a problem hiding this comment.
This docstring is confusing. Can you maybe give an example?
sql/memo/expr_group.go
Outdated
| } | ||
|
|
||
| // Update best to a DFS path to a tablescan | ||
| func (e *ExprGroup) fixItaConflict() { |
There was a problem hiding this comment.
I understand why you may want this to be a separate method from findTableScanPath, but I think it needs a docstring that explains how this fixes ita conflicts (and maybe what an ita conflict is.)
sql/analyzer/costed_index_scan.go
Outdated
| } | ||
|
|
||
| // create ranges, lookup, ITA for best indexScan | ||
| // TODO pass up FALSE filter information |
sql/analyzer/costed_index_scan.go
Outdated
| var retFilters []sql.Expression | ||
| if !iat.PreciseMatch() { | ||
| // cannot drop any filters | ||
| //itaGrp = m.MemoizeIta(nil, ret, aliasName, idx) |
There was a problem hiding this comment.
Remove this commented out code?
| return l*(cpuCostFactor+randIOCostFactor) - r*seqIOCostFactor - l*seqIOCostFactor, nil | ||
| } | ||
| if l*r*sel < l { | ||
| // 1 - (total rows - covered rows / total rows) |
There was a problem hiding this comment.
Make this comment more clear?
sql/memo/coster.go
Outdated
| if isInjectiveLookup(lookup.Index, n.JoinBase, lookup.Table.Expressions(), lookup.Table.NullMask()) { | ||
| sel = 0 | ||
| } else { | ||
| sel = lookupJoinSelectivity(lookup) * optimisticJoinSel |
There was a problem hiding this comment.
We could probably pull the isInjectiveLookup check (and multiplying by optimisticJoinSel) into lookupJoinSelectivity. This pattern repeats everywhere that lookupJoinSelectivity is called.
Put index costing inside join planning, so that in the future join planning will have better cardinalities (statistics) for join ordering. Most of the changes will look like refactoring the way we expression index lookups in the memo. I attempted to do this in a way that makes as few changes as possible to join planning; the goal here is to set me up for rewriting cardinality checks with stats objects. It didn't go as cleanly as I wanted, I ended up shifting a lot of join plans back to lookup plans because HASH_JOIN was beating LOOKUP_JOIN in several key places.
One downside of the current PR is that it converts a sysbench MERGE_JOIN into a LOOKUP_JOIN. I would prefer fixing this in the next PR when I do a bigger costing overhaul.
Variety of fixes for join hinting, correctness, etc.
At some point we appeared to fix this:
#1893