Feature/v0.10 sql ddl #1075

Yaiba · 2024-10-25T19:25:28Z

This pr adds new DDLs:

create table
alter table
create index
drop index

brennanjl

A few comments but mostly looked great

brennanjl · 2024-10-28T19:31:52Z

parse/grammar/KuneiformParser.g4

+;
+
+fk_constraint:
+    REFERENCES table=identifier (LPAREN column=identifier RPAREN) (fk_action (fk_action)*)?


Probably better to do:

REFERENCES table=identifier (LPAREN column=identifier RPAREN) (fk_action (fk_action)?)?

so that only a max of 2 can be specified

because we only support on DELETE and on UPDATE? make sense

I think that is all postgres supports too.

brennanjl · 2024-10-28T19:33:40Z

parse/grammar/KuneiformParser.g4

@@ -184,13 +216,58 @@ sql:

 sql_statement:
    (WITH RECURSIVE? common_table_expression (COMMA common_table_expression)*)?
-    (select_statement | update_statement | insert_statement | delete_statement)
+    (create_table_statement | alter_table_statement| create_index_statement | drop_index_statement // ddl


This seems incorrect. DDL shouldn't have CTEs. For example, I cant have:

WITH my_cte AS ( -- ... ) CREATE TABLE ...

These should instead be separate clauses

oh I wanted to make a quick pass first then create another statement, totally forget about it. A new ddl_statement sounds good?

brennanjl · 2024-10-28T19:37:33Z

parse/grammar/KuneiformParser.g4

+;
+
+create_index_statement:
+    CREATE UNIQUE? INDEX name=identifier? ON table=identifier LPAREN  columns=identifier_list RPAREN


Can you add if not exists?

sure, Do we need this for other statements, like alter table add column if not exist xxx?
I only create the syntax you put in the docs

Yeah that'd be great.

Actually I don't think this is important. As long as we have it for create that is ok for now. We can add it in a later release

brennanjl · 2024-10-28T19:37:36Z

parse/grammar/KuneiformParser.g4

 ;

 common_table_expression:
    identifier (LPAREN (identifier (COMMA identifier)*)? RPAREN)? AS LPAREN select_statement RPAREN
 ;

+create_table_statement:
+    CREATE TABLE name=identifier


Can you add if not exists?

brennanjl · 2024-10-28T19:40:01Z

parse/grammar/KuneiformParser.g4

+    PRIMARY KEY LPAREN identifier_list RPAREN
+    | UNIQUE LPAREN identifier_list RPAREN
+    | CHECK LPAREN sql_expr RPAREN
+    | FOREIGN KEY LPAREN identifier RPAREN fk_constraint


not a huge deal but it might be easier if these are named? e.g.

unnamed_constraint: PRIMARY KEY LPAREN identifier_list RPAREN #unnamed_constraint_pk | UNIQUE LPAREN identifier_list RPAREN #unnamed_contract_unique

the syntax of the constraint is simple enough, for now it's done just in one switch, I don't think it's necessary to create different tree nodes on constraints.

Yaiba · 2024-10-29T18:23:44Z

To deal with new DDL, I create ParseDDL and WriteDDL for now so I don't break the downstream analyzer/planner, also allow me to write tests.

Also add some basic validations on create table

Yaiba · 2024-10-29T18:33:14Z

parse/grammar/KuneiformParser.g4

-sql:
-    sql_statement SCOL
+// sql is a top-level SQL statement, it maps to SQLStmt interface in AST.
+sql_stmt:


To change sql_statement to sql_stmt in action/procedure block will break the parser/analyzer/planner, better do it together when we make the changes

brennanjl · 2024-10-30T19:52:52Z

parse/grammar/KuneiformParser.g4

+    (table_column_def | table_constraint_def | table_index_def)
+    (COMMA  (table_column_def | table_constraint_def | table_index_def))*
+    RPAREN
+;


I think you are missing drop table

ah shoot... Will add it now

hmm, do you want to add drop table in our DDL docs? Or do I just use syntax from https://www.postgresql.org/docs/current/sql-droptable.html ?

Yeah we should add drop ddl.

Just base it on postgres's tho

brennanjl · 2024-10-30T19:53:46Z

parse/parse.go

+// NOTE: THIS is only temporary so that I can write tests. After we shift from
+// *SQLStatement to SQLStmt we can delete this.
+func ParseDDL(sql string, schema *types.Schema, skipValidation bool) (res *DDLParseResult, err error) {
+	parser, errLis, visitor, deferFn, err := setupParser2(sql)


Why do you need the schema parameter? It is unused.

skipValidation is unused as well

ah didn't event look the function signature, just copied from ParseSQL, will change.

brennanjl · 2024-10-30T19:54:10Z

parse/parse.go

+	ParseErrs ParseErrs
+
+	// Mutative is true if the statement mutates state.
+	Mutative bool


All DDL is mutative, so this is redundant

Yaiba · 2024-10-30T20:39:44Z

parse/grammar/KuneiformParser.g4

+opt_drop_behavior:
+    CASCADE
+    | RESTRICT
+    |


Inspired by the antlr postgres grammar, I kind of like it

Yaiba · 2024-10-30T20:40:54Z

OK Just add Drop table statement, also address feedback on ParseDDL function.

Yaiba added 7 commits October 25, 2024 14:21

Add DDL grammars

c851c35

create table statement

aade82c

grammar change

db540b7

alter table

d71cbf7

grammar change

9f82683

add/drop index statement

78c8830

adjust

028fd0e

Yaiba requested a review from brennanjl October 25, 2024 19:25

adjust

3e68a06

brennanjl reviewed Oct 28, 2024

View reviewed changes

adjust grammar

1b6ab48

Yaiba requested a review from brennanjl October 29, 2024 18:26

Yaiba commented Oct 29, 2024

View reviewed changes

brennanjl reviewed Oct 30, 2024

View reviewed changes

Yaiba added 2 commits October 30, 2024 15:07

update ParseDDL signature

7d1569a

add DROP TABLE

600e9e5

Yaiba commented Oct 30, 2024

View reviewed changes

Yaiba marked this pull request as ready for review October 30, 2024 20:41

brennanjl approved these changes Nov 1, 2024

View reviewed changes

brennanjl merged commit d1c7ec8 into v0.10-sql Nov 1, 2024

brennanjl deleted the feature/v0.10-sql-ddl branch November 1, 2024 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/v0.10 sql ddl #1075

Feature/v0.10 sql ddl #1075

Yaiba commented Oct 25, 2024

brennanjl left a comment

brennanjl Oct 28, 2024

Yaiba Oct 28, 2024

brennanjl Oct 28, 2024

brennanjl Oct 28, 2024

Yaiba Oct 28, 2024

brennanjl Oct 28, 2024

Yaiba Oct 28, 2024

brennanjl Oct 28, 2024

brennanjl Oct 28, 2024 •

edited

Loading

brennanjl Oct 28, 2024

brennanjl Oct 28, 2024

Yaiba Oct 29, 2024

Yaiba commented Oct 29, 2024 •

edited

Loading

Yaiba Oct 29, 2024

brennanjl Oct 30, 2024

brennanjl Oct 30, 2024 •

edited

Loading

Yaiba Oct 30, 2024

Yaiba Oct 30, 2024

brennanjl Oct 30, 2024

brennanjl Oct 30, 2024

brennanjl Oct 30, 2024

brennanjl Oct 30, 2024

Yaiba Oct 30, 2024

brennanjl Oct 30, 2024

Yaiba Oct 30, 2024

Yaiba commented Oct 30, 2024

Feature/v0.10 sql ddl #1075

Feature/v0.10 sql ddl #1075

Conversation

Yaiba commented Oct 25, 2024

brennanjl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennanjl Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennanjl Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba commented Oct 30, 2024

brennanjl Oct 28, 2024 •

edited

Loading

Yaiba commented Oct 29, 2024 •

edited

Loading

brennanjl Oct 30, 2024 •

edited

Loading