-
Notifications
You must be signed in to change notification settings - Fork 90
Add support of ddl and dml sql statements to SqlToSubstrait #428
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
Conversation
fe86d02
to
9ce46e3
Compare
create table as
and create view
to SqlToSubstrait9ce46e3
to
3561c70
Compare
3561c70
to
b9a1c61
Compare
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.
This PR motivated me to continue some ongoing work for #362, specifically #430.
Effectively, the plan is to break SqlToSubstrait down into a conversion of SQL into Calcite, and then Calcite into Substrait. The DdlRelBuilder
you've introduced goes against this work because it couples the SQL to the Substrait translation. It also seems somewhat duplicative, because I think we should be able to use the Calcite machinary to convert DDL SqlNodes into RelNodes, and then all we would need would be your SubstraitRelVisitor updates.
var parsedList = parser.parseStmtList(); | ||
SqlToRelConverter converter = createSqlToRelConverter(validator, catalogReader); | ||
// IMPORTANT: parsedList gets filtered in the call below | ||
List<io.substrait.plan.Plan.Root> ddlRelRoots = ddlSqlToRootNodes(parsedList, converter); |
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.
Do we need to process DDL statements separately? Would it be possible to just parse all statements, and then just use the conversion code in SubstraitRelVisitor?
I think it's a good idea to break down SqlToSubstrait into Sql->Calcite and Calcite->Substrait, as it will give better flexibility. |
Ah, TIL. Poking around your PR, the DML statements I want to think more about the DDL API, and honestly kind of want to land it after #430. That being said, would you be open to splitting the DML and DDL processing into separate PRs? The DML works is pretty straightforward and should be easier to review on it's own. |
DDL:
For the above mentioned sql statements create the following relations:
NamedWrite
forcreate table as
NamedDdl
forcreate view
All other types of sql statements are handled as before.
DML:
The class
SubstraitRelVisitor
now supports the following operations for theTableModify
node: