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
expression: open CAST push down switcher default #12652
Changes from all commits
8e27d6b
c275ef1
c6ebe83
3d31c27
c33b5fa
5f09713
62639e8
3285154
d98dfc4
fec2b71
4432ba2
5ab6317
3746056
8755851
799bd92
7c2d019
df9f35f
7afdfba
677f3c5
9b37da7
7607ac5
57b24be
9c620a7
6d0d3b2
9acf7ad
e606ac3
e46be5c
daf7a02
9fc8390
48d4832
a04215c
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 |
---|---|---|
|
@@ -15,6 +15,7 @@ package aggregation | |
|
||
import ( | ||
"github.com/pingcap/errors" | ||
"github.com/pingcap/parser/mysql" | ||
"github.com/pingcap/tidb/sessionctx/stmtctx" | ||
"github.com/pingcap/tidb/types" | ||
"github.com/pingcap/tidb/util/codec" | ||
|
@@ -54,7 +55,7 @@ func (d *distinctChecker) Check(values []types.Datum) (bool, error) { | |
} | ||
|
||
// calculateSum adds v to sum. | ||
func calculateSum(sc *stmtctx.StatementContext, sum, v types.Datum) (data types.Datum, err error) { | ||
func calculateSum(sc *stmtctx.StatementContext, sum, v types.Datum, retType *types.FieldType) (data types.Datum, err error) { | ||
// for avg and sum calculation | ||
// avg and sum use decimal for integer and decimal type, use float for others | ||
// see https://dev.mysql.com/doc/refman/5.7/en/group-by-functions.html | ||
|
@@ -68,7 +69,22 @@ func calculateSum(sc *stmtctx.StatementContext, sum, v types.Datum) (data types. | |
data = types.NewDecimalDatum(d) | ||
} | ||
case types.KindMysqlDecimal: | ||
data = types.CloneDatum(v) | ||
dec := v.GetMysqlDecimal() | ||
frac := retType.Decimal | ||
if frac == types.UnspecifiedLength { | ||
frac = mysql.MaxDecimalScale | ||
} | ||
if int(dec.GetDigitsFrac()) > frac { | ||
to := new(types.MyDecimal) | ||
err := dec.Round(to, frac, types.ModeHalfEven) | ||
data = types.Datum{} | ||
data.SetMysqlDecimal(to) | ||
if err != nil { | ||
return data, err | ||
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. Why put 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. Yes, we should return the data even though overflow occurred (the caller should handle it in a proper way if the err is overflow or truncated). |
||
} | ||
} else { | ||
data = types.CloneDatum(v) | ||
} | ||
default: | ||
var f float64 | ||
f, err = v.ToFloat64(sc) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -266,6 +266,7 @@ func (s *testEvaluatorSuite) TestCompareFunc2Pb(c *C) { | |
|
||
func (s *testEvaluatorSuite) TestLikeFunc2Pb(c *C) { | ||
var likeFuncs []Expression | ||
var expects []string | ||
sc := new(stmtctx.StatementContext) | ||
client := new(mock.Client) | ||
|
||
|
@@ -280,19 +281,21 @@ func (s *testEvaluatorSuite) TestLikeFunc2Pb(c *C) { | |
} | ||
ctx := mock.NewContext() | ||
retTp = types.NewFieldType(mysql.TypeUnspecified) | ||
fc, err := NewFunction(ctx, ast.Like, retTp, args[0], args[1], args[3]) | ||
fc1, err := NewFunction(ctx, ast.Like, retTp, args[0], args[1], args[3]) | ||
c.Assert(err, IsNil) | ||
likeFuncs = append(likeFuncs, fc) | ||
expects = append(expects, `{"tp":10000,"children":[{"tp":5,"val":"c3RyaW5n","sig":0,"field_type":{"tp":254,"flag":0,"flen":-1,"decimal":-1,"collate":83,"charset":"utf8"}},{"tp":5,"val":"cGF0dGVybg==","sig":0,"field_type":{"tp":254,"flag":0,"flen":-1,"decimal":-1,"collate":83,"charset":"utf8"}},{"tp":10000,"val":"CAA=","children":[{"tp":5,"val":"XA==","sig":0,"field_type":{"tp":254,"flag":0,"flen":-1,"decimal":-1,"collate":83,"charset":"utf8"}}],"sig":30,"field_type":{"tp":8,"flag":128,"flen":-1,"decimal":0,"collate":63,"charset":"binary"}}],"sig":4310,"field_type":{"tp":8,"flag":128,"flen":1,"decimal":0,"collate":63,"charset":"binary"}}`) | ||
likeFuncs = append(likeFuncs, fc1) | ||
|
||
fc, err = NewFunction(ctx, ast.Like, retTp, args[0], args[2], args[3]) | ||
fc2, err := NewFunction(ctx, ast.Like, retTp, args[0], args[2], args[3]) | ||
c.Assert(err, IsNil) | ||
likeFuncs = append(likeFuncs, fc) | ||
expects = append(expects, `{"tp":10000,"children":[{"tp":5,"val":"c3RyaW5n","sig":0,"field_type":{"tp":254,"flag":0,"flen":-1,"decimal":-1,"collate":83,"charset":"utf8"}},{"tp":5,"val":"JWFiYyU=","sig":0,"field_type":{"tp":254,"flag":0,"flen":-1,"decimal":-1,"collate":83,"charset":"utf8"}},{"tp":10000,"val":"CAA=","children":[{"tp":5,"val":"XA==","sig":0,"field_type":{"tp":254,"flag":0,"flen":-1,"decimal":-1,"collate":83,"charset":"utf8"}}],"sig":30,"field_type":{"tp":8,"flag":128,"flen":-1,"decimal":0,"collate":63,"charset":"binary"}}],"sig":4310,"field_type":{"tp":8,"flag":128,"flen":1,"decimal":0,"collate":63,"charset":"binary"}}`) | ||
likeFuncs = append(likeFuncs, fc2) | ||
|
||
pbExprs := ExpressionsToPBList(sc, likeFuncs, client) | ||
for _, pbExpr := range pbExprs { | ||
for i, pbExpr := range pbExprs { | ||
js, err := json.Marshal(pbExpr) | ||
c.Assert(err, IsNil) | ||
c.Assert(string(js), Equals, "null") | ||
c.Assert(string(js), Equals, expects[i]) | ||
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. Why does this result change? |
||
} | ||
} | ||
|
||
|
@@ -428,28 +431,29 @@ func (s *testEvaluatorSerialSuites) TestPushDownSwitcher(c *C) { | |
cases := []struct { | ||
name string | ||
sig tipb.ScalarFuncSig | ||
argCnt int | ||
enable bool | ||
}{ | ||
{ast.And, tipb.ScalarFuncSig_BitAndSig, true}, | ||
{ast.Or, tipb.ScalarFuncSig_BitOrSig, false}, | ||
{ast.UnaryNot, tipb.ScalarFuncSig_UnaryNotInt, true}, | ||
{ast.And, tipb.ScalarFuncSig_BitAndSig, 2, true}, | ||
{ast.Or, tipb.ScalarFuncSig_BitOrSig, 2, false}, | ||
{ast.UnaryNot, tipb.ScalarFuncSig_UnaryNotInt, 1, true}, | ||
} | ||
var enabled []string | ||
for i, funcName := range cases { | ||
args := []Expression{dg.genColumn(mysql.TypeLong, 1)} | ||
if i+1 < len(cases) { | ||
args = append(args, dg.genColumn(mysql.TypeLong, 2)) | ||
for _, cs := range cases { | ||
args := make([]Expression, 0, cs.argCnt) | ||
for i := 0; i < cs.argCnt; i++ { | ||
args = append(args, dg.genColumn(mysql.TypeLong, int64(i))) | ||
} | ||
fc, err := NewFunction( | ||
mock.NewContext(), | ||
funcName.name, | ||
cs.name, | ||
types.NewFieldType(mysql.TypeUnspecified), | ||
args..., | ||
) | ||
c.Assert(err, IsNil) | ||
funcs = append(funcs, fc) | ||
if funcName.enable { | ||
enabled = append(enabled, funcName.name) | ||
if cs.enable { | ||
enabled = append(enabled, cs.name) | ||
} | ||
} | ||
|
||
|
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.
What's the relationship between pushdown CAST and
calculateSum
? Are there any test cases forsum
?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.
There are many test cases that will cover the
sum
(query withsum/avg
aggregation functions). The is no direct relationship withCAST
andsum
and I modify it because of the previous implementation does not round appropriately (this BUG is detected by this PR).