Adds DML (INSERT, DELETE, UPDATE, UPSERT, REPLACE) #1666

johnedquinn · 2024-11-27T23:05:36Z

Relevant Issues & Links

Description

Adds DML (INSERT, DELETE, UPDATE, UPSERT, REPLACE)
Fixed the modelling, naming, and parsing of many nodes.
Adds DQL, DML, and Common G4 files to make it easier to see what's going on.
- The only reason you would want to look at DQL is because I fixed ExprRow and the table value constructor (VALUES ..., ...). If you want, you can manually invoke diff to see what's different between DQL and PartiQLParser, but it's really just a removal of the DML.
- I removed the legacy DML stuff.
- I've left some TODOs on the DML for unanswered questions.
Named the nodes as close as I reasonably could to the SQL:1999 EBNF.
I did NOT bring over the FROM INSERT, FROM SET, FROM REMOVE, and RETURNING AST nodes since they are not SQL Spec and are not used by active customers. Please see the issues that reference this PR further below.

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

partiql-ast/src/main/java/org/partiql/ast/ConflictAction.java

partiql-ast/src/main/java/org/partiql/ast/Delete.java

alancai98 · 2024-12-05T00:11:04Z

partiql-ast/src/main/java/org/partiql/ast/InsertSource.java

+         * TODO
+         */
+        @Nullable
+        public final List<Identifier> columns;


Syntactically, could a FromDefault also include a list of columns? I couldn't find if there was this restriction from the RFCs. The EBNF seems to indicate there could be a list of columns with a DEFAULT VALUES specified:

<upsert statement> ::= UPSERT INTO <table name> [ AS <alias> ] [ ( <attr name> [, <attr name> ]... ) ] <values> ... <values> ::= DEFAULT VALUES | <values clause> | <bag value> | <sub-select>

See partiql/partiql-lang#95. I meant to cut an issue earlier, but the modelling in the RFCs is very likely wrong (and it's also unspecified). That being said, this PR adds the correct modelling while also allowing for the addition of the insert column list in the future if it is eventually specified. And, the ANTLR grammar can be updated accordingly. But for now, looking at the SQL:1999 EBNF in contrast with the RFC's EBNF, it is immediately apparent that one is wrong.

partiql-ast/src/main/java/org/partiql/ast/Update.java

partiql-ast/src/main/kotlin/org/partiql/ast/AstRewriter.kt

partiql-parser/src/main/antlr/Common.g4

partiql-parser/src/main/antlr/DML.g4

partiql-ast/src/main/java/org/partiql/ast/expr/ExprRowValue.java

Removes InsertColumnList AST node Fixes the passing of some AST children Updates Javadocs Adds tests for explicit use of ROW

alancai98

Thanks for all the detailed work adding DML to the new AST -- think we're almost there.

partiql-ast/src/main/java/org/partiql/ast/ConflictTarget.java

alancai98 · 2024-12-06T22:48:08Z

partiql-ast/src/main/java/org/partiql/ast/Insert.java

