Skip to content

fix rollup with having #21834

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1,501 changes: 769 additions & 732 deletions pkg/pb/plan/plan.pb.go

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions pkg/sql/parsers/tree/select.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,9 @@ func (node *SelectClause) GetQueryType() string { return QueryTypeDQL }

// WHERE or HAVING clause.
type Where struct {
Type string
Expr Expr
Type string
RollupHaving bool
Expr Expr
}

func (node *Where) Format(ctx *FmtCtx) {
Expand Down
13 changes: 10 additions & 3 deletions pkg/sql/plan/pushdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,20 @@ func (builder *QueryBuilder) pushdownFilters(nodeID int32, filters []*plan.Expr,

case plan.Node_FILTER:
canPushdown = filters
for _, filter := range node.FilterList {
canPushdown = append(canPushdown, splitPlanConjunction(applyDistributivity(builder.GetContext(), filter))...)
if !node.RollupFilter {
for _, filter := range node.FilterList {
canPushdown = append(canPushdown, splitPlanConjunction(applyDistributivity(builder.GetContext(), filter))...)
}
}

childID, cantPushdownChild := builder.pushdownFilters(node.Children[0], canPushdown, separateNonEquiConds)

if len(cantPushdownChild) > 0 {
if node.RollupFilter {
if len(cantPushdownChild) > 0 {
node.Children[0] = childID
node.FilterList = append(node.FilterList, cantPushdownChild...)
}
} else if len(cantPushdownChild) > 0 {
node.Children[0] = childID
node.FilterList = cantPushdownChild
} else {
Expand Down
17 changes: 13 additions & 4 deletions pkg/sql/plan/query_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -2638,6 +2638,7 @@ func (builder *QueryBuilder) bindSelect(stmt *tree.Select, ctx *BindContext, isR
var notCacheable bool
var helpFunc *helpFunc
var boundTimeWindowGroupBy *plan.Expr
var rollupFilter bool

astOrderBy := stmt.OrderBy
astLimit := stmt.Limit
Expand Down Expand Up @@ -2731,6 +2732,9 @@ func (builder *QueryBuilder) bindSelect(stmt *tree.Select, ctx *BindContext, isR
selectStmts := make([]*tree.SelectClause, groupingCount)
if groupingCount > 1 {
for i, list := range selectClause.GroupBy.GroupByExprsList {
if selectClause.Having != nil {
selectClause.Having.RollupHaving = true
}
selectStmts[i] = &tree.SelectClause{
Distinct: selectClause.Distinct,
Exprs: selectClause.Exprs,
Expand Down Expand Up @@ -2769,6 +2773,9 @@ func (builder *QueryBuilder) bindSelect(stmt *tree.Select, ctx *BindContext, isR
); err != nil {
return
}
if selectClause.Having != nil {
rollupFilter = selectClause.Having.RollupHaving
}
case *tree.UnionClause:
return builder.buildUnion(selectClause, astOrderBy, astLimit, ctx, isRoot)
case *tree.ValuesClause:
Expand Down Expand Up @@ -2874,7 +2881,7 @@ func (builder *QueryBuilder) bindSelect(stmt *tree.Select, ctx *BindContext, isR
return
}
} else if len(ctx.groups) > 0 || len(ctx.aggregates) > 0 {
if nodeID, err = builder.appendAggNode(ctx, nodeID, boundHavingList); err != nil {
if nodeID, err = builder.appendAggNode(ctx, nodeID, boundHavingList, rollupFilter); err != nil {
return
}
}
Expand Down Expand Up @@ -3545,6 +3552,7 @@ func (builder *QueryBuilder) appendAggNode(
ctx *BindContext,
nodeID int32,
boundHavingList []*plan.Expr,
rollupFilter bool,
) (newNodeID int32, err error) {
if ctx.bindingRecurStmt() {
err = moerr.NewInternalError(builder.GetContext(), "not support aggregate function recursive cte")
Expand Down Expand Up @@ -3579,9 +3587,10 @@ func (builder *QueryBuilder) appendAggNode(
}

nodeID = builder.appendNode(&plan.Node{
NodeType: plan.Node_FILTER,
Children: []int32{nodeID},
FilterList: newFilterList,
NodeType: plan.Node_FILTER,
Children: []int32{nodeID},
FilterList: newFilterList,
RollupFilter: rollupFilter,
}, ctx)
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/plan/query_builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ func TestQueryBuilder_appendAggNode(t *testing.T) {
boundHavingList, err := builder.bindHaving(bindCtx, selectClause.Having, NewHavingBinder(builder, bindCtx))
require.NoError(t, err)

nodeID, err = builder.appendAggNode(bindCtx, nodeID, boundHavingList)
nodeID, err = builder.appendAggNode(bindCtx, nodeID, boundHavingList, false)
require.NoError(t, err)
require.Equal(t, int32(2), nodeID)
require.Equal(t, 3, len(builder.qry.Nodes))
Expand Down
1 change: 1 addition & 0 deletions proto/plan.proto
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,7 @@ message Node {
OnDuplicateAction on_duplicate_action = 66;
string dedup_col_name = 67;
repeated Type dedup_col_types = 68 [(gogoproto.nullable) = false];
bool rollup_filter = 69;
}

// Snapshot Represents a snapshot of the database
Expand Down
31 changes: 28 additions & 3 deletions test/distributed/cases/window/rollup.result
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,18 @@ product with rollup
having
(product is not null and quarter is not null) or
(product is null and quarter is null);
[unknown result because it is related to issue#19984]
year quarter product total_sales
null null null 22000.00
2022 null null 7000.00
2023 null null 15000.00
2022 Q1 Product A 1000.00
2022 Q1 Product B 1500.00
2022 Q2 Product A 2000.00
2022 Q2 Product C 2500.00
2023 Q1 Product A 3000.00
2023 Q1 Product B 3500.00
2023 Q2 Product B 4000.00
2023 Q2 Product C 4500.00
select
year,
quarter,
Expand All @@ -165,7 +176,13 @@ quarter,
product with rollup
having
(product = 'Product A') or (product = 'Product B');
[unknown result because it is related to issue#19984]
year quarter product total_sales
2022 Q1 Product A 1000.00
2022 Q1 Product B 1500.00
2022 Q2 Product A 2000.00
2023 Q1 Product A 3000.00
2023 Q1 Product B 3500.00
2023 Q2 Product B 4000.00
drop table sales;
drop table if exists orders;
drop table if exists order_items;
Expand Down Expand Up @@ -367,7 +384,15 @@ group by
j, i with rollup
having
(i is not null and j is not null);
[unknown result because it is related to issue#19984]
i j total
18446744073709551615 2147483647 123214
4294967295 2147483647 2
18446744073709551615 1 2
2147483647 23289483 123214
13289392 2 2
18446744073709551615 23289483 1
3824 13289392 123215
2438294 1 2
drop table uint_64;
drop table if exists sales;
create table sales (
Expand Down
4 changes: 0 additions & 4 deletions test/distributed/cases/window/rollup.sql
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ group by
product with rollup;
-- @bvt:issue

-- @bvt:issue#19984
select
year,
quarter,
Expand Down Expand Up @@ -113,7 +112,6 @@ group by
product with rollup
having
(product = 'Product A') or (product = 'Product B');
-- @bvt:issue
drop table sales;


Expand Down Expand Up @@ -301,7 +299,6 @@ group by
j, i with rollup;
-- @bvt:issue

-- @bvt:issue#19984
select
i, j, sum(k) as total
from
Expand All @@ -310,7 +307,6 @@ group by
j, i with rollup
having
(i is not null and j is not null);
-- @bvt:issue
drop table uint_64;


Expand Down