Partially reorder indexing rules; use unique ids rather than execution ids #1984
Conversation
Name binding will tag a scope's subquery metadata when resolving and out of scope reference. Those references are used to perform cacheability checks and subquery decorrelation. Related fixes to indexing, joins, filter rules, and recursive analyzer rule precedence (subquery, trigger, procedure, DML).
max-hoffman
force-pushed
the
max/cache-checks-use-expr-ids
branch
from
0ff3b10 to
61abfb3
Compare
|
For a bit more context on the alias string rewriting, the query that was failing is |
| @@ -300,26 +300,6 @@ WHERE | |||
| " └─ columns: [c_id c_d_id c_w_id c_last c_credit c_discount]\n" + | |||
| "", | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
missing merge from main
| " ├─ static: [{(NULL, ∞)}]\n" + | ||
| " └─ columns: [id dkcaj kng7t tw55n qrqxw ecxaj fgg57 zh72s fsk67 xqdyt tce7a iwv2h hpcms n5cc2 fhcyt etaq7 a75x7]\n" + | ||
| " ├─ columns: [pbmrx.id:4!null as id, pbmrx.TW55N:5!null as TEYBZ, pbmrx.ZH72S:6 as FB6N7]\n" + | ||
| " └─ LookupJoin\n" + |
There was a problem hiding this comment.
Are we concerned that the join types of many of these complex queries are changing?
There was a problem hiding this comment.
yeah, we need statistics
sql/analyzer/hoist_filters.go
Outdated
| } | ||
| ret, same, filters, err := recurseSubqueryForOuterFilters(n, a, scope) | ||
| ret, same, filters, newCorr, err := recurseSubqueryForOuterFilters(n, a, sql.ColSet{}) | ||
| //if !newCorr.Empty() { |
| } | ||
|
|
||
| aggName := strings.ToLower(agg.String()) | ||
| aggName := strings.ToLower(plan.AliasSubqueryString(agg)) |
There was a problem hiding this comment.
Very skeptical this new method is being used in place of expression.String() everywhere it could matter
You have a plan out of this mess?
There was a problem hiding this comment.
expressions are given an expression id, the expression id is used for setting execution indexes
sql/planbuilder/cte.go
Outdated
| // not recursive | ||
| cteScope := b.buildSelectStmt(inScope, union) | ||
| sqScope := inScope.push() | ||
| sqScope.initSubquery() |
There was a problem hiding this comment.
This is probably better expressed as inScope.pushSubquery()
There's basically no use case for calling this except immediatley after construction, so move it to a constructor
There was a problem hiding this comment.
the case is your next comment, i added a method to do both but left the init for cases where we want replace() not push()
sql/planbuilder/cte.go
Outdated
|
|
||
| rightInScope := inScope.replace() | ||
| rightInScope.addCte(name, cteScope) | ||
| rightInScope.initSubquery() |
There was a problem hiding this comment.
Need inscope.replaceSubquery() here?
2 participants
Name binding stores caching information upfront. Rule interdependencies pushed me into fixing a bunch of other rules before tests would pass. All together I think most of the changes are simplifications that I was planning on doing related to the
fixidxrefactor. I was hoping to make it more piecemeal. Hopefully this gets us ~50% of the way towards removing those dependencies.fixidxis mostly contained toreorderJoinsandfixAuxiliaryExpressionsnow, both near the end of analysis. If we move the indexing inreorderJoinsintofixAuxiliaryExpressions, all indexing will happen at the end of analysis. That would let us index complicated joins with subqueries correctly and all queries more reliably.summary:
moveFiltersOutOfJoinConditionsto put filters below join when appropriatefixAuxiliaryExpresssionsat end of analysis