Vinai/group concat #366
Vinai/group concat #366
Conversation
sql/analyzer/validation_rules.go
Outdated
| @@ -80,7 +80,7 @@ var ( | |||
| // ErrSubqueryMultipleColumns is returned when an expression subquery returns | |||
| // more than a single column. | |||
| ErrSubqueryMultipleColumns = errors.NewKind( | |||
| "subquery expressions can only return a single column", | |||
| "operand contains more than one column", | |||
There was a problem hiding this comment.
I think we should create a new package for errors. That way errors like these can be properly associated with their MySQL code.
There was a problem hiding this comment.
If an error is user facing, put it in the sql package in errors.go
There was a problem hiding this comment.
There are a bunch of places that break this rule in the analyzer, fix them when you find them
| Expected: []sql.Row{{"2"}}, | ||
| }, | ||
| { | ||
| Query: `SELECT group_concat(DISTINCT attribute ORDER BY attribute ASC) FROM t`, |
There was a problem hiding this comment.
You need a few test queries with WHERE clauses as well
sql/analyzer/validation_rules.go
Outdated
| @@ -80,7 +80,7 @@ var ( | |||
| // ErrSubqueryMultipleColumns is returned when an expression subquery returns | |||
| // more than a single column. | |||
| ErrSubqueryMultipleColumns = errors.NewKind( | |||
| "subquery expressions can only return a single column", | |||
| "operand contains more than one column", | |||
There was a problem hiding this comment.
If an error is user facing, put it in the sql package in errors.go
sql/analyzer/validation_rules.go
Outdated
| @@ -80,7 +80,7 @@ var ( | |||
| // ErrSubqueryMultipleColumns is returned when an expression subquery returns | |||
| // more than a single column. | |||
| ErrSubqueryMultipleColumns = errors.NewKind( | |||
| "subquery expressions can only return a single column", | |||
| "operand contains more than one column", | |||
There was a problem hiding this comment.
There are a bunch of places that break this rule in the analyzer, fix them when you find them
|
|
||
| sb := strings.Builder{} | ||
| for i, row := range rows { | ||
| lastIdx := len(row) - 1 |
There was a problem hiding this comment.
This is called the fencepost problem, and it's usually solved in the opposite way: print the separator unless this is the first element. Then print the element.
| } | ||
| ret := sb.String() | ||
|
|
||
| if len(ret) > g.maxLen { |
There was a problem hiding this comment.
Want to do this during the loop (and exit early), rather than at the end. Otherwise you blow up memory for no reason
| } | ||
|
|
||
| func (g *GroupConcat) Resolved() bool { | ||
| for _, se := range g.selectExprs { |
There was a problem hiding this comment.
What about the sort fields? They might not be resolved
| } | ||
|
|
||
| func (g *GroupConcat) Children() []sql.Expression { | ||
| arr := g.sf.ToExpressions() |
| } | ||
|
|
||
| // Get the order by expression using the length of the sort fields. | ||
| delim := len(g.sf) |
There was a problem hiding this comment.
delim is a bad name here, considering the context
This PR adds support for the Group_Concat Aggregation Function