Repository: src-d/go-mysql-server
Pull request #774 (2019-06-27 18:42:20)

Limit / offset bug fixes
Prior to this PR, the following query returns no rows on a table with many:

SELECT i FROM mytable LIMIT 1 OFFSET 1;

In fact, no queries combining offset and limit return correct results before this PR.

This is because the parser creates an offset node that wraps a limit node, and the limit node's counter is incremented for every row the offset node skips, so that when the offset node is done skipping rows, the wrapped limit node returns fewer rows than asked for.

The fix is to add the offset to the row count when constructing the limit node. This is much simpler than adding ResetCount() or similar, but has the disadvantage of making the plan slightly more difficult to understand to a naive reader, best illustrated by this modified test, where the limit node has a limit of 7 instead of 2 as expressed in the query:

	`SELECT foo, bar FROM foo LIMIT 2 OFFSET 5;`: 
         plan.NewOffset(5,
		plan.NewLimit(7, plan.NewProject(
			[]sql.Expression{
				expression.NewUnresolvedColumn("foo"),
				expression.NewUnresolvedColumn("bar"),
			},
			plan.NewUnresolvedTable("foo", ""),
		)),
	),

Also included in this PR, queries with negative limits or offsets now return an error.


Patch:
diff --git a/sql/parse/parse.go b/sql/parse/parse.go
index c8a69c1..b8a82e8 100644
--- a/sql/parse/parse.go
+++ b/sql/parse/parse.go
@@ -9,7 +9,7 @@ import (
 	"strconv"
 	"strings"
 
-	opentracing "github.com/opentracing/opentracing-go"
+	"github.com/opentracing/opentracing-go"
 	"github.com/src-d/go-mysql-server/sql"
 	"github.com/src-d/go-mysql-server/sql/expression"
 	"github.com/src-d/go-mysql-server/sql/expression/function"
@@ -32,7 +32,7 @@ var (
 	ErrInvalidSQLValType = errors.NewKind("invalid SQLVal of type: %d")
 
 	// ErrInvalidSortOrder is returned when a sort order is not valid.
-	ErrInvalidSortOrder = errors.NewKind("invalod sort order: %s")
+	ErrInvalidSortOrder = errors.NewKind("invalid sort order: %s")
 )
 
 var (
@@ -321,21 +321,22 @@ func convertSelect(ctx *sql.Context, s *sqlparser.Select) (sql.Node, error) {
 		}
 	}
 
-	if s.Limit != nil {
-		node, err = limitToLimit(ctx, s.Limit.Rowcount, node)
+	// Limit must wrap offset, and not vice-versa, so that skipped rows don't count toward the returned row count.
+	if s.Limit != nil && s.Limit.Offset != nil {
+		node, err = offsetToOffset(ctx, s.Limit.Offset, node)
 		if err != nil {
 			return nil, err
 		}
-	} else if ok, val := sql.HasDefaultValue(ctx.Session, "sql_select_limit"); !ok {
-		limit := val.(int64)
-		node = plan.NewLimit(int64(limit), node)
 	}
 
-	if s.Limit != nil && s.Limit.Offset != nil {
-		node, err = offsetToOffset(ctx, s.Limit.Offset, node)
+	if s.Limit != nil {
+		node, err = limitToLimit(ctx, s.Limit.Rowcount, node)
 		if err != nil {
 			return nil, err
 		}
+	} else if ok, val := sql.HasDefaultValue(ctx.Session, "sql_select_limit"); !ok {
+		limit := val.(int64)
+		node = plan.NewLimit(int64(limit), node)
 	}
 
 	return node, nil
@@ -585,21 +586,16 @@ func limitToLimit(
 	limit sqlparser.Expr,
 	child sql.Node,
 ) (*plan.Limit, error) {
-	e, err := exprToExpression(limit)
+	rowCount, err := getInt64Value(ctx, limit, "LIMIT with non-integer literal")
 	if err != nil {
 		return nil, err
 	}
 
-	nl, ok := e.(*expression.Literal)
-	if !ok || nl.Type() != sql.Int64 {
-		return nil, ErrUnsupportedFeature.New("LIMIT with non-integer literal")
+	if rowCount < 0 {
+		return nil, ErrUnsupportedSyntax.New("LIMIT must be >= 0")
 	}
 
-	n, err := nl.Eval(ctx, nil)
-	if err != nil {
-		return nil, err
-	}
-	return plan.NewLimit(n.(int64), child), nil
+	return plan.NewLimit(rowCount, child), nil
 }
 
 func havingToHaving(having *sqlparser.Where, node sql.Node) (sql.Node, error) {
@@ -616,21 +612,48 @@ func offsetToOffset(
 	offset sqlparser.Expr,
 	child sql.Node,
 ) (*plan.Offset, error) {
-	e, err := exprToExpression(offset)
+	o, err := getInt64Value(ctx, offset, "OFFSET with non-integer literal")
+	if err != nil {
+		return nil, err
+	}
+
+	if o < 0 {
+		return nil, ErrUnsupportedSyntax.New("OFFSET must be >= 0")
+	}
+
+	return plan.NewOffset(o, child), nil
+}
+
+// getInt64Literal returns an int64 *expression.Literal for the value given, or an unsupported error with the string
+// given if the expression doesn't represent an integer literal.
+func getInt64Literal(expr sqlparser.Expr, errStr string) (*expression.Literal, error) {
+	e, err := exprToExpression(expr)
 	if err != nil {
 		return nil, err
 	}
 
 	nl, ok := e.(*expression.Literal)
 	if !ok || nl.Type() != sql.Int64 {
-		return nil, ErrUnsupportedFeature.New("OFFSET with non-integer literal")
+		return nil, ErrUnsupportedFeature.New(errStr)
+	} else {
+		return nl, nil
 	}
+}
 
-	n, err := nl.Eval(ctx, nil)
+// getInt64Value returns the int64 literal value in the expression given, or an error with the errStr given if it
+// cannot.
+func getInt64Value(ctx *sql.Context, expr sqlparser.Expr, errStr string) (int64, error) {
+	ie, err := getInt64Literal(expr, errStr)
 	if err != nil {
-		return nil, err
+		return 0, err
 	}
-	return plan.NewOffset(n.(int64), child), nil
+
+	i, err := ie.Eval(ctx, nil)
+	if err != nil {
+		return 0, err
+	}
+
+	return i.(int64), nil
 }
 
 func isAggregate(e sql.Expression) bool {


Test patch:
diff --git a/engine_test.go b/engine_test.go
index c7479a2..0ead613 100644
--- a/engine_test.go
+++ b/engine_test.go
@@ -48,6 +48,22 @@ var queries = []struct {
 		"SELECT i FROM mytable WHERE s = 'first row' ORDER BY i DESC LIMIT 1;",
 		[]sql.Row{{int64(1)}},
 	},
+	{
+		"SELECT i FROM mytable ORDER BY i LIMIT 1 OFFSET 1;",
+		[]sql.Row{{int64(2)}},
+	},
+	{
+		"SELECT i FROM mytable ORDER BY i LIMIT 1,1;",
+		[]sql.Row{{int64(2)}},
+	},
+	{
+		"SELECT i FROM mytable ORDER BY i LIMIT 3,1;",
+		nil,
+	},
+	{
+		"SELECT i FROM mytable ORDER BY i LIMIT 2,100;",
+		[]sql.Row{{int64(3)}},
+	},
 	{
 		"SELECT COUNT(*) FROM mytable;",
 		[]sql.Row{{int64(3)}},
@@ -1969,8 +1985,8 @@ func newEngineWithParallelism(t *testing.T, parallelism int) *sqle.Engine {
 	return sqle.New(catalog, a, new(sqle.Config))
 }
 
-const expectedTree = `Offset(2)
- └─ Limit(5)
+const expectedTree = `Limit(5)
+ └─ Offset(2)
      └─ Project(t.foo, bar.baz)
          └─ Filter(foo > qux)
              └─ InnerJoin(foo = baz)
diff --git a/sql/parse/parse_test.go b/sql/parse/parse_test.go
index 80c5a59..09b6938 100644
--- a/sql/parse/parse_test.go
+++ b/sql/parse/parse_test.go
@@ -5,7 +5,7 @@ import (
 
 	"github.com/src-d/go-mysql-server/sql/expression"
 	"github.com/src-d/go-mysql-server/sql/plan"
-	errors "gopkg.in/src-d/go-errors.v1"
+	"gopkg.in/src-d/go-errors.v1"
 
 	"github.com/src-d/go-mysql-server/sql"
 	"github.com/stretchr/testify/require"
@@ -309,8 +309,17 @@ var fixtures = map[string]sql.Node{
 		},
 		plan.NewUnresolvedTable("foo", ""),
 	),
-	`SELECT foo, bar FROM foo LIMIT 2 OFFSET 5;`: plan.NewOffset(5,
-		plan.NewLimit(2, plan.NewProject(
+	`SELECT foo, bar FROM foo LIMIT 2 OFFSET 5;`: plan.NewLimit(2,
+		plan.NewOffset(5, plan.NewProject(
+			[]sql.Expression{
+				expression.NewUnresolvedColumn("foo"),
+				expression.NewUnresolvedColumn("bar"),
+			},
+			plan.NewUnresolvedTable("foo", ""),
+		)),
+	),
+	`SELECT foo, bar FROM foo LIMIT 5,2;`: plan.NewLimit(2,
+		plan.NewOffset(5, plan.NewProject(
 			[]sql.Expression{
 				expression.NewUnresolvedColumn("foo"),
 				expression.NewUnresolvedColumn("bar"),
@@ -1143,7 +1152,7 @@ func TestParse(t *testing.T) {
 			require := require.New(t)
 			ctx := sql.NewEmptyContext()
 			p, err := Parse(ctx, query)
-			require.Nil(err, "error for query '%s'", query)
+			require.NoError(err)
 			require.Exactly(expectedPlan, p,
 				"plans do not match for query '%s'", query)
 		})
@@ -1156,6 +1165,8 @@ var fixturesErrors = map[string]*errors.Kind{
 	`LOCK TABLES foo AS READ`:                              errUnexpectedSyntax,
 	`LOCK TABLES foo LOW_PRIORITY READ`:                    errUnexpectedSyntax,
 	`SELECT * FROM mytable WHERE i IN (SELECT i FROM foo)`: ErrUnsupportedSubqueryExpression,
+	`SELECT * FROM mytable LIMIT -100`:                     ErrUnsupportedSyntax,
+	`SELECT * FROM mytable LIMIT 100 OFFSET -1`:            ErrUnsupportedSyntax,
 	`SELECT * FROM files
 		JOIN commit_files
 		JOIN refs