Skip to content

Commit

Permalink
planner: update IsCompleteModeAgg to find if any of aggFuncs are Comp…
Browse files Browse the repository at this point in the history
…leteMode, not just compare the first aggFunc (pingcap#24449)
  • Loading branch information
沐封 committed May 25, 2021
1 parent 11e7ac6 commit 67d22e7
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 6 deletions.
19 changes: 13 additions & 6 deletions planner/core/logical_plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,15 @@ func (la *LogicalAggregation) IsPartialModeAgg() bool {

// IsCompleteModeAgg returns if all of the AggFuncs are CompleteMode.
func (la *LogicalAggregation) IsCompleteModeAgg() bool {
// Since all of the AggFunc share the same AggMode, we only need to check the first one.
return la.AggFuncs[0].Mode == aggregation.CompleteMode
// not all of the AggFuncs has the same AggMode
// for example: when cascades planner is on, after PushAggDownGather transformed,
// some AggFuncs are CompleteMode, and the others are FinalMode.
for _, aggFunc := range la.AggFuncs {
if aggFunc.Mode == aggregation.CompleteMode {
return true
}
}
return false
}

// GetGroupByCols returns the columns that are group-by items.
Expand Down Expand Up @@ -391,7 +398,7 @@ func (la *LogicalAggregation) GetUsedCols() (usedCols []*expression.Column) {
type LogicalSelection struct {
baseLogicalPlan

// Originally the WHERE or ON condition is parsed into a single expression,
// Conditions the original WHERE or ON condition is parsed into a single expression,
// but after we converted to CNF(Conjunctive normal form), it can be
// split into a list of AND conditions.
Conditions []expression.Expression
Expand Down Expand Up @@ -494,12 +501,12 @@ type DataSource struct {
// possibleAccessPaths stores all the possible access path for physical plan, including table scan.
possibleAccessPaths []*util.AccessPath

// The data source may be a partition, rather than a real table.
// isPartition The data source may be a partition, rather than a real table.
isPartition bool
physicalTableID int64
partitionNames []model.CIStr

// handleCol represents the handle column for the datasource, either the
// handleCols represents the handle column for the datasource, either the
// int primary key column or extra handle column.
// handleCol *expression.Column
handleCols HandleCols
Expand Down Expand Up @@ -557,7 +564,7 @@ type LogicalTableScan struct {
// LogicalIndexScan is the logical index scan operator for TiKV.
type LogicalIndexScan struct {
logicalSchemaProducer
// DataSource should be read-only here.
// Source should be read-only here.
Source *DataSource
IsDoubleRead bool

Expand Down
26 changes: 26 additions & 0 deletions planner/core/logical_plans_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/pingcap/parser/model"
"github.com/pingcap/parser/mysql"
"github.com/pingcap/tidb/expression"
"github.com/pingcap/tidb/expression/aggregation"
"github.com/pingcap/tidb/planner/util"
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/types"
Expand Down Expand Up @@ -207,3 +208,28 @@ func (s *testUnitTestSuit) TestIndexPathSplitCorColCond(c *C) {
}
collate.SetNewCollationEnabledForTest(false)
}

func (s *testUnitTestSuit) TestIsCompleteModeAgg(c *C) {
defer testleak.AfterTest(c)()

aggFuncs := make([]*aggregation.AggFuncDesc, 2)
aggFuncs[0] = &aggregation.AggFuncDesc{
Mode: aggregation.FinalMode,
HasDistinct: true,
}
aggFuncs[1] = &aggregation.AggFuncDesc{
Mode: aggregation.CompleteMode,
HasDistinct: true,
}

newAgg := &LogicalAggregation{
AggFuncs: aggFuncs,
}
c.Assert(newAgg.IsCompleteModeAgg(), Equals, true)

aggFuncs[1] = &aggregation.AggFuncDesc{
Mode: aggregation.FinalMode,
HasDistinct: true,
}
c.Assert(newAgg.IsCompleteModeAgg(), Equals, false)
}

0 comments on commit 67d22e7

Please sign in to comment.