From 4995fe741ea527b3870731a6873d34a92403d3de Mon Sep 17 00:00:00 2001 From: gaoxingliang Date: Sat, 10 Aug 2019 16:22:45 +0800 Subject: [PATCH] planner/core: check the window func arg first before checking window specs (#11613) --- cmd/explaintest/main.go | 3 +- planner/core/logical_plan_builder.go | 51 ++++++++++++++++++++++++++++ planner/core/logical_plan_test.go | 8 +++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/cmd/explaintest/main.go b/cmd/explaintest/main.go index b29b05c93c25e..4e4f6defd9e05 100644 --- a/cmd/explaintest/main.go +++ b/cmd/explaintest/main.go @@ -357,10 +357,9 @@ func (t *tester) execute(query query) error { gotBuf := t.buf.Bytes()[offset:] buf := make([]byte, t.buf.Len()-offset) - if _, err = t.resultFD.ReadAt(buf, int64(offset)); err != nil { + if _, err = t.resultFD.ReadAt(buf, int64(offset)); !(err == nil || err == io.EOF) { return errors.Trace(errors.Errorf("run \"%v\" at line %d err, we got \n%s\nbut read result err %s", st.Text(), query.Line, gotBuf, err)) } - if !bytes.Equal(gotBuf, buf) { return errors.Trace(errors.Errorf("run \"%v\" at line %d err, we need:\n%s\nbut got:\n%s\n", query.Query, query.Line, buf, gotBuf)) } diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index 9faee975b1c39..b257b6287bca6 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -2099,6 +2099,11 @@ func (b *PlanBuilder) buildSelect(ctx context.Context, sel *ast.SelectStmt) (p L var windowMapper map[*ast.WindowFuncExpr]int if hasWindowFuncField { windowFuncs := extractWindowFuncs(sel.Fields.Fields) + // we need to check the func args first before we check the window spec + err := b.checkWindowFuncArgs(ctx, p, windowFuncs, windowAggMap) + if err != nil { + return nil, err + } groupedFuncs, err := b.groupWindowFuncs(windowFuncs) if err != nil { return nil, err @@ -3056,6 +3061,35 @@ func (b *PlanBuilder) buildProjectionForWindow(ctx context.Context, p LogicalPla return proj, propertyItems[:lenPartition], propertyItems[lenPartition:], newArgList, nil } +func (b *PlanBuilder) buildArgs4WindowFunc(ctx context.Context, p LogicalPlan, args []ast.ExprNode, aggMap map[*ast.AggregateFuncExpr]int) ([]expression.Expression, error) { + b.optFlag |= flagEliminateProjection + + newArgList := make([]expression.Expression, 0, len(args)) + // use below index for created a new col definition + // it's okay here because we only want to return the args used in window function + newColIndex := 0 + for _, arg := range args { + newArg, np, err := b.rewrite(ctx, arg, p, aggMap, true) + if err != nil { + return nil, err + } + p = np + switch newArg.(type) { + case *expression.Column, *expression.Constant: + newArgList = append(newArgList, newArg) + continue + } + col := &expression.Column{ + ColName: model.NewCIStr(fmt.Sprintf("%d_proj_window_%d", p.ID(), newColIndex)), + UniqueID: b.ctx.GetSessionVars().AllocPlanColumnID(), + RetType: newArg.GetType(), + } + newColIndex += 1 + newArgList = append(newArgList, col) + } + return newArgList, nil +} + func (b *PlanBuilder) buildByItemsForWindow( ctx context.Context, p LogicalPlan, @@ -3220,6 +3254,23 @@ func (b *PlanBuilder) buildWindowFunctionFrame(ctx context.Context, spec *ast.Wi return frame, err } +func (b *PlanBuilder) checkWindowFuncArgs(ctx context.Context, p LogicalPlan, windowFuncExprs []*ast.WindowFuncExpr, windowAggMap map[*ast.AggregateFuncExpr]int) error { + for _, windowFuncExpr := range windowFuncExprs { + args, err := b.buildArgs4WindowFunc(ctx, p, windowFuncExpr.Args, windowAggMap) + if err != nil { + return err + } + desc, err := aggregation.NewWindowFuncDesc(b.ctx, windowFuncExpr.F, args) + if err != nil { + return err + } + if desc == nil { + return ErrWrongArguments.GenWithStackByArgs(strings.ToLower(windowFuncExpr.F)) + } + } + return nil +} + func getAllByItems(itemsBuf []*ast.ByItem, spec *ast.WindowSpec) []*ast.ByItem { itemsBuf = itemsBuf[:0] if spec.PartitionBy != nil { diff --git a/planner/core/logical_plan_test.go b/planner/core/logical_plan_test.go index c79b630efb2b1..a2843b98fc6f8 100644 --- a/planner/core/logical_plan_test.go +++ b/planner/core/logical_plan_test.go @@ -2469,6 +2469,14 @@ func (s *testPlanSuite) TestWindowFunction(c *C) { sql: "SELECT NTH_VALUE(a, 1) FROM LAST IGNORE NULLS over (partition by b order by b), a FROM t", result: "[planner:1235]This version of TiDB doesn't yet support 'IGNORE NULLS'", }, + { + sql: "SELECT NTH_VALUE(fieldA, ATAN(-1)) OVER (w1) AS 'ntile', fieldA, fieldB FROM ( SELECT a AS fieldA, b AS fieldB FROM t ) as te WINDOW w1 AS ( ORDER BY fieldB ASC, fieldA DESC )", + result: "[planner:1210]Incorrect arguments to nth_value", + }, + { + sql: "SELECT NTH_VALUE(fieldA, -1) OVER (w1 PARTITION BY fieldB ORDER BY fieldB , fieldA ) AS 'ntile', fieldA, fieldB FROM ( SELECT a AS fieldA, b AS fieldB FROM t ) as temp WINDOW w1 AS ( ORDER BY fieldB ASC, fieldA DESC )", + result: "[planner:1210]Incorrect arguments to nth_value", + }, } s.Parser.EnableWindowFunc(true)