From dd1a6ad0339af83350ee8a36b8847a171e4d4988 Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Tue, 30 Jul 2019 14:51:50 +0800 Subject: [PATCH 1/3] extract error judge from build and make judgment to origin spec --- planner/core/logical_plan_builder.go | 130 +++++++++++++++++++-------- planner/core/logical_plan_test.go | 28 ++++++ 2 files changed, 120 insertions(+), 38 deletions(-) diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index bc2087d1190b3..4f0c64b3ea562 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -3099,14 +3099,7 @@ func (b *PlanBuilder) buildWindowFunctionFrameBound(ctx context.Context, spec *a if bound.Type == ast.CurrentRow { return bound, nil } - // Rows type does not support interval range. - if boundClause.Unit != nil { - return nil, ErrWindowRowsIntervalUse.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - numRows, isNull, isExpectedType := getUintFromNode(b.ctx, boundClause.Expr) - if isNull || !isExpectedType { - return nil, ErrWindowFrameIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) - } + numRows, _, _ := getUintFromNode(b.ctx, boundClause.Expr) bound.Num = numRows return bound, nil } @@ -3122,23 +3115,7 @@ func (b *PlanBuilder) buildWindowFunctionFrameBound(ctx context.Context, spec *a return bound, nil } - if len(orderByItems) != 1 { - return nil, ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) - } col := orderByItems[0].Col - isNumeric, isTemporal := types.IsTypeNumeric(col.RetType.Tp), types.IsTypeTemporal(col.RetType.Tp) - if !isNumeric && !isTemporal { - return nil, ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - // Interval bounds only support order by temporal types. - if boundClause.Unit != nil && isNumeric { - return nil, ErrWindowRangeFrameNumericType.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - // Non-interval bound only support order by numeric types. - if boundClause.Unit == nil && !isNumeric { - return nil, ErrWindowRangeFrameTemporalType.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - // TODO: We also need to raise error for non-deterministic expressions, like rand(). val, err := evalAstExpr(b.ctx, boundClause.Expr) if err != nil { @@ -3223,25 +3200,13 @@ func (b *PlanBuilder) buildWindowFunctionFrame(ctx context.Context, spec *ast.Wi if frameClause == nil { return nil, nil } - if frameClause.Type == ast.Groups { - return nil, ErrNotSupportedYet.GenWithStackByArgs("GROUPS") - } frame := &WindowFrame{Type: frameClause.Type} - start := frameClause.Extent.Start - if start.Type == ast.Following && start.UnBounded { - return nil, ErrWindowFrameStartIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) - } var err error - frame.Start, err = b.buildWindowFunctionFrameBound(ctx, spec, orderByItems, &start) + frame.Start, err = b.buildWindowFunctionFrameBound(ctx, spec, orderByItems, &frameClause.Extent.Start) if err != nil { return nil, err } - - end := frameClause.Extent.End - if end.Type == ast.Preceding && end.UnBounded { - return nil, ErrWindowFrameEndIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - frame.End, err = b.buildWindowFunctionFrameBound(ctx, spec, orderByItems, &end) + frame.End, err = b.buildWindowFunctionFrameBound(ctx, spec, orderByItems, &frameClause.Extent.End) return frame, err } @@ -3316,6 +3281,10 @@ func (b *PlanBuilder) buildWindowFunctions(ctx context.Context, p LogicalPlan, g if err != nil { return nil, nil, err } + err = b.checkOriginWindowSpecs(funcs, orderBy) + if err != nil { + return nil, nil, err + } frame, err := b.buildWindowFunctionFrame(ctx, spec, orderBy) if err != nil { return nil, nil, err @@ -3356,6 +3325,90 @@ func (b *PlanBuilder) buildWindowFunctions(ctx context.Context, p LogicalPlan, g return p, windowMap, nil } +// checkOriginWindowSpecs checks the validation for origin window specifications for a group of functions. +// Because of the grouped specification is different from it, we should especially check them before build window frame. +func (b *PlanBuilder) checkOriginWindowSpecs(funcs []*ast.WindowFuncExpr, orderByItems []property.Item) error { + for _, f := range funcs { + spec := f.Spec + if spec.Frame == nil { + continue + } + if spec.Frame.Type == ast.Groups { + return ErrNotSupportedYet.GenWithStackByArgs("GROUPS") + } + start, end := spec.Frame.Extent.Start, spec.Frame.Extent.End + if start.Type == ast.Following && start.UnBounded { + return ErrWindowFrameStartIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + if end.Type == ast.Preceding && end.UnBounded { + return ErrWindowFrameEndIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + if start.Type == ast.Following && end.Type == ast.Preceding { + return ErrWindowFrameIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + + needFrame := aggregation.NeedFrame(f.F) + if !needFrame && spec.Frame != nil && spec.OrderBy != nil && + len(orderByItems) > 1 && spec.Frame.Type == ast.Ranges { + if start.Type == ast.Preceding && !start.UnBounded { + return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + if end.Type == ast.Following && !end.UnBounded { + return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + } + + err := b.checkOriginWindowFrameBound(&start, &spec, orderByItems) + if err != nil { + return err + } + err = b.checkOriginWindowFrameBound(&end, &spec, orderByItems) + if err != nil { + return err + } + } + return nil +} + +func (b *PlanBuilder) checkOriginWindowFrameBound(bound *ast.FrameBound, spec *ast.WindowSpec, orderByItems []property.Item) error { + if bound.UnBounded { + return nil + } + frameType := spec.Frame.Type + if frameType == ast.Rows { + if bound.Type == ast.CurrentRow || bound.UnBounded { + return nil + } + if bound.Unit != nil { + return ErrWindowRowsIntervalUse.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + _, isNull, isExpectedType := getUintFromNode(b.ctx, bound.Expr) + if isNull || !isExpectedType { + return ErrWindowFrameIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + return nil + } + if bound.Type == ast.CurrentRow { + return nil + } + + if len(orderByItems) != 1 { + return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + orderItemType := orderByItems[0].Col.RetType.Tp + isNumeric, isTemporal := types.IsTypeNumeric(orderItemType), types.IsTypeTemporal(orderItemType) + if !isNumeric && !isTemporal { + return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + if bound.Unit != nil && !isTemporal { + return ErrWindowRangeFrameNumericType.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + if bound.Unit == nil && !isNumeric { + return ErrWindowRangeFrameTemporalType.GenWithStackByArgs(getWindowName(spec.Name.O)) + } + return nil +} + func extractWindowFuncs(fields []*ast.SelectField) []*ast.WindowFuncExpr { extractor := &WindowFuncExtractor{} for _, f := range fields { @@ -3422,6 +3475,7 @@ func (b *PlanBuilder) groupWindowFuncs(windowFuncs []*ast.WindowFuncExpr) (map[* if !ok { return nil, ErrWindowNoSuchWindow.GenWithStackByArgs(windowFunc.Spec.Name.O) } + windowFunc.Spec = *spec newSpec, updated := b.handleDefaultFrame(spec, windowFunc.F) if !updated { groupedWindow[spec] = append(groupedWindow[spec], windowFunc) diff --git a/planner/core/logical_plan_test.go b/planner/core/logical_plan_test.go index fac77b8e6c31a..13e92178ea768 100644 --- a/planner/core/logical_plan_test.go +++ b/planner/core/logical_plan_test.go @@ -2416,6 +2416,34 @@ func (s *testPlanSuite) TestWindowFunction(c *C) { sql: "select sum(a) over (partition by a order by b), sum(b) over (order by a, b, c), sum(c) over(partition by a order by c), sum(d) over() from t", result: "TableReader(Table(t))->Sort->Window(sum(cast(test.t.c)) over(partition by test.t.a order by test.t.c asc range between unbounded preceding and current row))->Sort->Window(sum(cast(test.t.b)) over(order by test.t.a asc, test.t.b asc, test.t.c asc range between unbounded preceding and current row))->Window(sum(cast(test.t.a)) over(partition by test.t.a order by test.t.b asc range between unbounded preceding and current row))->Window(sum(cast(test.t.d)) over())->Projection", }, + // Test issue 11010. + { + sql: "select dense_rank() over w1, a, b from t window w1 as (partition by t.b order by t.a desc, t.b desc range between current row and 1 following)", + result: "[planner:3587]Window 'w1' with RANGE N PRECEDING/FOLLOWING frame requires exactly one ORDER BY expression, of numeric or temporal type", + }, + { + sql: "select dense_rank() over w1, a, b from t window w1 as (partition by t.b order by t.a desc, t.b desc range between current row and unbounded following)", + result: "TableReader(Table(t))->Sort->Window(dense_rank() over(partition by test.t.b order by test.t.a desc, test.t.b desc))->Projection", + }, + { + sql: "select dense_rank() over w1, a, b from t window w1 as (partition by t.b order by t.a desc, t.b desc range between 1 preceding and 1 following)", + result: "[planner:3587]Window 'w1' with RANGE N PRECEDING/FOLLOWING frame requires exactly one ORDER BY expression, of numeric or temporal type", + }, + // Test issue 11001. + { + sql: "SELECT PERCENT_RANK() OVER w1 AS 'percent_rank', fieldA, fieldB FROM ( SELECT a AS fieldA, b AS fieldB FROM t ) t1 WINDOW w1 AS ( ROWS BETWEEN 0 FOLLOWING AND UNBOUNDED PRECEDING)", + result: "[planner:3585]Window 'w1': frame end cannot be UNBOUNDED PRECEDING.", + }, + // Test issue 11002. + { + sql: "SELECT PERCENT_RANK() OVER w1 AS 'percent_rank', fieldA, fieldB FROM ( SELECT a AS fieldA, b AS fieldB FROM t ) as t1 WINDOW w1 AS ( ROWS BETWEEN UNBOUNDED FOLLOWING AND UNBOUNDED FOLLOWING)", + result: "[planner:3584]Window 'w1': frame start cannot be UNBOUNDED FOLLOWING.", + }, + // Test issue 11011. + { + sql: "select dense_rank() over w1, a, b from t window w1 as (partition by t.b order by t.a asc range between 1250951168 following AND 1250951168 preceding)", + result: "[planner:3586]Window 'w1': frame start or end is negative, NULL or of non-integral type", + }, } s.Parser.EnableWindowFunc(true) From 3bebdd32de5e77534c47d8923879dac3e6f0109c Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Tue, 30 Jul 2019 14:59:07 +0800 Subject: [PATCH 2/3] remove unnecessary judgment --- planner/core/logical_plan_builder.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index 4f0c64b3ea562..7735481ccf748 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -3347,17 +3347,6 @@ func (b *PlanBuilder) checkOriginWindowSpecs(funcs []*ast.WindowFuncExpr, orderB return ErrWindowFrameIllegal.GenWithStackByArgs(getWindowName(spec.Name.O)) } - needFrame := aggregation.NeedFrame(f.F) - if !needFrame && spec.Frame != nil && spec.OrderBy != nil && - len(orderByItems) > 1 && spec.Frame.Type == ast.Ranges { - if start.Type == ast.Preceding && !start.UnBounded { - return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - if end.Type == ast.Following && !end.UnBounded { - return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O)) - } - } - err := b.checkOriginWindowFrameBound(&start, &spec, orderByItems) if err != nil { return err From adbde5e5c0201611fe56ab7db09ce041b05ec402 Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Tue, 30 Jul 2019 15:04:56 +0800 Subject: [PATCH 3/3] refine code --- planner/core/logical_plan_builder.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index 7735481ccf748..83de245b78eda 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -3360,14 +3360,12 @@ func (b *PlanBuilder) checkOriginWindowSpecs(funcs []*ast.WindowFuncExpr, orderB } func (b *PlanBuilder) checkOriginWindowFrameBound(bound *ast.FrameBound, spec *ast.WindowSpec, orderByItems []property.Item) error { - if bound.UnBounded { + if bound.Type == ast.CurrentRow || bound.UnBounded { return nil } + frameType := spec.Frame.Type if frameType == ast.Rows { - if bound.Type == ast.CurrentRow || bound.UnBounded { - return nil - } if bound.Unit != nil { return ErrWindowRowsIntervalUse.GenWithStackByArgs(getWindowName(spec.Name.O)) } @@ -3377,9 +3375,6 @@ func (b *PlanBuilder) checkOriginWindowFrameBound(bound *ast.FrameBound, spec *a } return nil } - if bound.Type == ast.CurrentRow { - return nil - } if len(orderByItems) != 1 { return ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O))