[Gen4] Only remove extracted subquery from predicates on merge by arthurschreiber · Pull Request #11072 · vitessio/vitess

arthurschreiber · 2022-08-23T09:27:07Z

Description

When merging a subquery into an outer route, all vindex predicates are currently reset and filled again from the outer route's SeenPredicates.

vitess/go/vt/vtgate/planbuilder/physical/route.go

Lines 603 to 623 in 3d376ea

    
           func (r *Route) resetRoutingSelections(ctx *plancontext.PlanningContext) error { 
        
           	switch r.RouteOpCode { 
        
           	case engine.DBA, engine.Next, engine.Reference, engine.Unsharded: 
        
           		// these we keep as is 
        
           	default: 
        
           		r.RouteOpCode = engine.Scatter 
        
           	} 
        
           	r.Selected = nil 
        
           	for i, vp := range r.VindexPreds { 
        
           		r.VindexPreds[i] = &VindexPlusPredicates{ColVindex: vp.ColVindex, TableID: vp.TableID} 
        
           	} 
        
           	for _, predicate := range r.SeenPredicates { 
        
           		err := r.tryImprovingVindex(ctx, predicate) 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           	} 
        
           	return nil 
        
           }

For routes of ApplyJoin nodes, the SeenPredicates list is empty and thus routing information could not be restored correctly.

Instead of resetting the vindex predicates, we can just remove all vindex predicate options for the merged subquery instead, and leave all other options untouched. This ensures that we still can pick any vindexes derived from the join operation, instead of falling back to a Scatter route.

Related Issue(s)

This fixes #10823.

Checklist

"Backport me!" label has been added if this change should be backported
Tests were added or are not required
Documentation was added or is not required

When merging a subquery into a route, all vindex predicates were reset and filled again from the route's `SeenPredicates`. For routes of `ApplyJoin` nodes, the `SeenPredicates` list is empty and thus routing information could not be restored correctly. Instead of resetting the vindex predicates, we can just remove all vindex predicate options for the merged subquery instead, and leave all other options untouched. Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

vitess-bot · 2022-08-23T09:27:13Z

arthurschreiber · 2022-08-23T09:38:49Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

