From a8e939f7e00e3f0cb3673f1827b3333ac2cb4026 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 19 Jun 2023 07:29:01 +0200 Subject: [PATCH] bugfixes: collection of fixes to bugs found while fuzzing (#13332) * bugfix: left join predicate only depending on RHS turns into inner join Signed-off-by: Andres Taylor * bugfix: use the incoming type fields to calculate the outgoing types Signed-off-by: Andres Taylor * bugfix: filter was using wrong method on evalengine Signed-off-by: Andres Taylor --------- Signed-off-by: Andres Taylor --- .../queries/aggregation/aggregation_test.go | 3 + .../endtoend/vtgate/queries/misc/misc_test.go | 10 +++ go/vt/vtgate/engine/filter.go | 7 +- go/vt/vtgate/engine/projection.go | 2 +- go/vt/vtgate/planbuilder/operators/joins.go | 2 +- .../planbuilder/testdata/from_cases.json | 78 +++++++++++++++++++ 6 files changed, 95 insertions(+), 7 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go index ac0991407d6..83cd14445db 100644 --- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go +++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go @@ -443,6 +443,9 @@ func TestBuggyQueries(t *testing.T) { "[[NULL NULL INT64(3) NULL NULL INT64(3)] "+ "[INT32(100) DECIMAL(300) INT64(3) INT32(100) DECIMAL(300) INT64(3)] "+ "[INT32(200) DECIMAL(600) INT64(3) INT32(200) DECIMAL(600) INT64(3)]]") + + mcmp.Exec("select /*vt+ PLANNER=gen4 */sum(tbl1.a), min(tbl0.b) from t10 as tbl0, t10 as tbl1 left join t10 as tbl2 on tbl1.a = tbl2.a and tbl1.b = tbl2.k") + mcmp.Exec("select /*vt+ PLANNER=gen4 */count(*) from t10 left join t10 as t11 on t10.a = t11.b where t11.a") } func TestMinMaxAcrossJoins(t *testing.T) { diff --git a/go/test/endtoend/vtgate/queries/misc/misc_test.go b/go/test/endtoend/vtgate/queries/misc/misc_test.go index 1eb5677700c..f3c811bd5ba 100644 --- a/go/test/endtoend/vtgate/queries/misc/misc_test.go +++ b/go/test/endtoend/vtgate/queries/misc/misc_test.go @@ -303,3 +303,13 @@ func TestPrepareStatements(t *testing.T) { _, err = mcmp.ExecAllowAndCompareError("deallocate prepare prep_art") assert.ErrorContains(t, err, "VT09011: Unknown prepared statement handler (prep_art) given to DEALLOCATE PREPARE") } + +func TestBuggyOuterJoin(t *testing.T) { + // We found a couple of inconsistencies around outer joins, adding these tests to stop regressions + mcmp, closer := start(t) + defer closer() + + mcmp.Exec("insert into t1(id1, id2) values (1,2), (42,5), (5, 42)") + + mcmp.Exec("select t1.id1, t2.id1 from t1 left join t1 as t2 on t2.id1 = t2.id2") +} diff --git a/go/vt/vtgate/engine/filter.go b/go/vt/vtgate/engine/filter.go index 3c2da650619..64e5f8ce02c 100644 --- a/go/vt/vtgate/engine/filter.go +++ b/go/vt/vtgate/engine/filter.go @@ -67,11 +67,8 @@ func (f *Filter) TryExecute(ctx context.Context, vcursor VCursor, bindVars map[s if err != nil { return nil, err } - intEvalResult, err := evalResult.Value().ToInt64() - if err != nil { - return nil, err - } - if intEvalResult == 1 { + + if evalResult.ToBoolean() { rows = append(rows, row) } } diff --git a/go/vt/vtgate/engine/projection.go b/go/vt/vtgate/engine/projection.go index dd0222eb014..dcd79f0f703 100644 --- a/go/vt/vtgate/engine/projection.go +++ b/go/vt/vtgate/engine/projection.go @@ -129,7 +129,7 @@ func (p *Projection) GetFields(ctx context.Context, vcursor VCursor, bindVars ma return nil, err } env := evalengine.NewExpressionEnv(ctx, bindVars, vcursor) - qr.Fields, err = p.evalFields(env, nil) + qr.Fields, err = p.evalFields(env, qr.Fields) if err != nil { return nil, err } diff --git a/go/vt/vtgate/planbuilder/operators/joins.go b/go/vt/vtgate/planbuilder/operators/joins.go index d027d4115a7..2d7451b83d4 100644 --- a/go/vt/vtgate/planbuilder/operators/joins.go +++ b/go/vt/vtgate/planbuilder/operators/joins.go @@ -54,7 +54,7 @@ func AddPredicate( case deps.IsSolvedBy(TableID(join.GetRHS())): // if we are dealing with an outer join, always start by checking if this predicate can turn // the join into an inner join - if !join.IsInner() && canConvertToInner(ctx, expr, TableID(join.GetRHS())) { + if !joinPredicates && !join.IsInner() && canConvertToInner(ctx, expr, TableID(join.GetRHS())) { join.MakeInner() } diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.json b/go/vt/vtgate/planbuilder/testdata/from_cases.json index 061def2c043..c406ac98e82 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.json @@ -6370,5 +6370,83 @@ "query": "select missing_column from unsharded, unsharded_tab", "v3-plan": "VT03019: column missing_column not found", "gen4-plan": "Column 'missing_column' in field list is ambiguous" + }, + { + "comment": "join predicate only depending on the RHS should not turn outer join into inner join", + "query": "select t1.id1, t2.id1 from t1 left join t1 as t2 on t2.id1 = t2.id2", + "v3-plan": { + "QueryType": "SELECT", + "Original": "select t1.id1, t2.id1 from t1 left join t1 as t2 on t2.id1 = t2.id2", + "Instructions": { + "OperatorType": "Join", + "Variant": "LeftJoin", + "JoinColumnIndexes": "L:0,R:0", + "TableName": "t1_t1", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "zlookup_unique", + "Sharded": true + }, + "FieldQuery": "select t1.id1 from t1 where 1 != 1", + "Query": "select t1.id1 from t1", + "Table": "t1" + }, + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "zlookup_unique", + "Sharded": true + }, + "FieldQuery": "select t2.id1 from t1 as t2 where 1 != 1", + "Query": "select t2.id1 from t1 as t2 where t2.id1 = t2.id2", + "Table": "t1" + } + ] + }, + "TablesUsed": [ + "zlookup_unique.t1" + ] + }, + "gen4-plan": { + "QueryType": "SELECT", + "Original": "select t1.id1, t2.id1 from t1 left join t1 as t2 on t2.id1 = t2.id2", + "Instructions": { + "OperatorType": "Join", + "Variant": "LeftJoin", + "JoinColumnIndexes": "L:0,R:0", + "TableName": "t1_t1", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "zlookup_unique", + "Sharded": true + }, + "FieldQuery": "select t1.id1 from t1 where 1 != 1", + "Query": "select t1.id1 from t1", + "Table": "t1" + }, + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "zlookup_unique", + "Sharded": true + }, + "FieldQuery": "select t2.id1 from t1 as t2 where 1 != 1", + "Query": "select t2.id1 from t1 as t2 where t2.id1 = t2.id2", + "Table": "t1" + } + ] + }, + "TablesUsed": [ + "zlookup_unique.t1" + ] + } } ]