diff --git a/enginetest/queries/insert_queries.go b/enginetest/queries/insert_queries.go index 9fd17e23a5..bd04695167 100644 --- a/enginetest/queries/insert_queries.go +++ b/enginetest/queries/insert_queries.go @@ -364,10 +364,8 @@ var InsertQueries = []WriteQueryTest{ {10, "numrows: 1"}, }, }, - // TODO: this doesn't match MySQL. MySQL requires giving an alias to the expression to use it in a HAVING clause, - // but that causes an error in our engine. Needs work { - WriteQuery: "INSERT INTO mytable (i,s) SELECT CHAR_LENGTH(s), concat('numrows: ', count(*)) from mytable group by 1 HAVING CHAR_LENGTH(s) > 9", + WriteQuery: "INSERT INTO mytable (i,s) SELECT CHAR_LENGTH(s) as len, concat('numrows: ', count(*)) from mytable group by 1 HAVING len > 9", ExpectedWriteResult: []sql.Row{{types.NewOkResult(1)}}, SelectQuery: "SELECT * FROM mytable ORDER BY i, s", ExpectedSelect: []sql.Row{ diff --git a/enginetest/queries/queries.go b/enginetest/queries/queries.go index 1f537cdd11..7279afb458 100644 --- a/enginetest/queries/queries.go +++ b/enginetest/queries/queries.go @@ -3484,6 +3484,10 @@ Select * from ( Query: `SELECT SUBSTRING_INDEX(mytable.s, "d", 1) AS s FROM mytable INNER JOIN othertable ON (SUBSTRING_INDEX(mytable.s, "d", 1) = SUBSTRING_INDEX(othertable.s2, "d", 1)) GROUP BY 1 HAVING s = 'secon';`, Expected: []sql.Row{{"secon"}}, }, + { + Query: `SELECT SUBSTRING_INDEX(mytable.s, "d", 1) AS s FROM mytable INNER JOIN othertable ON (SUBSTRING_INDEX(mytable.s, "d", 1) = SUBSTRING_INDEX(othertable.s2, "d", 1)) GROUP BY s HAVING s = 'secon';`, + Expected: []sql.Row{}, + }, { Query: `SELECT SUBSTRING_INDEX(mytable.s, "d", 1) AS ss FROM mytable INNER JOIN othertable ON (SUBSTRING_INDEX(mytable.s, "d", 1) = SUBSTRING_INDEX(othertable.s2, "d", 1)) GROUP BY s HAVING s = 'secon';`, Expected: []sql.Row{}, diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index 80a857409f..8789ce7d58 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -5277,6 +5277,134 @@ CREATE TABLE tab3 ( }, }, }, + { + Name: "group by having with conflicting aliases test", + SetUpScript: []string{ + "CREATE TABLE tab2(col0 INTEGER, col1 INTEGER, col2 INTEGER);", + "INSERT INTO tab2 VALUES(15,61,87);", + "INSERT INTO tab2 VALUES(91,59,79);", + "INSERT INTO tab2 VALUES(92,41,58);", + }, + Assertions: []ScriptTestAssertion{ + { + Query: `SELECT - col2 AS col0 FROM tab2 GROUP BY col0, col2 HAVING NOT + + col2 <= - col0;`, + Expected: []sql.Row{ + {-87}, + {-79}, + {-58}, + }, + }, + { + Query: `SELECT -col2 AS col0 FROM tab2 GROUP BY col0, col2 HAVING NOT col2 <= - col0;`, + Expected: []sql.Row{ + {-87}, + {-79}, + {-58}, + }, + }, + { + Query: `SELECT -col2 AS col0 FROM tab2 GROUP BY col0, col2 HAVING col2 > -col0;`, + Expected: []sql.Row{ + {-87}, + {-79}, + {-58}, + }, + }, + { + Query: `SELECT 500 * col2 AS col0 FROM tab2 GROUP BY col0, col2 HAVING col2 > -col0;`, + Expected: []sql.Row{ + {43500}, + {39500}, + {29000}, + }, + }, + + { + Query: `select col2-100 as col0 from tab2 group by col0 having col0 > 0;`, + Expected: []sql.Row{ + {-13}, + {-21}, + {-42}, + }, + }, + { + Query: `select col2-100 as col0 from tab2 group by 1 having col0 > 0;`, + Expected: []sql.Row{}, + }, + { + Query: `select col0, count(col0) as c from tab2 group by col0 having c > 0;`, + Expected: []sql.Row{ + {15, 1}, + {91, 1}, + {92, 1}, + }, + }, + { + Query: `SELECT col0 as a FROM tab2 GROUP BY a HAVING col0 = a;`, + Expected: []sql.Row{ + {15}, + {91}, + {92}, + }, + }, + { + Query: `SELECT col0 as a FROM tab2 GROUP BY col0 HAVING col0 = a;`, + Expected: []sql.Row{ + {15}, + {91}, + {92}, + }, + }, + { + Query: `SELECT col0 as a FROM tab2 GROUP BY col0, a HAVING col0 = a;`, + Expected: []sql.Row{ + {15}, + {91}, + {92}, + }, + }, + { + Query: `SELECT col0 as a FROM tab2 HAVING col0 = a;`, + Expected: []sql.Row{ + {15}, + {91}, + {92}, + }, + }, + { + Query: `select col0, (select col1 having col0 > 0) as asdf from tab2 where col0 < 1000;`, + Expected: []sql.Row{ + {15, 61}, + {91, 59}, + {92, 41}, + }, + }, + { + Query: `select col0, sum(col1 * col2) as val from tab2 group by col0 having sum(col1 * col2) > 0;`, + Expected: []sql.Row{ + {15, 5307.0}, + {91, 4661.0}, + {92, 2378.0}, + }, + }, + { + Query: `SELECT col0+1 as a FROM tab2 HAVING col0 = a;`, + ExpectedErr: sql.ErrColumnNotFound, + }, + { + Query: `select col2-100 as asdf from tab2 group by 1 having col0 > 0;`, + ExpectedErr: sql.ErrColumnNotFound, + }, + { + Query: `SELECT -col2 AS col0 FROM tab2 HAVING col2 > -col0;`, + ExpectedErr: sql.ErrColumnNotFound, + }, + { + Query: `insert into tab2(col2) select sin(col2) from tab2 group by 1 having col2 > 1;`, + ExpectedErr: sql.ErrColumnNotFound, + }, + }, + }, } var SpatialScriptTests = []ScriptTest{ diff --git a/sql/planbuilder/aggregates.go b/sql/planbuilder/aggregates.go index 956e6b04c8..f395050b6b 100644 --- a/sql/planbuilder/aggregates.go +++ b/sql/planbuilder/aggregates.go @@ -763,18 +763,19 @@ func (b *Builder) analyzeHaving(fromScope, projScope *scope, having *ast.Where) dbName := strings.ToLower(n.Qualifier.Qualifier.String()) tblName := strings.ToLower(n.Qualifier.Name.String()) colName := strings.ToLower(n.Name.String()) - c, ok := projScope.resolveColumn(dbName, tblName, colName, false, true) + c, ok := fromScope.resolveColumn(dbName, tblName, colName, true, false) if ok { - // references projection alias + c.scalar = expression.NewGetFieldWithTable(int(c.id), 0, c.typ, c.db, c.table, c.col, c.nullable) + fromScope.addExtraColumn(c) break } - c, ok = fromScope.resolveColumn(dbName, tblName, colName, true, false) - if !ok { - err := sql.ErrColumnNotFound.New(n.Name) - b.handleErr(err) + c, ok = projScope.resolveColumn(dbName, tblName, colName, false, true) + if ok { + // references projection alias + break } - c.scalar = expression.NewGetFieldWithTable(int(c.id), 0, c.typ, c.db, c.table, c.col, c.nullable) - fromScope.addExtraColumn(c) + err := sql.ErrColumnNotFound.New(n.Name) + b.handleErr(err) } return true, nil }, having.Expr) @@ -804,6 +805,16 @@ func (b *Builder) buildInnerProj(fromScope, projScope *scope) *scope { return outScope } +// getMatchingCol returns the column in cols that matches the name, if it exists +func getMatchingCol(cols []scopeColumn, name string) (scopeColumn, bool) { + for _, c := range cols { + if strings.EqualFold(c.col, name) { + return c, true + } + } + return scopeColumn{}, false +} + func (b *Builder) buildHaving(fromScope, projScope, outScope *scope, having *ast.Where) { // expressions in having can be from aggOut or projScop if having == nil { @@ -812,67 +823,53 @@ func (b *Builder) buildHaving(fromScope, projScope, outScope *scope, having *ast if fromScope.groupBy == nil { fromScope.initGroupBy() } - // Having specifies conditions on groups. If not group by is present, all rows implicitly form a single aggregate group. - havingScope := fromScope.push() // TODO: we should not be including the entire fromScope - - for _, c := range projScope.cols { - // if a projection overrides a column used in an aggregation, don't use projection - found := false - if fromScope.groupBy != nil && fromScope.groupBy.hasAggs() { - for _, cc := range fromScope.groupBy.aggregations() { - transform.InspectExpr(cc.scalar, func(e sql.Expression) bool { - switch e := e.(type) { - case *expression.GetField: - if strings.EqualFold(c.col, e.Name()) { - found = true - havingScope.addColumn(cc) - return true - } - default: - } - return false - }) - } - } - if found { - continue - } - - alias, isAlias := c.scalar.(*expression.Alias) - if !isAlias { - continue - } - // Aliased GetFields are allowed in having clauses regardless of weather they are in the group by - _, isGetField := alias.Child.(*expression.GetField) - if isGetField { - havingScope.addColumn(c) - continue - } + havingScope := b.newScope() + if fromScope.parent != nil { + havingScope.parent = fromScope.parent + } - // Aliased expression is allowed if there is no group by (it is implicitly a single group) - if len(fromScope.groupBy.inCols) == 0 { + // add columns from fromScope referenced in the groupBy + for _, c := range fromScope.groupBy.inCols { + if !havingScope.colset.Contains(sql.ColumnId(c.id)) { havingScope.addColumn(c) - continue } + } - // Aliased expressions are allowed in having clauses if they are in the group by - for _, cc := range fromScope.groupBy.inCols { - if c.col == cc.col { - havingScope.addColumn(c) - found = true - break + // add columns from fromScope referenced in any aggregate expressions + for _, c := range fromScope.groupBy.aggregations() { + transform.InspectExpr(c.scalar, func(e sql.Expression) bool { + switch e := e.(type) { + case *expression.GetField: + col, found := getMatchingCol(fromScope.cols, e.Name()) + if found && !havingScope.colset.Contains(sql.ColumnId(col.id)) { + havingScope.addColumn(col) + } } - } + return false + }) + } - if found { + // Add columns from projScope referenced in any aggregate expressions, that are not already in the havingScope + // This prevents aliases with the same name from overriding columns in the fromScope + // Additionally, the original name from plain aliases (not expressions) are added to havingScope + for _, c := range projScope.cols { + if !havingScope.colset.Contains(sql.ColumnId(c.id)) { + havingScope.addColumn(c) + } + // The unaliased column is allowed in having clauses regardless if it is just an aliased getfield and not an expression + alias, isAlias := c.scalar.(*expression.Alias) + if !isAlias { continue } - - if c.tableId.IsEmpty() { - havingScope.addColumn(c) + gf, isGetField := alias.Child.(*expression.GetField) + if !isGetField { continue } + col, found := getMatchingCol(fromScope.cols, gf.Name()) + if found && !havingScope.colset.Contains(sql.ColumnId(col.id)) { + havingScope.addColumn(col) + } } havingScope.groupBy = fromScope.groupBy diff --git a/sql/planbuilder/parse_test.go b/sql/planbuilder/parse_test.go index 225e51db66..14799dcbad 100644 --- a/sql/planbuilder/parse_test.go +++ b/sql/planbuilder/parse_test.go @@ -82,9 +82,9 @@ Project │ ├─ avg(cte.x):4 │ └─ 0 (tinyint) └─ Project - ├─ columns: [avg(cte.x):4, 1 (tinyint) as x] + ├─ columns: [avg(cte.x):4, cte.x:2!null, 1 (tinyint) as x] └─ GroupBy - ├─ select: AVG(cte.x:2!null) + ├─ select: AVG(cte.x:2!null), cte.x:2!null ├─ group: └─ SubqueryAlias ├─ name: cte @@ -112,9 +112,9 @@ Project │ ├─ avg(xy.x):5 │ └─ 0 (tinyint) └─ Project - ├─ columns: [avg(xy.x):5, 1 (tinyint) as x] + ├─ columns: [avg(xy.x):5, xy.x:1!null, 1 (tinyint) as x] └─ GroupBy - ├─ select: AVG(xy.x:1!null) + ├─ select: AVG(xy.x:1!null), xy.x:1!null ├─ group: └─ Table ├─ name: xy @@ -636,24 +636,6 @@ Project ├─ columns: [x y z] ├─ colSet: (1-3) └─ tableId: 1 -`, - }, - { - Query: "select x from xy having z > 0", - ExpectedPlan: ` -Project - ├─ columns: [xy.x:1!null] - └─ Having - ├─ GreaterThan - │ ├─ xy.z:3!null - │ └─ 0 (tinyint) - └─ Project - ├─ columns: [xy.x:1!null, xy.y:2!null, xy.z:3!null] - └─ Table - ├─ name: xy - ├─ columns: [x y z] - ├─ colSet: (1-3) - └─ tableId: 1 `, }, { @@ -669,25 +651,6 @@ Project ├─ columns: [x y z] ├─ colSet: (1-3) └─ tableId: 1 -`, - }, - { - Query: "select x from xy having z > 0 order by y", - ExpectedPlan: ` -Project - ├─ columns: [xy.x:1!null] - └─ Sort(xy.y:2!null ASC nullsFirst) - └─ Having - ├─ GreaterThan - │ ├─ xy.z:3!null - │ └─ 0 (tinyint) - └─ Project - ├─ columns: [xy.x:1!null, xy.y:2!null, xy.z:3!null] - └─ Table - ├─ name: xy - ├─ columns: [x y z] - ├─ colSet: (1-3) - └─ tableId: 1 `, }, { @@ -901,17 +864,17 @@ Project `, }, { - Query: "SELECT x, sum(x) FROM xy group by 1 having avg(y) > 1 order by 1", + Query: "SELECT x, sum(x) FROM xy group by 1 having avg(x) > 1 order by 2", ExpectedPlan: ` Project ├─ columns: [xy.x:1!null, sum(xy.x):4!null as sum(x)] - └─ Sort(xy.x:1!null ASC nullsFirst) + └─ Sort(sum(xy.x):4!null as sum(x) ASC nullsFirst) └─ Having ├─ GreaterThan - │ ├─ avg(xy.y):5 + │ ├─ avg(xy.x):5 │ └─ 1 (tinyint) └─ GroupBy - ├─ select: AVG(xy.y:2!null), SUM(xy.x:1!null), xy.x:1!null, xy.y:2!null + ├─ select: AVG(xy.x:1!null), SUM(xy.x:1!null), xy.x:1!null ├─ group: xy.x:1!null └─ Table ├─ name: xy @@ -921,23 +884,22 @@ Project `, }, { - Query: "SELECT x, sum(x) FROM xy group by 1 having avg(x) > 1 order by 2", + Query: "SELECT x, sum(y * z) FROM xy group by x having sum(y * z) > 1", ExpectedPlan: ` Project - ├─ columns: [xy.x:1!null, sum(xy.x):4!null as sum(x)] - └─ Sort(sum(xy.x):4!null as sum(x) ASC nullsFirst) - └─ Having - ├─ GreaterThan - │ ├─ avg(xy.x):5 - │ └─ 1 (tinyint) - └─ GroupBy - ├─ select: AVG(xy.x:1!null), SUM(xy.x:1!null), xy.x:1!null - ├─ group: xy.x:1!null - └─ Table - ├─ name: xy - ├─ columns: [x y z] - ├─ colSet: (1-3) - └─ tableId: 1 + ├─ columns: [xy.x:1!null, sum((xy.y * xy.z)):4!null as sum(y * z)] + └─ Having + ├─ GreaterThan + │ ├─ sum((xy.y * xy.z)):4!null + │ └─ 1 (tinyint) + └─ GroupBy + ├─ select: SUM((xy.y:2!null * xy.z:3!null)), xy.x:1!null, xy.y:2!null, xy.z:3!null + ├─ group: xy.x:1!null + └─ Table + ├─ name: xy + ├─ columns: [x y z] + ├─ colSet: (1-3) + └─ tableId: 1 `, }, { @@ -981,24 +943,6 @@ Project ├─ columns: [x y z] ├─ colSet: (1-3) └─ tableId: 1 -`, - }, - { - Query: "SELECT x, sum(x) FROM xy group by 1 having x+y order by 1", - ExpectedPlan: ` -Project - ├─ columns: [xy.x:1!null, sum(xy.x):4!null as sum(x)] - └─ Sort(xy.x:1!null ASC nullsFirst) - └─ Having - ├─ (xy.x:1!null + xy.y:2!null) - └─ GroupBy - ├─ select: SUM(xy.x:1!null), xy.x:1!null, xy.y:2!null - ├─ group: xy.x:1!null - └─ Table - ├─ name: xy - ├─ columns: [x y z] - ├─ colSet: (1-3) - └─ tableId: 1 `, }, { @@ -1591,7 +1535,7 @@ Project ├─ columns: [xy.x:1!null, count(xy.x):4!null as cnt] └─ Having ├─ GreaterThan - │ ├─ xy.x:1!null + │ ├─ xy.x:0!null │ └─ 1 (tinyint) └─ Project ├─ columns: [count(xy.x):4!null, xy.x:1!null, count(xy.x):4!null as cnt] @@ -2098,9 +2042,9 @@ Project ├─ NOT │ └─ avg(-xy.y):5 IS NULL └─ Project - ├─ columns: [avg(-xy.y):5, xy.x:1!null, xy.x:1!null as y] + ├─ columns: [avg(-xy.y):5, xy.x:1!null, xy.y:2!null, xy.x:1!null as y] └─ GroupBy - ├─ select: AVG(-xy.y), xy.x:1!null + ├─ select: AVG(-xy.y), xy.x:1!null, xy.y:2!null ├─ group: xy.x:1!null └─ Table ├─ name: xy @@ -2227,12 +2171,106 @@ Project `, }, { - Skip: true, - Query: "select x + 1 as xx from xy group by xx having x = 123; -- should error", + Query: "select x as xx from xy group by x having x = xx;", + ExpectedPlan: ` +Project + ├─ columns: [xy.x:1!null as xx] + └─ Having + ├─ Eq + │ ├─ xy.x:1!null + │ └─ xx:4!null + └─ Project + ├─ columns: [xy.x:1!null, xy.x:1!null as xx] + └─ GroupBy + ├─ select: xy.x:1!null + ├─ group: xy.x:1!null + └─ Table + ├─ name: xy + ├─ columns: [x y z] + ├─ colSet: (1-3) + └─ tableId: 1 +`, }, { - Skip: true, - Query: "select x + 1 as xx from xy having x = 123; -- should error", + Query: "select x as xx from xy group by xx having x = xx;", + ExpectedPlan: ` +Project + ├─ columns: [xy.x:1!null as xx] + └─ Having + ├─ Eq + │ ├─ xy.x:1!null + │ └─ xx:4!null + └─ Project + ├─ columns: [xy.x:1!null, xy.x:1!null as xx] + └─ GroupBy + ├─ select: xy.x:1!null + ├─ group: xy.x:1!null as xx + └─ Table + ├─ name: xy + ├─ columns: [x y z] + ├─ colSet: (1-3) + └─ tableId: 1 +`, + }, + { + Query: "select x as xx from xy group by x, xx having x = xx;", + ExpectedPlan: ` +Project + ├─ columns: [xy.x:1!null as xx] + └─ Having + ├─ Eq + │ ├─ xy.x:1!null + │ └─ xx:4!null + └─ Project + ├─ columns: [xy.x:1!null, xy.x:1!null as xx] + └─ GroupBy + ├─ select: xy.x:1!null + ├─ group: xy.x:1!null, xy.x:1!null as xx + └─ Table + ├─ name: xy + ├─ columns: [x y z] + ├─ colSet: (1-3) + └─ tableId: 1 +`, + }, + { + Query: "select x as xx from xy having x = xx;", + ExpectedPlan: ` +Project + ├─ columns: [xy.x:1!null as xx] + └─ Having + ├─ Eq + │ ├─ xy.x:1!null + │ └─ xx:4!null + └─ Project + ├─ columns: [xy.x:1!null, xy.y:2!null, xy.z:3!null, xy.x:1!null as xx] + └─ Table + ├─ name: xy + ├─ columns: [x y z] + ├─ colSet: (1-3) + └─ tableId: 1 +`, + }, + { + Query: "select -x as y from xy group by x, y having -x > y;", + ExpectedPlan: ` +Project + ├─ columns: [-xy.x as y] + └─ Having + ├─ GreaterThan + │ ├─ -xy.x + │ └─ xy.y:2!null + └─ Project + ├─ columns: [xy.x:1!null, xy.y:2!null, -xy.x as y] + └─ GroupBy + ├─ select: xy.x:1!null, xy.y:2!null + ├─ group: xy.x:1!null, xy.y:2!null + └─ Table + ├─ name: xy + ├─ columns: [x y z] + ├─ colSet: (1-3) + └─ tableId: 1 +`, }, { Query: "select x as xx from xy join uv on (x = u) group by xx having xx = 123;", @@ -2405,14 +2443,6 @@ Project └─ tableId: 2 `, }, - { - Skip: true, - Query: "select x + 1 as xx from xy join uv on (x = u) group by xx having x = 123; -- should error", - }, - { - Skip: true, - Query: "select x + 1 as xx from xy join uv on (x = u) having x = 123; -- should error", - }, { Query: "select x +1 as xx from xy join uv on (x = u) group by x having avg(x) = 123;", ExpectedPlan: ` @@ -2807,6 +2837,26 @@ func TestPlanBuilderErr(t *testing.T) { Query: "select x, y as x from xy order by x;", Err: "ambiguous column or alias name \"x\"", }, + { + Query: "select x from xy having z > 0", + Err: "column \"z\" could not be found in any table in scope", + }, + { + Query: "select x from xy having z > 0 order by y", + Err: "column \"z\" could not be found in any table in scope", + }, + { + Query: "SELECT x, sum(x) FROM xy group by 1 having x+y order by 1", + Err: "column \"y\" could not be found in any table in scope", + }, + { + Query: "select x + 1 as xx from xy join uv on (x = u) group by xx having x = 123;", + Err: "column \"x\" could not be found in any table in scope", + }, + { + Query: "select x + 1 as xx from xy join uv on (x = u) having x = 123;", + Err: "column \"x\" could not be found in any table in scope", + }, } db := memory.NewDatabase("mydb") diff --git a/sql/rowexec/agg.go b/sql/rowexec/agg.go index 91334be11b..8d470de283 100644 --- a/sql/rowexec/agg.go +++ b/sql/rowexec/agg.go @@ -91,7 +91,11 @@ func (i *groupByIter) Next(ctx *sql.Context) (sql.Row, error) { } } - return evalBuffers(ctx, i.buf) + row, err := evalBuffers(ctx, i.buf) + if err != nil { + return nil, err + } + return row, nil } func (i *groupByIter) Close(ctx *sql.Context) error { @@ -145,7 +149,13 @@ func (i *groupByGroupingIter) Next(ctx *sql.Context) (sql.Row, error) { return nil, err } i.pos++ - return evalBuffers(ctx, buffers) + + row, err := evalBuffers(ctx, buffers) + if err != nil { + return nil, err + } + + return row, nil } func (i *groupByGroupingIter) compute(ctx *sql.Context) error {