Subquery caching rules onceAfterAll #1470

max-hoffman · 2022-12-12T23:42:58Z

We cannot cache subquery results for a scope until the child scope has been finalized. A child scope cannot be finalized until the parent scope is finalized. Caching subqueries has to happen after all scopes are finalized. Applying hash lookups in the old way is much more difficult with this setup. This also exposed some bugs related to cacheability checks and subquery/union finalization recursion. Rewrite rules that depend on subquery caching to be in order traversals in OnceAfterAll.

The original bug sourced from a bad resolve in a query like this:

select ab.* from ab join pq on a = p where b = (select y from xy where y in (select v from uv where v = b)) order by a;

The join and two scopes appear necessary to trigger the original bug. The join's children will be unresolved when we index the inner scopes, whereas a single relation scope's child will be a resolved table. The two scopes provide a gap for determining cachability in the middle scope while the child scope still had a deferredColumn, which previously permitted caching.

GMS bump: dolthub/dolt#4987

We cannot cache subquery results for a scope until the child scope has been finalized. A child scope cannot be finalized until the parent scope is finalized. Caching subqueries has to happen after all scopes are finalized. Applying hash lookups in the old way is much more difficult with this setup. This also exposed some bugs related to cacheability checks and subquery/union finalization recursion.

max-hoffman · 2022-12-13T00:28:51Z

sql/analyzer/scope.go

