diff --git a/src/query/expr/parser_test.go b/src/query/expr/parser_test.go index f667226..898f674 100644 --- a/src/query/expr/parser_test.go +++ b/src/query/expr/parser_test.go @@ -545,6 +545,15 @@ func TestParsingSQL(t *testing.T) { {"SELECT foo FROM bar GROUP BY foo ORDER BY foo ASC NULLS LAST, bar DESC NULLS FIRST", nil}, {"SELECT foo FROM bar GROUP BY foo ORDER BY foo ASC NULLS LAST, bar DESC NULLS FIRST LIMIT 3", nil}, + // GROUP BY number + {"SELECT foo FROM bar GROUP BY 1", nil}, + {"SELECT foo, baz FROM bar GROUP BY 1, 2", nil}, + // ARCH: all those NULLS LAST/FIRST are due to roundtrip testing + {"SELECT foo, baz FROM bar ORDER BY 1 ASC NULLS LAST", nil}, + {"SELECT foo, baz FROM bar ORDER BY 1 ASC NULLS LAST, 2 ASC NULLS LAST", nil}, + {"SELECT foo, baz FROM bar ORDER BY 1 DESC NULLS FIRST, 2 ASC NULLS LAST", nil}, + {"SELECT foo, baz FROM bar ORDER BY 1 ASC NULLS LAST, 2 DESC NULLS LAST", nil}, + {"SELECT foo FROM bar@234", errInvalidQuery}, {"SELECT foo FROM bar GROUP for 1", errInvalidQuery}, {"SELECT foo FROM bar GROUP BY foo LIMIT foo", errInvalidQuery}, diff --git a/src/query/expr/types.go b/src/query/expr/types.go index 468e32b..eccd3d8 100644 --- a/src/query/expr/types.go +++ b/src/query/expr/types.go @@ -81,6 +81,12 @@ type Integer struct { value int64 } +// TODO/ARCH: doing this to avoid too many changes across the codebase, consider +// exporting the field +func (ex *Integer) Value() int64 { + return ex.value +} + func (ex *Integer) ReturnType(ts column.TableSchema) (column.Schema, error) { return column.Schema{ Name: ex.String(), diff --git a/src/query/query.go b/src/query/query.go index d3af688..6fb1d5c 100644 --- a/src/query/query.go +++ b/src/query/query.go @@ -18,6 +18,7 @@ var errInvalidLimitValue = errors.New("invalid limit value") var errInvalidProjectionInAggregation = errors.New("selections in aggregating expressions need to be either the group by clauses or aggregating expressions (e.g. sum(foo))") var errInvalidFilter = errors.New("invalid WHERE clause") var errInvalidOrderClause = errors.New("invalid ORDER BY clause") +var errInvalidGroupbyClause = errors.New("invalid GROUP BY clause") var errQueryNoDatasetIdentifiers = errors.New("query without a dataset has identifiers in the SELECT clause") // Result holds the result of a query, at this point it's fairly literal - in the future we may want @@ -386,6 +387,13 @@ func reorder(res *Result, q expr.Query) error { nullsFirst = oby.NullsFirst needle = oby.Children()[0] } + // TODO/ARCH: I wanted to change q.Order in place... but we can't create a new Ordering, + // because `.inner` is private and I didn't want to expose it. But it might be the right way to go + // TODO: test this properly (we have parsing tests, not implementation testing) + if idx, ok := needle.(*expr.Integer); ok { + needle = q.Select[idx.Value()-1] // no need to validate any more, already did that + } + pos := lookupExpr(needle, q.Select) if pos == -1 { return fmt.Errorf("cannot sort by a column not in projections: %s", needle) @@ -506,6 +514,15 @@ func Run(db *database.Database, q expr.Query) (*Result, error) { proj = wrapped.Children()[0] } + // ORDER BY 1, 2 + if idx, ok := proj.(*expr.Integer); ok { + n := idx.Value() + if n < 1 || n > int64(len(q.Select)) { + return nil, errInvalidOrderClause + } + continue + } + posS := lookupExpr(proj, q.Select) posG := -1 if q.Aggregate != nil { @@ -528,6 +545,17 @@ func Run(db *database.Database, q expr.Query) (*Result, error) { limit = *q.Limit } if q.Aggregate != nil || allAggregations { + // edit GROUP BY 1, 2 in place (replace them by their respective columns) + for j, agg := range q.Aggregate { + if idx, ok := agg.(*expr.Integer); ok { + n := idx.Value() + if n < 1 || n > int64(len(q.Select)) { + return nil, errInvalidGroupbyClause + } + q.Aggregate[j] = q.Select[n-1] + } + } + if err := aggregate(db, ds, res, q); err != nil { return nil, err } diff --git a/src/query/query_test.go b/src/query/query_test.go index 9370e7a..53f27a2 100644 --- a/src/query/query_test.go +++ b/src/query/query_test.go @@ -384,6 +384,13 @@ func TestQuerySetup(t *testing.T) { {"SELECT bar FROM dataset GROUP BY foo", errInvalidProjectionInAggregation}, {"SELECT foo FROM dataset GROUP BY nullif(foo, 2)", errInvalidProjectionInAggregation}, {"SELECT foo FROM dataset ORDER by FOO", nil}, + {"SELECT foo FROM dataset ORDER by 1", nil}, + {"SELECT foo, bar FROM dataset ORDER by 1, 2", nil}, + {"SELECT * FROM dataset ORDER by 1, 2", nil}, + {"SELECT * FROM dataset ORDER by 1, bar", nil}, + {"SELECT foo, bar FROM dataset ORDER by 0, 1", errInvalidOrderClause}, // underflow + {"SELECT foo, bar FROM dataset ORDER by 2, 3", errInvalidOrderClause}, // overflow + {"SELECT * FROM dataset ORDER by 1, 2, 4", errInvalidOrderClause}, // overflow {"SELECT foo FROM dataset ORDER by bar", errInvalidOrderClause}, {"SELECT foo FROM dataset ORDER by foo, bar", errInvalidOrderClause}, {"SELECT foo FROM dataset LIMIT 0", nil}, // cannot test -2, because that fails with a parser error @@ -401,6 +408,13 @@ func TestQuerySetup(t *testing.T) { // we get a parser issue, because we can get multiple where clauses only in JSON unmarshaling of queries // {"SELECT foo FROM dataset WHERE foo > 0, foo < 3", errInvalidFilter}, + {"SELECT * FROM dataset GROUP BY 1, 2, 3", nil}, + {"SELECT foo, bar FROM dataset GROUP BY 1, 2", nil}, + {"SELECT foo, bar FROM dataset GROUP BY 1, bar", nil}, + {"SELECT foo, bar FROM dataset GROUP BY 0, 1", errInvalidGroupbyClause}, // underflow + {"SELECT foo, bar FROM dataset GROUP BY 1, 3", errInvalidGroupbyClause}, // overflow + {"SELECT * FROM dataset GROUP BY 1, 4", errInvalidGroupbyClause}, // overflow + // relabeling can be tricky, especially when looking up columns across parts of the query - these are all legal // BUT this doesn't work the same for WHERE clauses - we cannot filter on relabeled fields {"SELECT foo AS bar FROM dataset GROUP BY foo", nil},