Support parsing for attribute and tuple level constraint #1442

yliuuuu · 2024-04-22T23:54:08Z

Relevant Issues

[Closes/Related To] Issue #XXX

Description

Supports parsing of Attribute-Level Constraint (CHECK, NULL, NOT NULL, UNIQUE, PRIMARY KEY)
Supports parsing of Table-Level Constraints (CHECK, UNIQUE, PRIMARY KEY)
It contains breaking changes on the AST modeling. in particular remodeling of constraints to be unnested from table definition.
This is because the constraint can be reused in multiple parts of the DDL operation, (in this PR, attribute level constraint and table level constraints; in the future, we might needs to use constraint in ALTER Statement).

Change log for future reference:

Added

Supports parsing of Attribute-Level Constraint (CHECK, NULL, NOT NULL, UNIQUE, PRIMARY KEY)
Supports parsing of Table-Level Constraints (CHECK, UNIQUE, PRIMARY KEY)

Changed

Modeling of DDL node.
- DDL node is modeled as a wrapper around DDLOp. For example: replace statementDDLCreateTable(...) to statementDDL(ddlOpCreateTable(....))

Other Information

Updated Unreleased Section in CHANGELOG: [YES/NO]
-No. Targeting V1 branch.
Any backward-incompatible changes? [YES/NO]
- Yes. AST modeling.
Any new external dependencies? [YES/NO]
- No.
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]
Yes.

License Information

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

github-actions · 2024-04-23T00:00:57Z

Conformance comparison report-Cross Engine

	Base (legacy)	eval	+/-
% Passing	92.52%	89.91%	-2.61%
✅ Passing	5383	5231	-152
❌ Failing	435	587	152
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5034

Number failing in both: 238

Number passing in legacy engine but fail in eval engine: 349

Number failing in legacy engine but pass in eval engine: 197
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
197 test(s) were failing in legacy but now pass in eval. Before merging, confirm they are intended to pass.
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

Conformance comparison report-Cross Commit-LEGACY

	Base (`cc05d7f`)	`330560b`	+/-
% Passing	92.51%	92.52%	0.02%
✅ Passing	5382	5383	1
❌ Failing	436	435	-1
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5382

Number failing in both: 435

Number passing in Base (cc05d7f) but now fail: 0

Number failing in Base (cc05d7f) but now pass: 1
The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass:

Click here to see

MYSQL_SELECT_29, compileOption: LEGACY

Conformance comparison report-Cross Commit-EVAL

	Base (`cc05d7f`)	`330560b`	+/-
% Passing	89.91%	89.91%	0.00%
✅ Passing	5231	5231	0
❌ Failing	587	587	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0
Number passing in both: 5231

Number failing in both: 587

Number passing in Base (cc05d7f) but now fail: 0

Number failing in Base (cc05d7f) but now pass: 0

alancai98 · 2024-04-24T20:35:39Z

partiql-ast/src/main/resources/partiql_ast.ion

