Refactoring of plan building #7054
Conversation
Introduce Inputs and Rewrite as a way to visit all nodes in the tree, and used them to implement First and PushMisc outside the builders Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
| case *mergeSort, *pulloutSubquery: | ||
| filteredInput, err := planFilter(pb, node.Inputs()[0], filter, whereType, origin) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = node.Rewrite(filteredInput) | ||
| return node, err |
There was a problem hiding this comment.
please add a test to cover this case
go/vt/vtgate/planbuilder/grouping.go
Outdated
| case *join: | ||
| return nil, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: group by on cross-shard join") |
There was a problem hiding this comment.
please add a test to cover this case
go/vt/vtgate/planbuilder/grouping.go
Outdated
| case *mergeSort, *pulloutSubquery: | ||
| inputs := node.Inputs() | ||
| input := inputs[0] | ||
|
|
||
| newInput, err := planDistinct(input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| inputs[0] = newInput | ||
| node.Rewrite(inputs...) | ||
| return node, nil |
There was a problem hiding this comment.
please add a test to cover this case
go/vt/vtgate/planbuilder/grouping.go
Outdated
| case *subquery: | ||
| return nil, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: distinct on cross-shard subquery") | ||
| case *concatenate: | ||
| return newDistinct(node), nil | ||
| case *join: | ||
| return nil, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: distinct on cross-shard join") |
There was a problem hiding this comment.
please add a test to cover these cases
There was a problem hiding this comment.
I think this is better than the old approach from the perspective of understanding how each SQL construct is handled. On the downside, it reduces the understanding of all the functionality that builds a primitive. A perfect code structure is not possible because we essentially have a matrix of primitives and AST constructs, and we have to provide code snippets for each of those.
However, if we have to make a choice, leaning towards grouping functions by the SQL construct (this new approach) feels better. So, I'm good with this.
Once review comments are addressed, you can push this forward.
Signed-off-by: Andres Taylor <andres@planetscale.com>
This is a rebased #7033
Today, in the planbuilding package, we have
builders and aprimitiveBuilder.The primitive builder contains the current
builderwe are working on, the symtab, jointab and a context object that allows us to talk with the vschema during planning.Part of the logic lives in static methods, such as
buildSelectPlanet al, that create the primitive builder and control the flow during planning.The other part of of the logic is spread out amongst the
builders - methods such asPushFilter, PushSelect, MakeDistinctand others.During planning, the current plan is represented by a tree of
builders, that at the end of it all are turned intoengine.Primitives.I think we can design this so it becomes easier to understand and work with:
builderinterface is shock full of Liskov Substitution Principle problems - no implementor implements all interface methods. The structs implement the interface but most methods return an "not implemented" error.builders - the bug is with DISTINCT, so we want to look atMakeDistinct(). Since we have 11 differentbuilders, this means we have to jump around to a lot of files, and it's harder to get an overview of how DISTINCT is solved.PushFilter,MakeDistinct,PushSelectmethods on the builder interface have very different signatures and behaviours - some change the builder in the primitiveBuilder, some return a new builder, and some are just mutating their own state.This refactoring will try to address this by making the following changes:
builderstructs - for example, allPushFilterlogic will instead live infiltering.goin a single method. The methods that will be extracted are:logicalPlanor something along those lines.