+
+		for j, option := range vp.Options {
+			if sqlparser.EqualsExpr(option.Predicates[0], extractedSubquery.Original) {
+				idx = j


I guess we could break here after we found one matching option?

I am unsure whether the extracted subquery can match more than one vindex option or not. @harshit-gangal, do you know?

Otherwise, let's break yes

I double checked this. Even if the same inner query exists multiple times in an outer query, each ExtractedSubquery object is unique. This is due to the unique name generated for each subquery's replacement bind variable.

I added a comment over at https://github.com/vitessio/vitess/pull/11104/files#diff-ecda8980a2e86d0f2a98ac67bb7b55d759fa7d1e8037dab2497f8fd167ada4c5R120-R121 that describes this.

arthurschreiber · 2022-08-23T09:40:20Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

+		idx := -1
+
+		for j, option := range vp.Options {
+			if sqlparser.EqualsExpr(option.Predicates[0], extractedSubquery.Original) {


This only looks at the first predicate in the list. I think that's correct for all single column vindexes. Do we need to do anything special here for multi column vindexes? I don't think there's any other support for subqueries that match against tuples, so multi column vindex (and thus vindex predicates with more than one value) are not relevant yet.

I think the safest bet would be to check the length of option.Predicates, and, only if the length is equal to one, compare the two predicates.

arthurschreiber · 2022-08-23T09:40:37Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

+	idx := -1
+	for i, predicate := range route.SeenPredicates {
+		if sqlparser.EqualsExpr(predicate, extractedSubquery) {
+			idx = i


Same here, we could break after we found one match.

arthurschreiber · 2022-08-23T09:42:09Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

-		return nil, err
+	outer.Selected = nil
+	switch outer.RouteOpCode {
+	case engine.DBA, engine.Next, engine.Reference, engine.Unsharded:


I've taken this list from resetRoutingSelections, but I'm not sure we can actually ever reach this code with one of these routing op codes? 🤔

I am unsure too. I am assuming if, for instance, we had a DBA query with a subquery? cc @systay

arthurschreiber · 2022-08-23T12:11:28Z

@systay After having talked this through with @evaccaro and @pudiva, there might be a different way of fixing this:

diff --git a/go/vt/vtgate/planbuilder/physical/route_planning.go b/go/vt/vtgate/planbuilder/physical/route_planning.go
index 8c78dc4667..5ac19a3e94 100644
--- a/go/vt/vtgate/planbuilder/physical/route_planning.go
+++ b/go/vt/vtgate/planbuilder/physical/route_planning.go
@@ -563,6 +563,7 @@ func createRouteOperatorForJoin(aRoute, bRoute *Route, joinPredicates []sqlparse
                VindexPreds:         append(aRoute.VindexPreds, bRoute.VindexPreds...),
                SysTableTableSchema: append(aRoute.SysTableTableSchema, bRoute.SysTableTableSchema...),
                SysTableTableName:   sysTableName,
+               SeenPredicates:      append(aRoute.SeenPredicates, bRoute.SeenPredicates...),
                Source: &ApplyJoin{
                        LHS:       aRoute.Source,
                        RHS:       bRoute.Source,

By merging the SeenPredicates from both routes of the join into the merged join route, resetRoutingSelections would be able to re-create the required Vindex predicate options and pick the correct vindex option for routing.

Which fix do you prefer? 🙇‍♂️

frouioui

Looks good to me. I will let one more person ack this and answer my questions.

frouioui · 2022-08-24T20:47:41Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

-		return nil, err
+	outer.Selected = nil
+	switch outer.RouteOpCode {
+	case engine.DBA, engine.Next, engine.Reference, engine.Unsharded:


I am unsure too. I am assuming if, for instance, we had a DBA query with a subquery? cc @systay

frouioui · 2022-08-24T20:55:38Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

+		idx := -1
+
+		for j, option := range vp.Options {
+			if sqlparser.EqualsExpr(option.Predicates[0], extractedSubquery.Original) {


I think the safest bet would be to check the length of option.Predicates, and, only if the length is equal to one, compare the two predicates.

frouioui · 2022-08-24T20:57:13Z

go/vt/vtgate/planbuilder/physical/subquery_planning.go

+
+		for j, option := range vp.Options {
+			if sqlparser.EqualsExpr(option.Predicates[0], extractedSubquery.Original) {
+				idx = j


I am unsure whether the extracted subquery can match more than one vindex option or not. @harshit-gangal, do you know?

Otherwise, let's break yes

arthurschreiber · 2022-08-26T08:19:26Z

@frouioui Thank you for reviewing this! 🙇‍♂️

I opened #11104 which contains an alternative (and I think architecturally more sound) approach to fixing the same issue.

I'm going to close this pull request.

arthurschreiber requested review from frouioui, harshit-gangal and systay as code owners August 23, 2022 09:27

arthurschreiber commented Aug 23, 2022

View reviewed changes

arthurschreiber mentioned this pull request Aug 23, 2022

Merge subqueries that "join" on lookup index columns. #10966

Merged

3 tasks

systay added Type: Feature Component: Query Serving labels Aug 24, 2022

frouioui reviewed Aug 24, 2022

View reviewed changes

arthurschreiber mentioned this pull request Aug 26, 2022

[Gen4] Merge SeenPredicates when creating route operator for join #11104

Merged

3 tasks

arthurschreiber closed this Aug 26, 2022

	func (r Route) resetRoutingSelections(ctx plancontext.PlanningContext) error {
	switch r.RouteOpCode {
	case engine.DBA, engine.Next, engine.Reference, engine.Unsharded:
	// these we keep as is
	default:
	r.RouteOpCode = engine.Scatter
	}

	r.Selected = nil
	for i, vp := range r.VindexPreds {
	r.VindexPreds[i] = &VindexPlusPredicates{ColVindex: vp.ColVindex, TableID: vp.TableID}
	}

	for _, predicate := range r.SeenPredicates {
	err := r.tryImprovingVindex(ctx, predicate)
	if err != nil {
	return err
	}
	}
	return nil
	}

Conversation

arthurschreiber commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Uh oh!

vitess-bot bot commented Aug 23, 2022 • edited by frouioui Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist

General

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurschreiber commented Aug 23, 2022

Uh oh!

frouioui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurschreiber commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arthurschreiber commented Aug 23, 2022 •

edited

Loading

vitess-bot bot commented Aug 23, 2022 •

edited by frouioui

Loading

arthurschreiber commented Aug 26, 2022 •

edited

Loading