Repository: vitessio/vitess
Pull request #12890 (2023-04-13 22:49:09)

[release-15.0] planner fix: scoping rules for JOIN ON expression inside a subquery
## Description

This is a backport of #12881


vitess-bot[bot]: ### Review Checklist
            
Hello reviewers! :wave: Please follow this checklist when reviewing this Pull Request.

#### General
- [ ] Ensure that the Pull Request has a descriptive title.
- [ ] If this is a change that users need to know about, please apply the `release notes (needs details)` label so that merging is blocked unless the summary release notes document is included.
- [ ] If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

#### If a new flag is being introduced:
- [ ] Is it really necessary to add this flag?
- [ ] Flag names should be clear and intuitive (as far as possible)
- [ ] Help text should be descriptive.
- [ ] Flag names should use dashes (`-`) as word separators rather than underscores (`_`).

#### If a workflow is added or modified:
- [ ] Each item in `Jobs` should be named in order to mark it as `required`.
- [ ] If the workflow should be required, the maintainer team should be notified.

#### Bug fixes
- [ ] There should be at least one unit or end-to-end test.
- [ ] The Pull Request description should include a link to an issue that describes the bug.

#### Non-trivial changes
- [ ] There should be some code comments as to why things are implemented the way they are.

#### New/Existing features
- [ ] Should be documented, either by modifying the existing documentation or creating new documentation.
- [ ] New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

#### Backward compatibility            
- [ ] Protobuf changes should be wire-compatible.
- [ ] Changes to `_vt` tables and RPCs need to be backward compatible.
- [ ] `vtctl` command output order should be stable and `awk`-able.
- [ ] RPC changes should be compatible with vitess-operator
- [ ] If a flag is removed, then it should also be removed from VTop, if used there.


Patch:
diff --git a/go/vt/vtgate/semantics/scoper.go b/go/vt/vtgate/semantics/scoper.go
index 5d5f1c8510..349d131e72 100644
--- a/go/vt/vtgate/semantics/scoper.go
+++ b/go/vt/vtgate/semantics/scoper.go
@@ -46,6 +46,7 @@ type (
 		tables    []TableInfo
 		isUnion   bool
 		joinUsing map[string]TableSet
+		stmtScope bool
 	}
 )
 
