-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for common table expressions #14321
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
d9ba97f
to
f7bd9e0
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
"Sharded": true | ||
}, | ||
"FieldQuery": "select id, foo, weight_string(id), weight_string(foo) from (select id, foo from (select id, foo from `user` where 1 != 1) as x where 1 != 1 union select id, foo from (select id, foo from `user` where 1 != 1) as x where 1 != 1) as dt where 1 != 1", | ||
"Query": "select id, foo, weight_string(id), weight_string(foo) from (select id, foo from (select id, foo from `user`) as x union select id, foo from (select id, foo from `user`) as x) as dt", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we simplify this:
"Query": "select id, foo, weight_string(id), weight_string(foo) from (select id, foo from (select id, foo from `user`) as x union select id, foo from (select id, foo from `user`) as x) as dt", | |
"Query": "select id, foo, weight_string(id), weight_string(foo) from (select id, foo from `user`) as x union select id, foo, weight_string(id), weight_string(foo) from (select id, foo from `user`) as x" |
{ | ||
"comment": "CTEs cant use a table with the same name as the CTE alias", | ||
"query": "with user as (select aa from user where user.id=1) select ref.col from ref join user", | ||
"plan": "VT12001: unsupported: do not support CTE that use the CTE alias inside the CTE query" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any plan on supporting this? Also, is the case where the table name is prefixed with the database identifier unsupported? (i.e.: with user as (select aa from user where user.id=1) select ref.col from ref join my_db.user
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, long term I think we want much better support for CTEs. instead of rewriting them to derived tables they should be first hand citizens, so that the parts of a query that we push down to MySQL uses CTE if the original query used a CTE, and even supporting recursive CTEs using a new engine primitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few questions, nits and suggestions. Approving this for now as there is nothing major blocking this for me. Really excited about this, thank you 🚀
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me after the last few changes.
Signed-off-by: Andres Taylor <[email protected]>
@@ -0,0 +1,1844 @@ | |||
[ | |||
{ | |||
"comment": "with t as (select count(*) as a from user) select a from t", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can all the cases have comments about what is getting tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the right of join route can be improvement as it is using sharding key in where condition.
{
"QueryType": "SELECT",
"Original": "with x as (select * from authoritative) select music.id, x.col1 from music join x on music.id = x.user_id",
"Instructions": {
"OperatorType": "Join",
"Variant": "Join",
"JoinColumnIndexes": "L:0,R:0",
"JoinVars": {
"music_id": 0
},
"TableName": "music_authoritative",
"Inputs": [
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select music.id from music where 1 != 1",
"Query": "select music.id from music",
"Table": "music"
},
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select x.col1 from (select user_id, col1, col2 from authoritative where 1 != 1) as x where 1 != 1",
"Query": "select x.col1 from (select user_id, col1, col2 from authoritative where user_id = :music_id) as x",
"Table": "authoritative"
}
]
},
"TablesUsed": [
"user.authoritative",
"user.music"
]
}
Signed-off-by: Andres Taylor <[email protected]>
Description
ANSI SQL has had support for WITH and common table expressions since SQL-99, and MySQL added support in the 8.0 release, so it's about time Vitess supports it.
This PR adds tentative support for
WITH
andCTE
s. The approach I have taken is to rewrite the CTE into derived tables before the actual planning starts.In this PR, only
SELECT
andUNION
supportWITH
.Related Issue(s)
Part of solving #4099