Support recursive CTEs #16427

systay · 2024-07-18T07:31:06Z

Description

Adds support for recursive CTEs in Vitess.

A recursive CTE looks like:

with recursive myCTE as (
<seed>
union [all]
<term>) 
select * from myCTE

This PR includes both planner code that tries to merge the seed and term with each other, so the recursion can be pushed down to mysql, and also an engine primitive that can execute the recursion.

Related Issue(s)

Part of #16415
Builds on top of #16569
Website: vitessio/website#1815

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2024-07-18T07:31:09Z

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

Ensure that the Pull Request has a descriptive title.
Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

Apply the release notes (needs details) label if users need to know about this change.
New features should be documented.
There should be some code comments as to why things are implemented the way they are.
There should be a comment at the top of each new or modified test to explain what the test does.

New flags

Is this flag really necessary?
Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

Each item in Jobs should be named in order to mark it as required.
If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

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

codecov · 2024-08-01T11:37:32Z

Codecov Report

Attention: Patch coverage is 86.00629% with 89 lines in your changes missing coverage. Please review.

Project coverage is 68.86%. Comparing base (0d6b768) to head (b67eb1a).
Report is 2 commits behind head on main.

Files	Patch %	Lines
go/vt/vtgate/engine/recurse_cte.go	71.83%	20 Missing ⚠️
go/vt/vtgate/planbuilder/operators/recurse_cte.go	80.00%	18 Missing ⚠️
go/vt/vtgate/semantics/cte_table.go	75.00%	18 Missing ⚠️
go/vt/vtgate/planbuilder/operators/phases.go	82.53%	11 Missing ⚠️
go/vt/vtgate/planbuilder/operators/ast_to_op.go	85.71%	7 Missing ⚠️
go/vt/vtgate/planbuilder/operators/SQL_builder.go	92.98%	4 Missing ⚠️
go/vt/vtgate/planbuilder/operator_transformers.go	85.71%	2 Missing ⚠️
go/vt/vtgate/planbuilder/operators/cte_merging.go	95.55%	2 Missing ⚠️
go/vt/vtgate/semantics/real_table.go	95.23%	2 Missing ⚠️
go/vt/vtgate/semantics/table_collector.go	97.29%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16427      +/-   ##
==========================================
+ Coverage   68.83%   68.86%   +0.03%     
==========================================
  Files        1558     1562       +4     
  Lines      200042   200618     +576     
==========================================
+ Hits       137693   138163     +470     
- Misses      62349    62455     +106

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Andres Taylor <andres@planetscale.com>

harshit-gangal

looks like there are simple mysql test files available here which can be used for testing
https://github.com/search?q=repo%3Amysql%2Fmysql-server+path%3A%2F%5Emysql-test%5C%2Ft%5C%2F%2F+WITH+RECURSIVE&type=code

frouioui · 2024-08-16T12:46:30Z

go/vt/vterrors/code.go

@@ -97,6 +97,10 @@ var (
 VT09023 = errorWithoutState("VT09023", vtrpcpb.Code_FAILED_PRECONDITION, "could not map %v to a keyspace id", "Unable to determine the shard for the given row.")
 VT09024 = errorWithoutState("VT09024", vtrpcpb.Code_FAILED_PRECONDITION, "could not map %v to a unique keyspace id: %v", "Unable to determine the shard for the given row.")
 VT09025 = errorWithoutState("VT09025", vtrpcpb.Code_FAILED_PRECONDITION, "atomic transaction error: %v", "Error in atomic transactions")
+ VT09026 = errorWithState("VT09026", vtrpcpb.Code_FAILED_PRECONDITION, CTERecursiveRequiresUnion, "Recursive Common Table Expression '%s' should contain a UNION", "")


These new errors need to be added to the Errors slice below in that file.

harshit-gangal · 2024-08-16T13:02:55Z

go/vt/sqlparser/ast_funcs.go

+
+ tableName, ok := node.Expr.(TableName)
+ if !ok {
+ panic(vterrors.Errorf(vtrpcpb.Code_INTERNAL, "BUG: Derived table should have an alias. This should not be possible"))


nit: should use vterrors.VT13001

GuptaManan100

Rest LGTM

GuptaManan100 · 2024-08-16T13:05:31Z

go/vt/vtgate/engine/recurse_cte.go

+ if vcursor.Session().InTransaction() {
+ res, err := r.TryExecute(ctx, vcursor, bindVars, wantfields)
+ if err != nil {
+ return err
+ }
+ return callback(res)
+ }


Why do we do this? How is the the running in Streaming or non-streaming mode connected to transactions?

GuptaManan100 · 2024-08-16T13:07:48Z

go/vt/vtgate/engine/recurse_cte.go

+ return r.recurse(ctx, vcursor, bindVars, result, callback)
+ })


Why don't we do the same for loop like the one for TryExecute? Why is this recursion required?

GuptaManan100 · 2024-08-16T13:09:48Z

go/vt/vtgate/engine/recurse_cte.go