+ */
+@Builder(builderClassName = "Builder")
+@EqualsAndHashCode(callSuper = false)
+public final class Insert extends Statement {


I guess the idea is that these DML statements do have shared functionality in being DML statements. Yingtao had done a similar thing for DDL statements:

partiql-lang-kotlin/partiql-ast/src/main/java/org/partiql/ast/ddl/CreateTable.java

Line 20 in 51f4ad1

public class CreateTable extends Ddl {

It can make it a bit easier to case on the statements to apply the same logic to DQL, DML, DDL. Whatever we choose, we should try to be consistent (i.e. have DDL and DML have separate abstract classes or have the statements directly extend Statement).

alancai98 · 2024-12-06T22:55:07Z

partiql-ast/src/main/java/org/partiql/ast/ConflictTarget.java

+         */
+        // TODO: Should this be a list of identifiers? Or paths? Expressions?
+        @NotNull
+        public final List<Identifier> indexes;


So does <attr name> in the RFC (https://github.com/partiql/partiql-lang/blob/main/RFCs/0011-partiql-insert.md?plain=1#L123) refer to Identifier or IdentifierChain AstNode? In the other comment (#1666 (comment)), <index attr name> is an <attr name> which we then define as an IdentifierChain.

I'm thinking if we're inclined at all to classify it as an IdentifierChain in the AST, it is more backwards compatible API-wise since an IdentifierChain without a next is just an Identifier.

partiql-ast/src/main/java/org/partiql/ast/dml/Upsert.java

partiql-ast/src/main/java/org/partiql/ast/dml/Replace.java

partiql-ast/src/main/java/org/partiql/ast/dml/OnConflict.java

partiql-ast/src/main/java/org/partiql/ast/dml/Insert.java

partiql-ast/src/main/java/org/partiql/ast/dml/ConflictTarget.java

partiql-ast/src/main/kotlin/org/partiql/ast/AstRewriter.kt

alancai98 · 2024-12-06T23:10:44Z

From the most recent changes (may also apply to earlier changes), there are some conformance tests failing -- https://github.com/partiql/partiql-lang-kotlin/actions/runs/12205681348.

Adds SqlDialect for the DML statements Fixes the getChildren for multiple DML AST nodes Updates Javadocs to reference unimplemented, future features

Adds the conversion for ExprValues to a plan node

johnedquinn · 2024-12-07T01:05:03Z

Alright! I fixed the table value construction issue with the conformance tests, and for the other 4 remaining failing tests, I believe those tests are actually wrong. See partiql/partiql-tests#130

alancai98

Pre-approving following rebase from v1. Thanks for the deep dive into adding DML along with all the partiql-lang issues!

Regarding the conformance test failures, seems very likely that's an error in the conformance tests, but we should look more into it. Since it doesn't affect the public APIs, we could figure things out in a later work stream.

alancai98 · 2024-12-07T01:25:08Z

partiql-ast/src/main/java/org/partiql/ast/dml/UpdateTargetStep.java

+        /**
+         * TODO
+         */
+        // TODO: Change this to a literal, not an ExprLit


The updated literal modeling (#1650) has been merged in, so we can address this TODO. Think we'll have to otherwise build will fail.

This was referenced Dec 2, 2024

Implement DML RETURNING clause #1667

Open

Determine future of REMOVE DML statement #1668

Open

Determine future of FROM DML statements #1669

Open

johnedquinn force-pushed the v1-dml branch from a8e9c1c to 247776b Compare December 2, 2024 22:58

johnedquinn requested a review from alancai98 December 2, 2024 23:11

johnedquinn marked this pull request as ready for review December 2, 2024 23:11

johnedquinn mentioned this pull request Dec 3, 2024

Conflict target ambiguity and other questions partiql/partiql-lang#94

Open

alancai98 reviewed Dec 5, 2024

View reviewed changes

This was referenced Dec 5, 2024

Define the allowable syntax for updateTarget in PartiQL's grammar #1671

Open

RFC-0030 and RFC-0011 DEFAULT VALUES with <insert columns list> bug partiql/partiql-lang#95

Open

johnedquinn force-pushed the v1-dml branch from d828d61 to 429b5d3 Compare December 6, 2024 19:19

johnedquinn added 5 commits December 6, 2024 12:14

Adds INSERT, DELETE, REPLACE, UPSERT, and UPDATE

995d38d

Renames AST FromSubquery to FromExpr

1e1ccb3

Removes InsertColumnList AST node Fixes the passing of some AST children Updates Javadocs Adds tests for explicit use of ROW

Adds DML to the AST Rewriter

5d525fe

Moves DML AST nodes to dedicated package

3d822d0

Updates ConflictTargetConstraint to use an IdentifierChain

51f4ad1

johnedquinn force-pushed the v1-dml branch from aeb8204 to 51f4ad1 Compare December 6, 2024 20:28

johnedquinn requested a review from alancai98 December 6, 2024 20:28

alancai98 reviewed Dec 6, 2024

View reviewed changes

partiql deleted a comment from alancai98 Dec 7, 2024

johnedquinn added 2 commits December 6, 2024 16:30

Updates ConflictTarget.Constraint#constraintName to name

801dd2e

Adds SqlDialect for the DML statements Fixes the getChildren for multiple DML AST nodes Updates Javadocs to reference unimplemented, future features

Adds the correct parsing for table value constructors

5b594e7

Adds the conversion for ExprValues to a plan node

johnedquinn mentioned this pull request Dec 7, 2024

<in predicate> incorrectly interpreting RHS values partiql/partiql-tests#130

Open

alancai98 approved these changes Dec 7, 2024

View reviewed changes

johnedquinn added 2 commits December 9, 2024 09:29

Merge remote-tracking branch 'upstream/v1' into v1-dml

7d1a594

Converts UpdateTargetStep.Element#key to Literal from ExprLit

cd2f178

johnedquinn merged commit 9b6d7cc into partiql:v1 Dec 9, 2024
7 checks passed

johnedquinn deleted the v1-dml branch December 9, 2024 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds DML (INSERT, DELETE, UPDATE, UPSERT, REPLACE) #1666

Adds DML (INSERT, DELETE, UPDATE, UPSERT, REPLACE) #1666

johnedquinn commented Nov 27, 2024 •

edited

Loading

alancai98 Dec 5, 2024

johnedquinn Dec 6, 2024

alancai98 left a comment

alancai98 Dec 6, 2024

alancai98 Dec 6, 2024

alancai98 commented Dec 6, 2024

johnedquinn commented Dec 7, 2024

alancai98 left a comment

alancai98 Dec 7, 2024

Adds DML (INSERT, DELETE, UPDATE, UPSERT, REPLACE) #1666

Adds DML (INSERT, DELETE, UPDATE, UPSERT, REPLACE) #1666

Conversation

johnedquinn commented Nov 27, 2024 • edited Loading

Relevant Issues & Links

Description

License Information

alancai98 Dec 5, 2024

Choose a reason for hiding this comment

johnedquinn Dec 6, 2024

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

alancai98 Dec 6, 2024

Choose a reason for hiding this comment

alancai98 Dec 6, 2024

Choose a reason for hiding this comment

alancai98 commented Dec 6, 2024

johnedquinn commented Dec 7, 2024

alancai98 left a comment

Choose a reason for hiding this comment

alancai98 Dec 7, 2024

Choose a reason for hiding this comment

johnedquinn commented Nov 27, 2024 •

edited

Loading