From dc6cbad3fcd1086f8305327ddd13f75b53e9e5ac Mon Sep 17 00:00:00 2001 From: tgruben Date: Tue, 7 Mar 2023 15:56:58 -0600 Subject: [PATCH] compileOrderingTermExpr needs to return alias and not expression (#2300) --- sql3/planner/expression.go | 81 ++++++++++++++++++-------------------- sql3/sql_complex_test.go | 21 ++++++++++ 2 files changed, 60 insertions(+), 42 deletions(-) diff --git a/sql3/planner/expression.go b/sql3/planner/expression.go index b8d11e6e6..bc91d6769 100644 --- a/sql3/planner/expression.go +++ b/sql3/planner/expression.go @@ -84,7 +84,7 @@ func coerceValue(sourceType parser.ExprDataType, targetType parser.ExprDataType, case *parser.DataTypeString: return value, nil case *parser.DataTypeTimestamp: - //try to coerce to a date + // try to coerce to a date val, ok := value.(string) if !ok { return nil, sql3.NewErrInternalf("unexpected value type '%T'", value) @@ -112,7 +112,7 @@ func coerceValue(sourceType parser.ExprDataType, targetType parser.ExprDataType, return value, nil case *parser.DataTypeIDSetQuantum: return []interface{}{ - nil, //no timestamp + nil, // no timestamp value, }, nil } @@ -123,7 +123,7 @@ func coerceValue(sourceType parser.ExprDataType, targetType parser.ExprDataType, return value, nil case *parser.DataTypeStringSetQuantum: return []interface{}{ - nil, //no timestamp + nil, // no timestamp value, }, nil } @@ -346,7 +346,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e switch coercedDataType.(type) { case *parser.DataTypeBool: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -370,7 +370,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok) case *parser.DataTypeInt: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -431,7 +431,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok) case *parser.DataTypeID: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -492,7 +492,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok) case *parser.DataTypeDecimal: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -541,7 +541,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%T', '%T'", coercedLhs, coercedRhs) case *parser.DataTypeTimestamp: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -581,7 +581,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok) case *parser.DataTypeIDSet: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -603,7 +603,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok) case *parser.DataTypeString: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -646,7 +646,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok) case *parser.DataTypeStringSet: - //if either side is nil, return nil + // if either side is nil, return nil if evalLhs == nil || evalRhs == nil { return nil, nil } @@ -844,7 +844,7 @@ func (n *casePlanExpression) Evaluate(currentRow []interface{}) (interface{}, er return nil, sql3.NewErrInternalf("unhandled type '%s'", n.baseExpr.Type()) } } - //if we get to here, we're falling back to else + // if we get to here, we're falling back to else if n.elseExpr != nil { evalElse, err := n.elseExpr.Evaluate(currentRow) if err != nil { @@ -915,7 +915,7 @@ func (n *casePlanExpression) Evaluate(currentRow []interface{}) (interface{}, er } } } - //if we get to here, we're falling back to else + // if we get to here, we're falling back to else if n.elseExpr != nil { evalElse, err := n.elseExpr.Evaluate(currentRow) if err != nil { @@ -1092,25 +1092,25 @@ func newSubqueryPlanExpression(op types.PlanOperator) *subqueryPlanExpression { func (n *subqueryPlanExpression) Evaluate(currentRow []interface{}) (interface{}, error) { ctx := context.Background() - //get an iterator + // get an iterator iter, err := n.op.Iterator(ctx, currentRow) if err != nil { return nil, err } - //get the first row + // get the first row row, err := iter.Next(ctx) if err != nil { if err == types.ErrNoMoreRows { - //no rows, so return null - //TODO(pok) - check that this is the right behavior + // no rows, so return null + // TODO(pok) - check that this is the right behavior return nil, nil } return nil, err } result := row[0] - //make sure we don't have a next row - this is an error + // make sure we don't have a next row - this is an error _, err = iter.Next(ctx) if err != nil && err == types.ErrNoMoreRows { return result, nil @@ -1312,7 +1312,7 @@ func (n *inOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er return nil, err } - //if lhs is nil, bail + // if lhs is nil, bail if evalLhs == nil { return nil, nil } @@ -1324,13 +1324,13 @@ func (n *inOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er listMembers := []interface{}{} - //evaluate all the list members + // evaluate all the list members for _, lm := range exprList.exprs { lv, err := lm.Evaluate(currentRow) if err != nil { return nil, err } - //if any of the list members eval to nil, bail + // if any of the list members eval to nil, bail if lv == nil { return nil, nil } @@ -1494,7 +1494,6 @@ func (n *inOpPlanExpression) Plan() map[string]interface{} { result["lhs"] = n.lhs.Plan() result["rhs"] = n.rhs.Plan() return result - } func (n *inOpPlanExpression) Children() []types.PlanExpression { @@ -1723,7 +1722,7 @@ func (n *qualifiedRefPlanExpression) Evaluate(currentRow []interface{}) (interfa return result, nil case *parser.DataTypeID: - //this could be an int64 or a uint64 internally + // this could be an int64 or a uint64 internally iv, iok := currentRow[n.columnIndex].(int64) if iok { return iv, nil @@ -1799,7 +1798,6 @@ func (n *variableRefPlanExpression) Evaluate(currentRow []interface{}) (interfac } switch n.dataType.(type) { - default: return currentRow[n.variableIndex], nil } @@ -2071,7 +2069,7 @@ func (n *stringLiteralPlanExpression) WithChildren(children ...types.PlanExpress } func (expr *stringLiteralPlanExpression) ConvertToTimestamp() *time.Time { - //try to coerce to a date + // try to coerce to a date if tm, err := time.ParseInLocation(time.RFC3339Nano, expr.value, time.UTC); err == nil { return &tm } else if tm, err := time.ParseInLocation(time.RFC3339, expr.value, time.UTC); err == nil { @@ -2178,7 +2176,7 @@ func (n *castPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er case *parser.DataTypeIDSet: return nl, nil case *parser.DataTypeString: - //TODO(pok) come up with a better string representation of idset + // TODO(pok) come up with a better string representation of idset return fmt.Sprintf("%v", nl), nil } @@ -2191,7 +2189,7 @@ func (n *castPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er case *parser.DataTypeInt, *parser.DataTypeID: i, err := strconv.Atoi(nl) if err != nil { - //TODO(pok) need to push location into here + // TODO(pok) need to push location into here return nil, sql3.NewErrInvalidCast(0, 0, nl, n.targetType.TypeDescription()) } return int64(i), nil @@ -2199,7 +2197,7 @@ func (n *castPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er case *parser.DataTypeBool: i, err := strconv.ParseBool(nl) if err != nil { - //TODO(pok) need to push location into here + // TODO(pok) need to push location into here return nil, sql3.NewErrInvalidCast(0, 0, nl, n.targetType.TypeDescription()) } return i, nil @@ -2207,7 +2205,7 @@ func (n *castPlanExpression) Evaluate(currentRow []interface{}) (interface{}, er case *parser.DataTypeDecimal: castValue, err := pql.ParseDecimal(nl) if err != nil { - //TODO(pok) need to push location into here + // TODO(pok) need to push location into here return nil, sql3.NewErrInvalidCast(0, 0, nl, n.targetType.TypeDescription()) } if tt.Scale < castValue.Scale { @@ -2678,7 +2676,7 @@ func (p *ExecutionPlanner) compileExpr(expr parser.Expr) (_ types.PlanExpression func (p *ExecutionPlanner) compileUnaryExpr(expr *parser.UnaryExpr) (_ types.PlanExpression, err error) { switch op := expr.Op; op { - //bitwise operators + // bitwise operators case parser.BITNOT: x, err := p.compileExpr(expr.X) if err != nil { @@ -2686,7 +2684,7 @@ func (p *ExecutionPlanner) compileUnaryExpr(expr *parser.UnaryExpr) (_ types.Pla } return newUnaryOpPlanExpression(expr.Op, x, expr.ResultDataType), nil - //arithmetic operators + // arithmetic operators case parser.PLUS, parser.MINUS: x, err := p.compileExpr(expr.X) if err != nil { @@ -2710,26 +2708,26 @@ func (p *ExecutionPlanner) compileBinaryExpr(expr *parser.BinaryExpr) (_ types.P switch op := expr.Op; op { - //logical operators + // logical operators case parser.AND, parser.OR: return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil - //equality operators + // equality operators case parser.EQ, parser.NE: return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil - //comparison operators + // comparison operators case parser.LT, parser.LE, parser.GT, parser.GE: return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil - //arithmetic operators + // arithmetic operators case parser.PLUS, parser.MINUS, parser.STAR, parser.SLASH, parser.REM: - //TODO(pok) move constant folding to optimizer + // TODO(pok) move constant folding to optimizer opx, okx := x.(*intLiteralPlanExpression) opy, oky := y.(*intLiteralPlanExpression) if okx && oky { - //both literals so we can fold + // both literals so we can fold numx := opx.value numy := opy.value @@ -2755,18 +2753,18 @@ func (p *ExecutionPlanner) compileBinaryExpr(expr *parser.BinaryExpr) (_ types.P return newIntLiteralPlanExpression(value), nil default: - //run home to momma + // run home to momma return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil } } else { return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil } - //bitwise operators + // bitwise operators case parser.BITAND, parser.BITOR, parser.LSHIFT, parser.RSHIFT: return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil - //null test + // null test case parser.IS, parser.ISNOT: return newBinOpPlanExpression(x, expr.Op, y, expr.ResultDataType), nil @@ -2841,7 +2839,6 @@ func (p *ExecutionPlanner) compileOrderingTermExpr(expr parser.Expr, projections if expr == nil { return nil, nil } - switch thisExpr := expr.(type) { case *parser.Ident: for _, proj := range projections { @@ -2858,7 +2855,7 @@ func (p *ExecutionPlanner) compileOrderingTermExpr(expr parser.Expr, projections if !typeCanBeSortedOn(p.expr.Type()) { return nil, sql3.NewErrExpectedSortableExpression(0, 0, p.expr.Type().TypeDescription()) } - return p.expr, nil + return p, nil } } diff --git a/sql3/sql_complex_test.go b/sql3/sql_complex_test.go index 69955408b..ab9568895 100644 --- a/sql3/sql_complex_test.go +++ b/sql3/sql_complex_test.go @@ -3074,4 +3074,25 @@ WITH }, got); diff != "" { t.Fatal(diff) } + + // FB_2062 + _, _, _, err = sql_test.MustQueryRows(t, node, `create table sup305-fails (_id id, bucket string, value int);`) + assert.NoError(t, err) + _, _, _, err = sql_test.MustQueryRows(t, node, ` + insert into sup305-fails values (1, 'a', 1000), (2, 'b', 1000), (3, 'c', 1000), (4, 'c', 1000), (5, 'c', 1000), (6, 'c', 1000), (7, 'c', 1000), +(8, 'a', 1000), (9, 'b', 1000), (10, 'c', 1000), (11, 'c', 1000), (12, 'c', 1000), (13, 'c', 1000), (14, 'c', 1000), +(15, 'a', 1000), (16, 'b', 1000), (17, 'c', 1000), (18, 'c', 1000), (19, 'c', 1000), (20, 'c', 1000), (21, 'c', 1000);`) + assert.NoError(t, err) + results, _, _, err = sql_test.MustQueryRows(t, node, `select bucket, count(*) as cnt from sup305-fails group by bucket having count(*) > 1 order by cnt;`) + assert.NoError(t, err) + got = make([]int64, 0) + for i := range results { + got = append(got, results[i][1].(int64)) + } + sort.Slice(got, func(i, j int) bool { + return got[i] < got[j] + }) + if diff := cmp.Diff([]int64{3, 3, 15}, got); diff != "" { + t.Fatal(diff) + } }