-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support complex aggregation in Gen4's Operators #13326
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,7 +112,7 @@ func createProjectionFromSelect(ctx *plancontext.PlanningContext, horizon horizo | |
return nil, err | ||
} | ||
|
||
aggregations, err := qp.AggregationExpressions(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can call this as |
||
aggregations, complexAggr, err := qp.AggregationExpressions(ctx, true) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -135,6 +135,13 @@ func createProjectionFromSelect(ctx *plancontext.PlanningContext, horizon horizo | |
a.Alias = derived.Alias | ||
} | ||
|
||
if complexAggr { | ||
return createProjectionForComplexAggregation(a, qp) | ||
} | ||
return createProjectionForSimpleAggregation(ctx, a, qp) | ||
} | ||
|
||
func createProjectionForSimpleAggregation(ctx *plancontext.PlanningContext, a *Aggregator, qp *QueryProjection) (ops.Operator, error) { | ||
outer: | ||
for colIdx, expr := range qp.SelectExprs { | ||
ae, err := expr.GetAliasedExpr() | ||
|
@@ -165,10 +172,35 @@ outer: | |
} | ||
return nil, vterrors.VT13001(fmt.Sprintf("Could not find the %s in aggregation in the original query", sqlparser.String(ae))) | ||
} | ||
|
||
return a, nil | ||
} | ||
|
||
func createProjectionForComplexAggregation(a *Aggregator, qp *QueryProjection) (ops.Operator, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add comments as how this is built |
||
p := &Projection{ | ||
Source: a, | ||
Alias: a.Alias, | ||
TableID: a.TableID, | ||
} | ||
|
||
for _, expr := range qp.SelectExprs { | ||
ae, err := expr.GetAliasedExpr() | ||
if err != nil { | ||
return nil, err | ||
} | ||
p.Columns = append(p.Columns, ae) | ||
p.Projections = append(p.Projections, UnexploredExpression{E: ae.Expr}) | ||
} | ||
for i, by := range a.Grouping { | ||
a.Grouping[i].ColOffset = len(a.Columns) | ||
a.Columns = append(a.Columns, aeWrap(by.SimplifiedExpr)) | ||
} | ||
for i, aggregation := range a.Aggregations { | ||
a.Aggregations[i].ColOffset = len(a.Columns) | ||
a.Columns = append(a.Columns, aggregation.Original) | ||
} | ||
return p, nil | ||
} | ||
|
||
func createProjectionWithoutAggr(qp *QueryProjection, src ops.Operator) (*Projection, error) { | ||
proj := &Projection{ | ||
Source: src, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,8 +91,11 @@ type ( | |
OriginalOpCode opcode.AggregateOpcode | ||
|
||
Alias string | ||
|
||
// The index at which the user expects to see this aggregated function. Set to nil, if the user does not ask for it | ||
Index *int | ||
// Only used in the old Horizon Planner | ||
Index *int | ||
|
||
Distinct bool | ||
|
||
// the offsets point to columns on the same aggregator | ||
|
@@ -444,13 +447,9 @@ func checkForInvalidAggregations(exp *sqlparser.AliasedExpr) error { | |
}, exp.Expr) | ||
} | ||
|
||
func (qp *QueryProjection) isExprInGroupByExprs(ctx *plancontext.PlanningContext, expr SelectExpr) bool { | ||
func (qp *QueryProjection) isExprInGroupByExprs(ctx *plancontext.PlanningContext, expr sqlparser.Expr) bool { | ||
for _, groupByExpr := range qp.groupByExprs { | ||
exp, err := expr.GetExpr() | ||
if err != nil { | ||
return false | ||
} | ||
if ctx.SemTable.EqualsExprWithDeps(groupByExpr.SimplifiedExpr, exp) { | ||
if ctx.SemTable.EqualsExprWithDeps(groupByExpr.SimplifiedExpr, expr) { | ||
return true | ||
} | ||
} | ||
|
@@ -623,7 +622,7 @@ func (qp *QueryProjection) NeedsDistinct() bool { | |
return true | ||
} | ||
|
||
func (qp *QueryProjection) AggregationExpressions(ctx *plancontext.PlanningContext) (out []Aggr, err error) { | ||
func (qp *QueryProjection) AggregationExpressions(ctx *plancontext.PlanningContext, allowComplexExpression bool) (out []Aggr, complex bool, err error) { | ||
orderBy: | ||
for _, orderExpr := range qp.OrderExprs { | ||
orderExpr := orderExpr.SimplifiedExpr | ||
|
@@ -649,27 +648,54 @@ orderBy: | |
for idx, expr := range qp.SelectExprs { | ||
aliasedExpr, err := expr.GetAliasedExpr() | ||
if err != nil { | ||
return nil, err | ||
return nil, false, err | ||
} | ||
|
||
idxCopy := idx | ||
|
||
if !sqlparser.ContainsAggregation(expr.Col) { | ||
if !qp.isExprInGroupByExprs(ctx, expr) { | ||
getExpr, err := expr.GetExpr() | ||
if err != nil { | ||
return nil, false, err | ||
} | ||
if !qp.isExprInGroupByExprs(ctx, getExpr) { | ||
aggr := NewAggr(opcode.AggregateRandom, nil, aliasedExpr, aliasedExpr.ColumnName()) | ||
aggr.Index = &idxCopy | ||
out = append(out, aggr) | ||
} | ||
continue | ||
} | ||
fnc, isAggregate := aliasedExpr.Expr.(sqlparser.AggrFunc) | ||
if !isAggregate { | ||
return nil, vterrors.VT12001("in scatter query: complex aggregate expression") | ||
_, isAggregate := aliasedExpr.Expr.(sqlparser.AggrFunc) | ||
if !isAggregate && !allowComplexExpression { | ||
return nil, false, vterrors.VT12001("in scatter query: complex aggregate expression") | ||
} | ||
|
||
aggr := createAggrFromAggrFunc(fnc, aliasedExpr) | ||
aggr.Index = &idxCopy | ||
out = append(out, aggr) | ||
sqlparser.CopyOnRewrite(aliasedExpr.Expr, func(node, parent sqlparser.SQLNode) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add some comments here. |
||
ex, isExpr := node.(sqlparser.Expr) | ||
if !isExpr { | ||
return true | ||
} | ||
if aggr, isAggr := node.(sqlparser.AggrFunc); isAggr { | ||
ae := aeWrap(aggr) | ||
if aggr == aliasedExpr.Expr { | ||
ae = aliasedExpr | ||
} | ||
aggrFunc := createAggrFromAggrFunc(aggr, ae) | ||
aggrFunc.Index = &idxCopy | ||
out = append(out, aggrFunc) | ||
return false | ||
} | ||
if sqlparser.ContainsAggregation(node) { | ||
complex = true | ||
return true | ||
} | ||
if !qp.isExprInGroupByExprs(ctx, ex) { | ||
aggr := NewAggr(opcode.AggregateRandom, nil, aeWrap(ex), "") | ||
aggr.Index = &idxCopy | ||
out = append(out, aggr) | ||
} | ||
return false | ||
}, nil, nil) | ||
} | ||
return | ||
} | ||
|
@@ -683,9 +709,7 @@ func createAggrFromAggrFunc(fnc sqlparser.AggrFunc, aliasedExpr *sqlparser.Alias | |
} | ||
} | ||
|
||
aggrF, _ := aliasedExpr.Expr.(sqlparser.AggrFunc) | ||
|
||
if aggrF.IsDistinct() { | ||
if fnc.IsDistinct() { | ||
switch code { | ||
case opcode.AggregateCount: | ||
code = opcode.AggregateCountDistinct | ||
|
@@ -694,8 +718,8 @@ func createAggrFromAggrFunc(fnc sqlparser.AggrFunc, aliasedExpr *sqlparser.Alias | |
} | ||
} | ||
|
||
aggr := NewAggr(code, aggrF, aliasedExpr, aliasedExpr.ColumnName()) | ||
aggr.Distinct = aggrF.IsDistinct() | ||
aggr := NewAggr(code, fnc, aliasedExpr, aliasedExpr.ColumnName()) | ||
aggr.Distinct = fnc.IsDistinct() | ||
return aggr | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frouioui do we need to check the version of vtgate so these are not run on the upgrade/downgrade testing?