@@ -62,11 +63,13 @@ func (s *scoper) down(cursor *sqlparser.Cursor) error {
 	switch node := node.(type) {
 	case *sqlparser.Update, *sqlparser.Delete:
 		currScope := newScope(s.currentScope())
+		currScope.stmtScope = true
 		s.push(currScope)
 
 		currScope.stmt = node.(sqlparser.Statement)
 	case *sqlparser.Select:
 		currScope := newScope(s.currentScope())
+		currScope.stmtScope = true
 		s.push(currScope)
 
 		// Needed for order by with Literal to find the Expression.
@@ -77,10 +80,10 @@ func (s *scoper) down(cursor *sqlparser.Cursor) error {
 	case sqlparser.TableExpr:
 		if isParentSelect(cursor) {
 			// when checking the expressions used in JOIN conditions, special rules apply where the ON expression
-			// can only see the two tables involved in the JOIN, and no other tables.
-			// To create this special context, we create a special scope here that is then merged with
-			// the surrounding scope when we come back out from the JOIN
-			nScope := newScope(nil)
+			// can only see the two tables involved in the JOIN, and no other tables of that select statement.
+			// They are allowed to see the tables of the outer select query.
+			// To create this special context, we will find the parent scope of the select statement involved.
+			nScope := newScope(s.currentScope().findParentScopeOfStatement())
 			nScope.stmt = cursor.Parent().(*sqlparser.Select)
 			s.push(nScope)
 		}
@@ -289,3 +292,14 @@ func (s *scope) prepareUsingMap() (result map[TableSet]map[string]TableSet) {
 	}
 	return
 }
+
+// findParentScopeOfStatement finds the scope that belongs to a statement.
+func (s *scope) findParentScopeOfStatement() *scope {
+	if s.stmtScope {
+		return s.parent
+	}
+	if s.parent == nil {
+		return nil
+	}
+	return s.parent.findParentScopeOfStatement()
+}
diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go
index f685bec531..417a25b340 100644
--- a/go/vt/vtgate/semantics/semantic_state.go
+++ b/go/vt/vtgate/semantics/semantic_state.go
@@ -361,7 +361,7 @@ var _ evalengine.TranslationLookup = (*SemTable)(nil)
 var columnNotSupportedErr = vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "column access not supported here")
 
 // ColumnLookup implements the TranslationLookup interface
-func (st *SemTable) ColumnLookup(col *sqlparser.ColName) (int, error) {
+func (st *SemTable) ColumnLookup(*sqlparser.ColName) (int, error) {
 	return 0, columnNotSupportedErr
 }
 
diff --git a/go/vt/vtgate/semantics/table_set.go b/go/vt/vtgate/semantics/table_set.go
index 0ddbc87a22..aa9042f958 100644
--- a/go/vt/vtgate/semantics/table_set.go
+++ b/go/vt/vtgate/semantics/table_set.go
@@ -27,7 +27,7 @@ import (
 type TableSet bitset.Bitset
 
 // Format formats the TableSet.
-func (ts TableSet) Format(f fmt.State, verb rune) {
+func (ts TableSet) Format(f fmt.State, _ rune) {
 	first := true
 	fmt.Fprintf(f, "TableSet{")
 	bitset.Bitset(ts).ForEach(func(tid int) {


Test patch:
diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.json b/go/vt/vtgate/planbuilder/testdata/select_cases.json
index aadbc7840c..bbc97e2ea0 100644
--- a/go/vt/vtgate/planbuilder/testdata/select_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/select_cases.json
@@ -7919,5 +7919,32 @@
         "main.unsharded_a"
       ]
     }
+  },
+  {
+    "comment": "subquery having join table on clause, using column reference of outer select table",
+    "query": "select (select 1 from user u1 join user u2 on u1.id = u2.id and u1.id = u3.id) subquery from user u3 where u3.id = 1",
+    "v3-plan": "symbol u3.id not found",
+    "gen4-plan": {
+      "QueryType": "SELECT",
+      "Original": "select (select 1 from user u1 join user u2 on u1.id = u2.id and u1.id = u3.id) subquery from user u3 where u3.id = 1",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "EqualUnique",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select (select 1 from `user` as u1 join `user` as u2 on u1.id = u2.id and u1.id = u3.id where 1 != 1) as subquery from `user` as u3 where 1 != 1",
+        "Query": "select (select 1 from `user` as u1 join `user` as u2 on u1.id = u2.id and u1.id = u3.id) as subquery from `user` as u3 where u3.id = 1",
+        "Table": "`user`",
+        "Values": [
+          "INT64(1)"
+        ],
+        "Vindex": "user_index"
+      },
+      "TablesUsed": [
+        "user.user"
+      ]
+    }
   }
 ]
diff --git a/go/vt/vtgate/semantics/analyzer_test.go b/go/vt/vtgate/semantics/analyzer_test.go
index 07826fd7a9..5b02fcc493 100644
--- a/go/vt/vtgate/semantics/analyzer_test.go
+++ b/go/vt/vtgate/semantics/analyzer_test.go
@@ -320,7 +320,7 @@ func TestMissingTable(t *testing.T) {
 
 func TestUnknownColumnMap2(t *testing.T) {
 	varchar := querypb.Type_VARCHAR
-	int := querypb.Type_INT32
+	integer := querypb.Type_INT32
 
 	authoritativeTblA := vindexes.Table{
 		Name: sqlparser.NewIdentifierCS("a"),
@@ -346,7 +346,7 @@ func TestUnknownColumnMap2(t *testing.T) {
 		Name: sqlparser.NewIdentifierCS("a"),
 		Columns: []vindexes.Column{{
 			Name: sqlparser.NewIdentifierCI("col"),
-			Type: int,
+			Type: integer,
 		}},
 		ColumnListAuthoritative: true,
 	}
@@ -354,7 +354,7 @@ func TestUnknownColumnMap2(t *testing.T) {
 		Name: sqlparser.NewIdentifierCS("b"),
 		Columns: []vindexes.Column{{
 			Name: sqlparser.NewIdentifierCI("col"),
-			Type: int,
+			Type: integer,
 		}},
 		ColumnListAuthoritative: true,
 	}
@@ -391,7 +391,7 @@ func TestUnknownColumnMap2(t *testing.T) {
 		name:   "authoritative columns",
 		schema: map[string]*vindexes.Table{"a": &authoritativeTblA, "b": &authoritativeTblBWithInt},
 		err:    false,
-		typ:    &int,
+		typ:    &integer,
 	}, {
 		name:   "authoritative columns with overlap",
 		schema: map[string]*vindexes.Table{"a": &authoritativeTblAWithConflict, "b": &authoritativeTblB},
@@ -1434,6 +1434,28 @@ func TestSingleUnshardedKeyspace(t *testing.T) {
 	}
 }
 
+// TestScopingSubQueryJoinClause tests the scoping behavior of a subquery containing a join clause.
+// The test ensures that the scoping analysis correctly identifies and handles the relationships
+// between the tables involved in the join operation with the outer query.
+func TestScopingSubQueryJoinClause(t *testing.T) {
+	query := "select (select 1 from u1 join u2 on u1.id = u2.id and u2.id = u3.id) x from u3"
+
+	parse, err := sqlparser.Parse(query)
+	require.NoError(t, err)
+
+	st, err := Analyze(parse, "user", &FakeSI{
+		Tables: map[string]*vindexes.Table{
+			"t": {Name: sqlparser.NewIdentifierCS("t")},
+		},
+	})
+	require.NoError(t, err)
+	require.NoError(t, st.NotUnshardedErr)
+
+	tb := st.DirectDeps(parse.(*sqlparser.Select).SelectExprs[0].(*sqlparser.AliasedExpr).Expr.(*sqlparser.Subquery).Select.(*sqlparser.Select).From[0].(*sqlparser.JoinTableExpr).Condition.On)
+	require.Equal(t, 3, tb.NumberOfTables())
+
+}
+
 var ks1 = &vindexes.Keyspace{
 	Name:    "ks1",
 	Sharded: false,