Fix validation rule for tuples in Projections instead of adding a new…

… one Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
src-d · Apr 22, 2019 · 9f3baf5 · 9f3baf5
1 parent 854f879
commit 9f3baf5
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 107 deletions.
diff --git a/engine_test.go b/engine_test.go
@@ -909,8 +909,6 @@ var queries = []struct {
 		"SELECT FROM_BASE64('YmFy')",
 		[]sql.Row{{string("bar")}},
 	},
-<<<<<<< HEAD
-<<<<<<< HEAD
 	{
 		"SELECT DATE_ADD('2018-05-02', INTERVAL 1 DAY)",
 		[]sql.Row{{time.Date(2018, time.May, 3, 0, 0, 0, 0, time.UTC)}},

diff --git a/sql/analyzer/resolve_columns.go b/sql/analyzer/resolve_columns.go
@@ -38,41 +38,6 @@ func checkAliases(ctx *sql.Context, a *Analyzer, n sql.Node) (sql.Node, error) {
 	return n, err
 }
 
-func checkNoTuplesProjected(ctx *sql.Context, a *Analyzer, node sql.Node) (sql.Node, error) {
-	span, _ := ctx.Span("no_tuples_projected")
-	defer span.Finish()
-
-	a.Log("check no tuples as in projection, node of type: %T", node)
-	return node.TransformUp(func(node sql.Node) (sql.Node, error) {
-		project, ok := node.(*plan.Project)
-		if ok {
-			for _, col := range project.Projections {
-				_, ok := col.(expression.Tuple)
-				if ok {
-					return node, ErrTupleProjected.New()
-				}
-			}
-		}
-		groupby, ok := node.(*plan.GroupBy)
-		if ok {
-			for _, c := range groupby.Grouping {
-				_, ok := c.(expression.Tuple)
-				if ok {
-					return node, ErrTupleProjected.New()
-				}
-			}
-			for _, c := range groupby.Aggregate {
-				_, ok := c.(expression.Tuple)
-				if ok {
-					return node, ErrTupleProjected.New()
-				}
-			}
-		}
-
-		return node, nil
-	})
-}
-
 func lookForAliasDeclarations(node sql.Expressioner) map[string]struct{} {
 	var (
 		aliases = map[string]struct{}{}

diff --git a/sql/analyzer/resolve_columns_test.go b/sql/analyzer/resolve_columns_test.go
@@ -75,64 +75,6 @@ func TestMisusedAlias(t *testing.T) {
 	require.EqualError(err, ErrMisusedAlias.New("alias_i").Error())
 }
 
-func TestNoTuplesProjected(t *testing.T) {
-	require := require.New(t)
-	f := getRule("no_tuples_projected")
-
-	table := mem.NewTable("mytable", sql.Schema{
-		{Name: "i", Type: sql.Int32},
-	})
-
-	node := plan.NewProject([]sql.Expression{
-		expression.NewTuple(
-			expression.NewLiteral(1, sql.Int64),
-			expression.NewLiteral(2, sql.Int64),
-		),
-	}, plan.NewResolvedTable(table))
-
-	_, err := f.Apply(sql.NewEmptyContext(), nil, node)
-	require.EqualError(err, ErrTupleProjected.New().Error())
-}
-
-func TestNoTuplesGroupBy(t *testing.T) {
-	require := require.New(t)
-	f := getRule("no_tuples_projected")
-
-	table := mem.NewTable("mytable", sql.Schema{
-		{Name: "i", Type: sql.Int32},
-	})
-
-	node := plan.NewGroupBy([]sql.Expression{
-		expression.NewUnresolvedColumn("a"),
-		expression.NewUnresolvedColumn("b"),
-	},
-		[]sql.Expression{
-			expression.NewTuple(
-				expression.NewLiteral(1, sql.Int64),
-				expression.NewLiteral(2, sql.Int64),
-			),
-		},
-		plan.NewResolvedTable(table))
-
-	_, err := f.Apply(sql.NewEmptyContext(), nil, node)
-	require.EqualError(err, ErrTupleProjected.New().Error())
-
-	node = plan.NewGroupBy([]sql.Expression{
-		expression.NewTuple(
-			expression.NewLiteral(1, sql.Int64),
-			expression.NewLiteral(2, sql.Int64),
-		),
-	},
-		[]sql.Expression{
-			expression.NewUnresolvedColumn("a"),
-			expression.NewUnresolvedColumn("b"),
-		},
-		plan.NewResolvedTable(table))
-
-	_, err = f.Apply(sql.NewEmptyContext(), nil, node)
-	require.EqualError(err, ErrTupleProjected.New().Error())
-}
-
 func TestQualifyColumns(t *testing.T) {
 	require := require.New(t)
 	f := getRule("qualify_columns")

diff --git a/sql/analyzer/rules.go b/sql/analyzer/rules.go
@@ -28,7 +28,6 @@ var OnceBeforeDefault = []Rule{
 	{"resolve_subqueries", resolveSubqueries},
 	{"resolve_tables", resolveTables},
 	{"check_aliases", checkAliases},
-	{"no_tuples_projected", checkNoTuplesProjected},
 }
 
 // OnceAfterDefault contains the rules to be applied just once after the
@@ -67,5 +66,4 @@ var (
 	// ErrMisusedAlias is returned when a alias is defined and used in the same projection.
 	ErrMisusedAlias = errors.NewKind("column %q does not exist in scope, but there is an alias defined in" +
 		" this projection with that name. Aliases cannot be used in the same projection they're defined in")
-	ErrTupleProjected = errors.NewKind("unexpected tuple found, maybe remove the ()?")
 )
diff --git a/sql/analyzer/validation_rules.go b/sql/analyzer/validation_rules.go
@@ -195,27 +195,36 @@ func validateSchema(t *plan.ResolvedTable) error {
 	return nil
 }
 
-func validateProjectTuples(ctx *sql.Context, a *Analyzer, n sql.Node) (sql.Node, error) {
-	span, _ := ctx.Span("validate_project_tuples")
-	defer span.Finish()
+func findProjectTuples(n sql.Node) (sql.Node, error) {
+	if n == nil {
+		return n, nil
+	}
 
 	switch n := n.(type) {
-	case *plan.Project:
-		for i, e := range n.Projections {
+	case *plan.Project, *plan.GroupBy:
+		for i, e := range n.(sql.Expressioner).Expressions() {
 			if sql.IsTuple(e.Type()) {
 				return nil, ErrProjectTuple.New(i+1, sql.NumColumns(e.Type()))
 			}
 		}
-	case *plan.GroupBy:
-		for i, e := range n.Aggregate {
-			if sql.IsTuple(e.Type()) {
-				return nil, ErrProjectTuple.New(i+1, sql.NumColumns(e.Type()))
+	default:
+		for _, ch := range n.Children() {
+			_, err := findProjectTuples(ch)
+			if err != nil {
+				return nil, err
 			}
 		}
 	}
+
 	return n, nil
 }
 
+func validateProjectTuples(ctx *sql.Context, a *Analyzer, n sql.Node) (sql.Node, error) {
+	span, _ := ctx.Span("validate_project_tuples")
+	defer span.Finish()
+	return findProjectTuples(n)
+}
+
 func validateCaseResultTypes(ctx *sql.Context, a *Analyzer, n sql.Node) (sql.Node, error) {
 	span, ctx := ctx.Span("validate_case_result_types")
 	defer span.Finish()

diff --git a/sql/analyzer/validation_rules_test.go b/sql/analyzer/validation_rules_test.go
@@ -235,11 +235,22 @@ func TestValidateProjectTuples(t *testing.T) {
 			plan.NewProject([]sql.Expression{
 				expression.NewTuple(
 					expression.NewLiteral(1, sql.Int64),
-					expression.NewLiteral(1, sql.Int64),
+					expression.NewLiteral(2, sql.Int64),
 				),
 			}, nil),
 			false,
 		},
+		{
+			"distinct with a 2 elem tuple inside the project",
+			plan.NewDistinct(
+				plan.NewProject([]sql.Expression{
+					expression.NewTuple(
+						expression.NewLiteral(1, sql.Int64),
+						expression.NewLiteral(2, sql.Int64),
+					),
+				}, nil)),
+			false,
+		},
 		{
 			"groupby with no tuple",
 			plan.NewGroupBy([]sql.Expression{