-    },
-  ],
+  d_d_l::{
+    op: ddl_op


out of curiosity, what's the rationale behind creating a separate ddl_op node rather than the previous modeling?

partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserDDLTest.kt

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

partiql-ast/src/main/resources/partiql_ast.ion

alancai98 · 2024-04-24T21:39:13Z

partiql-ast/src/main/resources/partiql_ast.ion

@@ -785,23 +788,27 @@ returning::{
 // `( CONSTRAINT <column_constraint_name> )?  <column_constraint_def>`
 table_definition::{
  columns: list::[column],
+  // table level constraints
+  constraints: list::[constraint],
  _: [
    column::{


(naming) Should we keep the prior modeling as column? In the PR description and the DDL BNF I looked at, the noun you currently use attribute? I think the noun we use should be consistent.

partiql-ast/src/main/resources/partiql_ast.ion

partiql-parser/src/test/kotlin/org/partiql/parser/internal/PartiQLParserDDLTests.kt

Co-authored-by: Alan Cai <[email protected]>

partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt

alancai98 · 2024-04-25T18:52:32Z

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

    ;

 columnConstraintDef
-    : NOT NULL                                  # ColConstrNotNull
-    | NULL                                      # ColConstrNull
+    : NOT NULL                                     # ColConstrNotNull


Since the partiql-ast now uses "attribute" in the table definition, should the ANTLR definitions also use "attribute" rather than "column"? I think we should keep the parser and ast terminology consistent.

Related -- would the PIG parser also need to be updated to use "attribute"? Perhaps it does't matter since the PIG parser is getting deprecated.

I think this is a valid call out. But given the fact that Antlr grammar is an internal concern, perhaps we can treat this as a backlog item?

I don't think we want to introduce changes to the PIG parser.

At some point we would want to update the ANTLR grammar on our website. Perhaps then would be a good time to do those changes in batch?

I think this is a valid call out. But given the fact that Antlr grammar is an internal concern, perhaps we can treat this as a backlog item?

Ah, I hadn't realized the current ANTLR-generated parse code was meant to be "internal" despite the visibility modifiers indicating it was "public". Created an issue to track the internalization of the ANTLR-generated sources -- #1446.

At some point we would want to update the ANTLR grammar on our website. Perhaps then would be a good time to do those changes in batch?

Since we'll likely have other changes to the ANTLR grammar before the DDL syntax is finalized, I'm fine w/ punting on the changes. Could you create an issue to track updating the website grammar and grammar within partiql-parser? That should make it easier to track what'll need changed before the next release.

partiql-ast/src/main/resources/partiql_ast.ion

…)" This reverts commit abfc58d

* Revert "V1 ddl extended keyword (#1455)" This reverts commit 2879f3a * Revert "struct subfield and list element type (#1449)" This reverts commit 23f6fee * Revert "run apiDump (#1447)" This reverts commit 607c4c0 * Revert "Support parsing for attribute and tuple level constraint (#1442)" This reverts commit abfc58d * fix post-revert build * submodule

Parsing for attribute and tuple constraint

e98bdae

yliuuuu requested a review from alancai98 April 23, 2024 16:44

yliuuuu marked this pull request as ready for review April 23, 2024 16:44

alancai98 reviewed Apr 24, 2024

View reviewed changes

yliuuuu and others added 2 commits April 24, 2024 17:57

Update partiql-parser/src/main/antlr/PartiQL.g4

023a5c8

Co-authored-by: Alan Cai <[email protected]>

address feedback

51e3a0f

yliuuuu requested a review from alancai98 April 25, 2024 17:04

alancai98 reviewed Apr 25, 2024

View reviewed changes

alancai98 approved these changes Apr 25, 2024

View reviewed changes

yliuuuu merged commit abfc58d into v1 Apr 25, 2024
14 checks passed

yliuuuu added a commit that referenced this pull request Jul 25, 2024

Revert "Support parsing for attribute and tuple level constraint (#1442…

8c354c6

…)" This reverts commit abfc58d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support parsing for attribute and tuple level constraint #1442

Support parsing for attribute and tuple level constraint #1442

yliuuuu commented Apr 22, 2024

github-actions bot commented Apr 23, 2024 •

edited

Loading

alancai98 Apr 24, 2024

alancai98 Apr 24, 2024

alancai98 Apr 25, 2024 •

edited

Loading

yliuuuu Apr 25, 2024

yliuuuu Apr 25, 2024

alancai98 Apr 25, 2024

Support parsing for attribute and tuple level constraint #1442

Support parsing for attribute and tuple level constraint #1442

Conversation

yliuuuu commented Apr 22, 2024

Relevant Issues

Description

Change log for future reference:

Added

Changed

Other Information

License Information

github-actions bot commented Apr 23, 2024 • edited Loading

Conformance comparison report-Cross Engine

Conformance comparison report-Cross Commit-LEGACY

Conformance comparison report-Cross Commit-EVAL

alancai98 Apr 24, 2024

Choose a reason for hiding this comment

alancai98 Apr 24, 2024

Choose a reason for hiding this comment

alancai98 Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

yliuuuu Apr 25, 2024

Choose a reason for hiding this comment

yliuuuu Apr 25, 2024

Choose a reason for hiding this comment

alancai98 Apr 25, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024 •

edited

Loading

alancai98 Apr 25, 2024 •

edited

Loading