Skip to content

fix group by ... having ... aliasing once more #2311

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.

Sign up for GitHub

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

Merged
jycor merged 5 commits into from
Feb 5, 2024
Merged

fix group by ... having ... aliasing once more #2311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42af47d
fix group by aliasing once more
Feb 3, 2024
c6d5467
[ga-format-pr] Run ./format_repo.sh to fix formatting
jycor Feb 3, 2024
f65047b
regen plans
Feb 3, 2024
02293a3
adding tests
Feb 4, 2024
81aa911
remove rewrite
Feb 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions enginetest/queries/insert_queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
4 changes: 4 additions & 0 deletions enginetest/queries/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand Down
128 changes: 128 additions & 0 deletions enginetest/queries/script_queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;`,
Copy link
Copy Markdown
Contributor

@max-hoffman max-hoffman Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would any subset of these be useful planbuilder tests that verify refs get the right definition id?

Copy link
Copy Markdown
Contributor Author

@jycor jycor Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some of these to planbuilder

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{
Expand Down
113 changes: 55 additions & 58 deletions sql/planbuilder/aggregates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
Comment thread
jycor marked this conversation as resolved.
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
Expand Down
Loading