+ if r.Seed.GetKeyspaceName() == r.Term.GetKeyspaceName() {
+ return r.Seed.GetKeyspaceName()
+ }
+ return r.Seed.GetKeyspaceName() + "_" + r.Term.GetKeyspaceName()


I know this is printing only, but just pointing out that it won't always give unique keyspace names.

In the sense that if Seed returns user_t1 and Term returns user_t2. , the correct output would be user_t1_t2 but not user_t1_user_t2.

I hate GetKeyspaceName and I think we should get rid of it

GuptaManan100 · 2024-08-16T13:10:21Z

go/vt/vtgate/engine/recurse_cte.go

+func (r *RecurseCTE) GetTableName() string {
+ return r.Seed.GetTableName()
+}


Shouldn't we be also accounting for Term's table name?

Don't like GetTableName either.

go/vt/vtgate/planbuilder/operators/SQL_builder.go

go/vt/vtgate/semantics/analyzer.go

go/vt/vterrors/code.go

go/vt/vtgate/engine/recurse_cte.go

harshit-gangal · 2024-08-16T15:00:47Z

go/vt/vtgate/engine/recurse_cte.go

+
+func (r *RecurseCTE) NeedsTransaction() bool {
+ return r.Seed.NeedsTransaction() || r.Term.NeedsTransaction()
+}


AFAIK, this engine primitive only deals with select statements. This should be noTxNeeded

WITH is valid with SELECT, UPDATE and DELETE. might as well leave it open for both and just check what the inputs do, right?

Update and Delete are not part of the recursive CTE inputs.
The output of the CTE will be used to perform the Update and Delete,
so CTE output becomes the input for Update/Delete operator/engine.

harshit-gangal · 2024-08-16T15:22:47Z

go/vt/vtgate/engine/recurse_cte.go

+ recurseRows = append(recurseRows, rresult.Rows...)
+ res.Rows = append(res.Rows, rresult.Rows...)
+ }
+ }
+ return res, nil


coercion is not done, either we add the support or fail on type mismatch,
this only needs to be done with result of seed and first result of term

yep. coercion and typing are still TODO, and I'm not planning on doing that in this PR

This could lead to wrong output. I am suggesting we at least add a failing condition check on field types.

go/vt/vtgate/planbuilder/operators/SQL_builder.go

go/vt/vtgate/planbuilder/operators/recurse_cte.go

harshit-gangal

mysql> with recursive cte as (select 1 union all select 1 from cte) select * from cte;
ERROR 3636 (HY000): Recursive query aborted after 1001 iterations. Try increasing @@cte_max_recursion_depth to a larger value.

We should also add this check in CTE engine to avoid infinite loop

go/vt/vtgate/semantics/scoper.go

go/vt/vtgate/planbuilder/operators/phases.go

go/vt/vtgate/planbuilder/operators/recurse_cte.go

go/vt/vtgate/planbuilder/plancontext/planning_context.go

go/vt/vtgate/semantics/real_table.go

Signed-off-by: Andres Taylor <andres@planetscale.com>

GrahamCampbell · 2024-08-19T09:43:07Z

go/mysql/sqlerror/constants.go

+ ERCTERecursiveRequiresSingleReference = ErrorCode(3577)
+ ERCTEMaxRecursionDepth = ErrorCode(3636)
+ ERRegexpStringNotTerminated = ErrorCode(3684)
+ ERRegexpBufferOverflow = ErrorCode(3684)


Error code 3684 is duplicated. Is that an error?

It's the same on the MySQL manual. I think the duplication in Vitess it's intentional 🤷

Also, the whole page is about errors, so clearly it's an error. 😜

Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Andres Taylor <andres@planetscale.com>

github-actions bot added this to the v21.0.0 milestone Jul 18, 2024

systay force-pushed the recursive-cte branch 2 times, most recently from e08d482 to 6864482 Compare July 19, 2024 05:52

harshit-gangal force-pushed the recursive-cte branch from eac93e3 to 15e11cc Compare July 23, 2024 09:04

systay force-pushed the recursive-cte branch from 15e11cc to a399988 Compare July 23, 2024 14:04

GuptaManan100 force-pushed the recursive-cte branch from 4b854ed to c22f9b1 Compare July 24, 2024 09:45

vitess-bot bot mentioned this pull request Jul 24, 2024

Update error code documentation (#16427) vitessio/website#1795

Closed

GuptaManan100 force-pushed the recursive-cte branch from c22f9b1 to 08b3b1a Compare July 24, 2024 11:38

frouioui force-pushed the recursive-cte branch from 08b3b1a to 62db64e Compare July 24, 2024 15:45

systay force-pushed the recursive-cte branch 6 times, most recently from b5f0583 to fd531fb Compare July 31, 2024 09:08

systay force-pushed the recursive-cte branch 3 times, most recently from 0add00f to dbd11c7 Compare August 8, 2024 06:55

make sure to respect the transaction needs

3aaad01

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay force-pushed the recursive-cte branch from 2eed37a to 3aaad01 Compare August 15, 2024 07:38