@@ -80,6 +79,8 @@ func (s *Scope) newScopeFromSubqueryAlias(sqa *plan.SubqueryAlias) *Scope {
 		// We don't include the current inner node so that the outer scope nodes are still present, but not the lateral nodes
 		if s.currentNodeIsFromSubqueryExpression {
 			sqa.OuterScopeVisibility = true
+		}
+		if sqa.OuterScopeVisibility {


this concerns me, can't really explain yet

hrm... yeah... that does seem weird... leaving line 84 inside the if s.currentNodeIsFromSubqueryExpression block doesn't work? Seems like that currentNodeIsFromSubqueryExpression bit isn't getting set consistently?

It would probably be interesting to see if we can catch it where s.currentNodeIsFromSubqueryExpression == false && sqa.OuterScopeVisibility == true. That shouldn't ever happen, but if you need that separate if block, seems like it must be happening somewhere?

my later changes appeared to resolved this, still don't remember which test broke, but reverting back to the original code

…lthub/go-mysql-server into max/cache-sq-after-resolving-children

max-hoffman · 2022-12-14T17:12:13Z

enginetest/queries/query_plans.go

@@ -316,11 +373,9 @@ var PlanTests = []QueryPlanTest{
 		Query: `with cte(a,b) as (select * from ab) select * from xy where exists (select * from cte where a = x)`,
 		ExpectedPlan: "SemiJoin(cte.a = xy.x)\n" +
 			" ├─ Table(xy)\n" +
-			" └─ HashLookup(child: (cte.a), lookup: (xy.x))\n" +


this was an invalid plan, partial joins are not correct with hash lookups yet. need a new operator to support this.

fulghum

Very awesome changes @max-hoffman! 👏

Just some minor comments and questions.

fulghum · 2022-12-14T18:48:47Z

sql/analyzer/exec_builder.go

+	case plan.JoinTypeSemi:
+		newOp = plan.JoinTypeSemiHash
+	case plan.JoinTypeAnti:
+		newOp = plan.JoinTypeAntiHash
 	default:
 		panic("can only apply hash join to InnerJoin or LeftOuterJoin")


(minor)

Suggested change

panic("can only apply hash join to InnerJoin or LeftOuterJoin")

panic("can only apply hash join to InnerJoin, LeftOuterJoin, SemiJoin, or AntiJoin")

Actually... similar to another comment below... would be safer to just return an error. Seems like that would still have the effect of failing the statement and communicating the message, but wouldn't crash the whole process if somehow this codepath becomes active.

fulghum · 2022-12-14T18:57:00Z

sql/analyzer/scope.go

@@ -80,6 +79,8 @@ func (s *Scope) newScopeFromSubqueryAlias(sqa *plan.SubqueryAlias) *Scope {
 		// We don't include the current inner node so that the outer scope nodes are still present, but not the lateral nodes
 		if s.currentNodeIsFromSubqueryExpression {
 			sqa.OuterScopeVisibility = true
+		}
+		if sqa.OuterScopeVisibility {


hrm... yeah... that does seem weird... leaving line 84 inside the if s.currentNodeIsFromSubqueryExpression block doesn't work? Seems like that currentNodeIsFromSubqueryExpression bit isn't getting set consistently?

It would probably be interesting to see if we can catch it where s.currentNodeIsFromSubqueryExpression == false && sqa.OuterScopeVisibility == true. That shouldn't ever happen, but if you need that separate if block, seems like it must be happening somewhere?

enginetest/queries/query_plans.go

fulghum · 2022-12-14T19:52:07Z

sql/analyzer/analyzer.go

-			parallelizeId,
-			pushdownFiltersId,
-			subqueryIndexesId:
+			finalizeSubqueryExprsId:


finalizeSubqueries now takes care of subqueries as well as subquery expressions, so you can kill off finalizeSubqueryExprsId (sorry I didn't clean that up when I combined the rules!)

fulghum · 2022-12-14T20:07:24Z

sql/analyzer/analyzer.go

-			// recursively handle subqueries from the bottom of the plan up.
+		case
+			// skip recursive resolve rules
+			resolveSubqueryExprsId,


(minor) similar comment for resolveSubqueryExprs... I needed to combine the handling of subqs and subq expressions when I added outer scope visibility to sqas so the scopes would be nested correctly, so the resolveSubqueryExprs rule has been removed. Sorry for missing cleaning up these Ids!

fulghum · 2022-12-14T20:26:53Z

sql/analyzer/apply_indexes_for_subquery_comparisons.go

+				target = c.Name()
+			case *plan.RecursiveCte:
+			case *plan.UnresolvedTable:
+				panic("Table not resolved")


Do these need to be panics? Seems like we could just return an error and still fail the statement. Having the entire sql-server process crash from a single bad statement sucks. I know we don't expect these to ever be hit, but seems like a good process to avoid future crashes.

fulghum · 2022-12-14T20:30:05Z

sql/analyzer/apply_indexes_for_subquery_comparisons.go

+		case *plan.TableAlias:
+			alias = n.Name()
+			switch c := n.Child.(type) {
+			case *plan.ResolvedTable, *plan.SubqueryAlias, *plan.ValueDerivedTable, *plan.TransformedNamedNode, *plan.RecursiveTable, *plan.DeferredAsOfTable:


Is it worth using the sql.Table interface to capture table types more aggressively? For example... would other implementations like FilteredTable be valid here? (not sure if we'd push a filter down through a TableAlias currently or not, just brainstorming)

i think sql.Table is normally under one of these nodes, not a plan node itself, although the main two ResolvedTable and IndexedTableAccess do implement sql.Table.Mmight need a different interface i the future to capture TableNode

sql/analyzer/apply_indexes_for_subquery_comparisons.go

fulghum · 2022-12-14T20:42:09Z

sql/analyzer/apply_indexes_for_subquery_comparisons.go

 		return nil
 	}
 	indexes, err := newIndexAnalyzerForNode(ctx, rt)
 	if err != nil {
 		return nil
 	}
 	defer indexes.releaseUsedIndexes()
-	idx := indexes.MatchingIndex(ctx, ctx.GetCurrentDatabase(), rt.Name(), normalizeExpressions(tableAliases, gf)...)
+	if rt := scope.get(strings.ToLower(gf.Table())); rt != "" {


(super minor) might be worth adding the strings.ToLower to the aliasScope.get method so that it's symmetric with how the aliasScope.add automatically lower cases the name.

sql/analyzer/resolve_subqueries_test.go

max-hoffman commented Dec 13, 2022

View reviewed changes

max-hoffman and others added 6 commits December 12, 2022 16:32

docstring

0b9e820

more queries

93a147d

progress

345da75

prog

1e4453a

tests running

98f3a2e

[ga-format-pr] Run ./format_repo.sh to fix formatting

f93b99d

max-hoffman marked this pull request as ready for review December 14, 2022 02:45

max-hoffman mentioned this pull request Dec 14, 2022

[no-release-notes] GMS bump dolthub/dolt#4987

Closed

max-hoffman added 4 commits December 13, 2022 19:17

more plan tests

5b8a306

Merge branch 'max/cache-sq-after-resolving-children' of github.com:do…

688c444

…lthub/go-mysql-server into max/cache-sq-after-resolving-children

fix first join node caching

cccc1cf

coments

b1947c9

max-hoffman commented Dec 14, 2022

View reviewed changes

max-hoffman requested a review from fulghum December 14, 2022 17:16

max-hoffman assigned fulghum Dec 14, 2022

max-hoffman changed the title ~~Cache Subquery results rule in OnceAfterAll~~ Subquery caching rewrite Dec 14, 2022

max-hoffman changed the title ~~Subquery caching rewrite~~ Subquery caching rules onceAfterAll Dec 14, 2022

max-hoffman added 2 commits December 14, 2022 10:19

cleanup analyzer rule blocks

e0c8903

delete comments

11d32b2

fulghum approved these changes Dec 14, 2022

View reviewed changes

max-hoffman and others added 2 commits December 14, 2022 15:20

jason's comments

8c68814

[ga-format-pr] Run ./format_repo.sh to fix formatting

ca2af19

max-hoffman merged commit d56fa94 into main Dec 14, 2022

max-hoffman deleted the max/cache-sq-after-resolving-children branch December 14, 2022 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subquery caching rules onceAfterAll #1470

Subquery caching rules onceAfterAll #1470

max-hoffman commented Dec 12, 2022 •

edited

max-hoffman Dec 13, 2022 •

edited

fulghum Dec 14, 2022

max-hoffman Dec 14, 2022

max-hoffman Dec 14, 2022

fulghum left a comment

fulghum Dec 14, 2022

fulghum Dec 14, 2022

fulghum Dec 14, 2022

fulghum Dec 14, 2022

max-hoffman Dec 14, 2022

fulghum Dec 14, 2022

fulghum Dec 14, 2022

fulghum Dec 14, 2022

max-hoffman Dec 14, 2022

fulghum Dec 14, 2022

max-hoffman Dec 14, 2022

	panic("can only apply hash join to InnerJoin or LeftOuterJoin")
	panic("can only apply hash join to InnerJoin, LeftOuterJoin, SemiJoin, or AntiJoin")

Subquery caching rules onceAfterAll #1470

Subquery caching rules onceAfterAll #1470

Conversation

max-hoffman commented Dec 12, 2022 • edited

max-hoffman Dec 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulghum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-hoffman commented Dec 12, 2022 •

edited

max-hoffman Dec 13, 2022 •

edited