executor: fix wrong behavior for max/min on ENUM/SET column (#19552) (

#19869)
pingcap · Sep 8, 2020 · 3706da0 · 3706da0
1 parent 1ae5a03
commit 3706da0
Show file tree

Hide file tree

Showing 5 changed files with 161 additions and 2 deletions.
diff --git a/executor/aggfuncs/aggfuncs.go b/executor/aggfuncs/aggfuncs.go
@@ -66,6 +66,8 @@ var (
 	_ AggFunc = (*maxMin4String)(nil)
 	_ AggFunc = (*maxMin4Duration)(nil)
 	_ AggFunc = (*maxMin4JSON)(nil)
+	_ AggFunc = (*maxMin4Enum)(nil)
+	_ AggFunc = (*maxMin4Set)(nil)
 
 	// All the AggFunc implementations for "AVG" are listed here.
 	_ AggFunc = (*avgOriginal4Decimal)(nil)

diff --git a/executor/aggfuncs/builder.go b/executor/aggfuncs/builder.go
@@ -323,6 +323,13 @@ func buildMaxMin(aggFuncDesc *aggregation.AggFuncDesc, ordinal int, isMax bool)
 	switch aggFuncDesc.Mode {
 	case aggregation.DedupMode:
 	default:
+		switch fieldType.Tp {
+		case mysql.TypeEnum:
+			return &maxMin4Enum{base}
+		case mysql.TypeSet:
+			return &maxMin4Set{base}
+		}
+
 		switch evalType {
 		case types.ETInt:
 			if mysql.HasUnsignedFlag(fieldType.Flag) {

diff --git a/executor/aggfuncs/func_max_min.go b/executor/aggfuncs/func_max_min.go
@@ -69,6 +69,16 @@ type partialResult4MaxMinJSON struct {
 	isNull bool
 }
 
+type partialResult4MaxMinEnum struct {
+	val    types.Enum
+	isNull bool
+}
+
+type partialResult4MaxMinSet struct {
+	val    types.Set
+	isNull bool
+}
+
 type baseMaxMinAggFunc struct {
 	baseAggFunc
 
@@ -657,3 +667,129 @@ func (e *maxMin4JSON) MergePartialResult(sctx sessionctx.Context, src, dst Parti
 	}
 	return nil
 }
+
+type maxMin4Enum struct {
+	baseMaxMinAggFunc
+}
+
+func (e *maxMin4Enum) AllocPartialResult() PartialResult {
+	p := new(partialResult4MaxMinEnum)
+	p.isNull = true
+	return PartialResult(p)
+}
+
+func (e *maxMin4Enum) ResetPartialResult(pr PartialResult) {
+	p := (*partialResult4MaxMinEnum)(pr)
+	p.isNull = true
+}
+
+func (e *maxMin4Enum) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error {
+	p := (*partialResult4MaxMinEnum)(pr)
+	if p.isNull {
+		chk.AppendNull(e.ordinal)
+		return nil
+	}
+	chk.AppendEnum(e.ordinal, p.val)
+	return nil
+}
+
+func (e *maxMin4Enum) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
+	p := (*partialResult4MaxMinEnum)(pr)
+	for _, row := range rowsInGroup {
+		d, err := e.args[0].Eval(row)
+		if err != nil {
+			return err
+		}
+		if d.IsNull() {
+			continue
+		}
+		if p.isNull {
+			p.val = d.GetMysqlEnum().Copy()
+			p.isNull = false
+			continue
+		}
+		en := d.GetMysqlEnum()
+		if e.isMax && en.Value > p.val.Value || !e.isMax && en.Value < p.val.Value {
+			p.val = en.Copy()
+		}
+	}
+	return nil
+}
+
+func (e *maxMin4Enum) MergePartialResult(sctx sessionctx.Context, src, dst PartialResult) error {
+	p1, p2 := (*partialResult4MaxMinEnum)(src), (*partialResult4MaxMinEnum)(dst)
+	if p1.isNull {
+		return nil
+	}
+	if p2.isNull {
+		*p2 = *p1
+		return nil
+	}
+	if e.isMax && p1.val.Value > p2.val.Value || !e.isMax && p1.val.Value < p2.val.Value {
+		p2.val, p2.isNull = p1.val, false
+	}
+	return nil
+}
+
+type maxMin4Set struct {
+	baseMaxMinAggFunc
+}
+
+func (e *maxMin4Set) AllocPartialResult() PartialResult {
+	p := new(partialResult4MaxMinSet)
+	p.isNull = true
+	return PartialResult(p)
+}
+
+func (e *maxMin4Set) ResetPartialResult(pr PartialResult) {
+	p := (*partialResult4MaxMinSet)(pr)
+	p.isNull = true
+}
+
+func (e *maxMin4Set) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error {
+	p := (*partialResult4MaxMinSet)(pr)
+	if p.isNull {
+		chk.AppendNull(e.ordinal)
+		return nil
+	}
+	chk.AppendSet(e.ordinal, p.val)
+	return nil
+}
+
+func (e *maxMin4Set) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
+	p := (*partialResult4MaxMinSet)(pr)
+	for _, row := range rowsInGroup {
+		d, err := e.args[0].Eval(row)
+		if err != nil {
+			return err
+		}
+		if d.IsNull() {
+			continue
+		}
+		if p.isNull {
+			p.val = d.GetMysqlSet().Copy()
+			p.isNull = false
+			continue
+		}
+		s := d.GetMysqlSet()
+		if e.isMax && s.Value > p.val.Value || !e.isMax && s.Value < p.val.Value {
+			p.val = s.Copy()
+		}
+	}
+	return nil
+}
+
+func (e *maxMin4Set) MergePartialResult(sctx sessionctx.Context, src, dst PartialResult) error {
+	p1, p2 := (*partialResult4MaxMinSet)(src), (*partialResult4MaxMinSet)(dst)
+	if p1.isNull {
+		return nil
+	}
+	if p2.isNull {
+		*p2 = *p1
+		return nil
+	}
+	if e.isMax && p1.val.Value > p2.val.Value || !e.isMax && p1.val.Value < p2.val.Value {
+		p2.val, p2.isNull = p1.val, false
+	}
+	return nil
+}
diff --git a/executor/aggfuncs/func_max_min_test.go b/executor/aggfuncs/func_max_min_test.go
@@ -24,6 +24,15 @@ import (
 )
 
 func (s *testSuite) TestMergePartialResult4MaxMin(c *C) {
+	elems := []string{"a", "b", "c", "d", "e"}
+	enumA, _ := types.ParseEnumName(elems, "a", mysql.DefaultCollationName)
+	enumC, _ := types.ParseEnumName(elems, "c", mysql.DefaultCollationName)
+	enumE, _ := types.ParseEnumName(elems, "e", mysql.DefaultCollationName)
+
+	setA, _ := types.ParseSetName(elems, "a", mysql.DefaultCollationName)    // setA.Value == 1
+	setAB, _ := types.ParseSetName(elems, "a,b", mysql.DefaultCollationName) // setAB.Value == 3
+	setAC, _ := types.ParseSetName(elems, "a,c", mysql.DefaultCollationName) // setAC.Value == 5
+
 	unsignedType := types.NewFieldType(mysql.TypeLonglong)
 	unsignedType.Flag |= mysql.UnsignedFlag
 	tests := []aggTest{
@@ -36,6 +45,8 @@ func (s *testSuite) TestMergePartialResult4MaxMin(c *C) {
 		buildAggTester(ast.AggFuncMax, mysql.TypeDate, 5, types.TimeFromDays(369), types.TimeFromDays(369), types.TimeFromDays(369)),
 		buildAggTester(ast.AggFuncMax, mysql.TypeDuration, 5, types.Duration{Duration: time.Duration(4)}, types.Duration{Duration: time.Duration(4)}, types.Duration{Duration: time.Duration(4)}),
 		buildAggTester(ast.AggFuncMax, mysql.TypeJSON, 5, json.CreateBinary(int64(4)), json.CreateBinary(int64(4)), json.CreateBinary(int64(4))),
+		buildAggTester(ast.AggFuncMax, mysql.TypeEnum, 5, enumE, enumE, enumE),
+		buildAggTester(ast.AggFuncMax, mysql.TypeSet, 5, setAC, setAC, setAC),
 
 		buildAggTester(ast.AggFuncMin, mysql.TypeLonglong, 5, 0, 2, 0),
 		buildAggTesterWithFieldType(ast.AggFuncMin, unsignedType, 5, 0, 2, 0),
@@ -46,6 +57,8 @@ func (s *testSuite) TestMergePartialResult4MaxMin(c *C) {
 		buildAggTester(ast.AggFuncMin, mysql.TypeDate, 5, types.TimeFromDays(365), types.TimeFromDays(367), types.TimeFromDays(365)),
 		buildAggTester(ast.AggFuncMin, mysql.TypeDuration, 5, types.Duration{Duration: time.Duration(0)}, types.Duration{Duration: time.Duration(2)}, types.Duration{Duration: time.Duration(0)}),
 		buildAggTester(ast.AggFuncMin, mysql.TypeJSON, 5, json.CreateBinary(int64(0)), json.CreateBinary(int64(2)), json.CreateBinary(int64(0))),
+		buildAggTester(ast.AggFuncMin, mysql.TypeEnum, 5, enumA, enumC, enumA),
+		buildAggTester(ast.AggFuncMin, mysql.TypeSet, 5, setA, setAB, setA),
 	}
 	for _, test := range tests {
 		s.testMergePartialResult(c, test)

diff --git a/expression/aggregation/base_func.go b/expression/aggregation/base_func.go
@@ -201,8 +201,9 @@ func (a *baseFuncDesc) typeInfer4MaxMin(ctx sessionctx.Context) {
 		a.RetTp = a.Args[0].GetType().Clone()
 		a.RetTp.Flag &^= mysql.NotNullFlag
 	}
-	// TODO: fix other aggFuncs for TypeEnum & TypeSet
-	if (a.RetTp.Tp == mysql.TypeEnum || a.RetTp.Tp == mysql.TypeSet) && a.Name != ast.AggFuncFirstRow {
+	// issue #13027, #13961
+	if (a.RetTp.Tp == mysql.TypeEnum || a.RetTp.Tp == mysql.TypeSet) &&
+		(a.Name != ast.AggFuncFirstRow && a.Name != ast.AggFuncMax && a.Name != ast.AggFuncMin) {
 		a.RetTp = &types.FieldType{Tp: mysql.TypeString, Flen: mysql.MaxFieldCharLength}
 	}
 }