Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support parsing for attribute and tuple level constraint #1442

Merged
merged 3 commits into from
Apr 25, 2024
Merged

Support parsing for attribute and tuple level constraint #1442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e98bdae
Parsing for attribute and tuple constraint
yliuuuu Apr 22, 2024
023a5c8
Update partiql-parser/src/main/antlr/PartiQL.g4
yliuuuu Apr 25, 2024
51e3a0f
address feedback
yliuuuu Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 26 additions & 25 deletions partiql-ast/src/main/kotlin/org/partiql/ast/helpers/ToLegacyAst.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import com.amazon.ionelement.api.ionString
import com.amazon.ionelement.api.ionSymbol
import com.amazon.ionelement.api.metaContainerOf
import org.partiql.ast.AstNode
import org.partiql.ast.Constraint
import org.partiql.ast.DatetimeField
import org.partiql.ast.DdlOp
import org.partiql.ast.Exclude
import org.partiql.ast.Expr
import org.partiql.ast.From
Expand Down Expand Up @@ -121,24 +123,23 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
domain(statement, type, format, metas)
}

override fun visitStatementDDL(node: Statement.DDL, ctx: Ctx) = super.visit(node, ctx) as PartiqlAst.Statement.Ddl
override fun visitStatementDDL(node: Statement.DDL, ctx: Ctx) = when (val op = node.op) {
is DdlOp.CreateIndex -> visitDdlOpCreateIndex(op, ctx)
is DdlOp.CreateTable -> visitDdlOpCreateTable(op, ctx)
is DdlOp.DropIndex -> visitDdlOpDropIndex(op, ctx)
is DdlOp.DropTable -> visitDdlOpDropTable(op, ctx)
}

override fun visitStatementDDLCreateTable(
node: Statement.DDL.CreateTable,
ctx: Ctx,
) = translate(node) { metas ->
override fun visitDdlOpCreateTable(node: DdlOp.CreateTable, ctx: Ctx) = translate(node) { metas ->
if (node.name !is Identifier.Symbol) {
error("The legacy AST does not support qualified identifiers as table names")
}
val tableName = (node.name as Identifier.Symbol).symbol
val tableName = node.name.symbol
val def = node.definition?.let { visitTableDefinition(it, ctx) }
ddl(createTable(tableName, def), metas)
}

override fun visitStatementDDLCreateIndex(
node: Statement.DDL.CreateIndex,
ctx: Ctx,
) = translate(node) { metas ->
override fun visitDdlOpCreateIndex(node: DdlOp.CreateIndex, ctx: Ctx) = translate(node) { metas ->
if (node.index != null) {
error("The legacy AST does not support index names")
}
Expand All @@ -150,7 +151,7 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
ddl(createIndex(tableName, fields), metas)
}

override fun visitStatementDDLDropTable(node: Statement.DDL.DropTable, ctx: Ctx) = translate(node) { metas ->
override fun visitDdlOpDropTable(node: DdlOp.DropTable, ctx: Ctx) = translate(node) { metas ->
if (node.table !is Identifier.Symbol) {
error("The legacy AST does not support qualified identifiers as table names")
}
Expand All @@ -159,7 +160,7 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
ddl(dropTable(tableName), metas)
}

override fun visitStatementDDLDropIndex(node: Statement.DDL.DropIndex, ctx: Ctx) = translate(node) { metas ->
override fun visitDdlOpDropIndex(node: DdlOp.DropIndex, ctx: Ctx) = translate(node) { metas ->
if (node.index !is Identifier.Symbol) {
error("The legacy AST does not support qualified identifiers as index names")
}
Expand All @@ -174,28 +175,28 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
}

override fun visitTableDefinition(node: TableDefinition, ctx: Ctx) = translate(node) { metas ->
val parts = node.columns.translate<PartiqlAst.TableDefPart>(ctx)
val parts = node.attributes.translate<PartiqlAst.TableDefPart>(ctx)
if (node.constraints.isNotEmpty()) {
error("The legacy AST does not support table level constraint declaration")
}
tableDef(parts, metas)
}

override fun visitTableDefinitionColumn(node: TableDefinition.Column, ctx: Ctx) = translate(node) { metas ->
val name = node.name
override fun visitTableDefinitionAttribute(node: TableDefinition.Attribute, ctx: Ctx) = translate(node) { metas ->
// Legacy AST treat table name as a case-sensitive string
val name = node.name.symbol
val type = visitType(node.type, ctx)
val constraints = node.constraints.translate<PartiqlAst.ColumnConstraint>(ctx)
columnDeclaration(name, type, constraints, metas)
}

override fun visitTableDefinitionColumnConstraint(
node: TableDefinition.Column.Constraint,
ctx: Ctx,
) = translate(node) { metas ->
override fun visitConstraint(node: Constraint, ctx: Ctx) = translate(node) {
val name = node.name
val def = when (node.body) {
is TableDefinition.Column.Constraint.Body.Check -> {
throw IllegalArgumentException("PIG AST does not support CHECK (<expr>) constraint")
}
is TableDefinition.Column.Constraint.Body.NotNull -> columnNotnull()
is TableDefinition.Column.Constraint.Body.Nullable -> columnNull()
val def = when (node.definition) {
is Constraint.Definition.Check -> throw IllegalArgumentException("PIG AST does not support CHECK (<expr>) constraint")
is Constraint.Definition.NotNull -> columnNotnull()
is Constraint.Definition.Nullable -> columnNull()
is Constraint.Definition.Unique -> throw IllegalArgumentException("PIG AST does not support Unique/Primary Key constraint")
}
columnConstraint(name, def, metas)
}
Expand Down
91 changes: 48 additions & 43 deletions partiql-ast/src/main/resources/partiql_ast.ion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,32 +106,9 @@ statement::[
],

// Data Definition Language
d_d_l::[

// CREATE TABLE <identifier> [<table_def>]
create_table::{
name: identifier,
definition: optional::table_definition,
},

// CREATE INDEX [<identifier>] ON <identifier> (<path> [, <path>]...)
create_index::{
index: optional::identifier,
table: identifier,
fields: list::[path],
},

// DROP TABLE <identifier>
drop_table::{
table: identifier,
},

// DROP INDEX <identifier> ON <identifier>
drop_index::{
index: identifier, // <identifier>[0]
table: identifier, // <identifier>[1]
},
],
d_d_l::{
op: ddl_op
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},

// EXEC <symbol> [<expr>.*]
exec::{
Expand All @@ -151,6 +128,32 @@ statement::[
},
]

ddl_op::[
// CREATE TABLE <identifier> [<table_def>]
create_table::{
name: identifier,
definition: optional::table_definition,
},

// CREATE INDEX [<identifier>] ON <identifier> (<path> [, <path>]...)
create_index::{
index: optional::identifier,
table: identifier,
fields: list::[path],
},

// DROP TABLE <identifier>
drop_table::{
table: identifier,
},

// DROP INDEX <identifier> ON <identifier>
drop_index::{
index: identifier, // <identifier>[0]
table: identifier, // <identifier>[1]
},
]

// PartiQL Type AST nodes
//
// Several of these are the same "type", but have various syntax rules we wish to capture.
Expand Down Expand Up @@ -781,27 +784,29 @@ returning::{
],
}

// `<column_name> <type> <column_constraint>*`
// `( CONSTRAINT <column_constraint_name> )? <column_constraint_def>`
table_definition::{
columns: list::[column],
attributes: list::[attribute],
// table level constraints
constraints: list::[constraint],
_: [
column::{
name: string,
attribute::{
name: '.identifier.symbol',
type: '.type',
constraints: list::[constraint],
_: [
// TODO improve modeling language to avoid these wrapped unions
// Also, prefer not to nest more than twice
constraint::{
name: optional::string,
body: [
nullable::{},
not_null::{},
check::{ expr: expr },
],
},
],
}
],
}

constraint::{
alancai98 marked this conversation as resolved.
Show resolved Hide resolved
name: optional::string,
alancai98 marked this conversation as resolved.
Show resolved Hide resolved
definition: [
nullable::{},
not_null::{},
check::{ expr: expr },
unique::{
// for attribute level constraint, we can set this attribute to null
attributes: optional::list::['.identifier.symbol'],
is_primary_key: bool,
},
],
}
Expand Down
2 changes: 1 addition & 1 deletion partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ internal class PartiQLPigVisitor(
}

override fun visitColumnConstraint(ctx: PartiQLParser.ColumnConstraintContext) = PartiqlAst.build {
val name = ctx.columnConstraintName()?.let { visitSymbolPrimitive(it.symbolPrimitive()).name.text }
val name = ctx.constraintName()?.let { visitSymbolPrimitive(it.symbolPrimitive()).name.text }
val def = visit(ctx.columnConstraintDef()) as PartiqlAst.ColumnConstraintDef
columnConstraint(name, def)
}
Expand Down
42 changes: 41 additions & 1 deletion partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserDDLTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,47 @@ internal class PartiQLParserDDLTest : PartiQLParserTestBase() {
query = "DROP Table foo.bar",
code = ErrorCode.PARSE_UNEXPECTED_TOKEN,
context = mapOf(),
)
),
alancai98 marked this conversation as resolved.
Show resolved Hide resolved
ParserErrorTestCase(
description = "PIG Parser does not support Unique Constraints in CREATE TABLE",
query = """
CREATE TABLE tbl (
a INT2 UNIQUE
)
""".trimIndent(),
code = ErrorCode.PARSE_UNEXPECTED_TOKEN,
context = mapOf(),
),
ParserErrorTestCase(
description = "PIG Parser does not support Primary Key Constraint in CREATE TABLE",
query = """
CREATE TABLE tbl (
a INT2 PRIMARY KEY
)
""".trimIndent(),
code = ErrorCode.PARSE_UNEXPECTED_TOKEN,
context = mapOf(),
),
ParserErrorTestCase(
description = "PIG Parser does not support CHECK Constraint in CREATE TABLE",
query = """
CREATE TABLE tbl (
a INT2 CHECK(a > 0)
)
""".trimIndent(),
code = ErrorCode.PARSE_UNEXPECTED_TOKEN,
context = mapOf(),
),
ParserErrorTestCase(
description = "PIG Parser does not support table constraint in CREATE TABLE",
query = """
CREATE TABLE tbl (
check (a > 0)
)
""".trimIndent(),
code = ErrorCode.PARSE_UNEXPECTED_TOKEN,
context = mapOf(),
),
)
}
}
38 changes: 30 additions & 8 deletions partiql-parser/src/main/antlr/PartiQL.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,8 @@ execCommand
qualifiedName : (qualifier+=symbolPrimitive PERIOD)* name=symbolPrimitive;

tableName : symbolPrimitive;
tableConstraintName : symbolPrimitive;
columnName : symbolPrimitive;
columnConstraintName : symbolPrimitive;
constraintName : symbolPrimitive;

ddl
: createCommand
Expand All @@ -100,17 +99,43 @@ tableDef

tableDefPart
: columnName type columnConstraint* # ColumnDeclaration
| ( CONSTRAINT constraintName )? tableConstraintDef # TableConstrDeclaration
;

tableConstraintDef
: checkConstraintDef # TableConstrCheck
| uniqueConstraintDef # TableConstrUnique
;

columnConstraint
: ( CONSTRAINT columnConstraintName )? columnConstraintDef
: ( CONSTRAINT constraintName )? columnConstraintDef
;

columnConstraintDef
: NOT NULL # ColConstrNotNull
| NULL # ColConstrNull
: NOT NULL # ColConstrNotNull
Copy link
Member

@alancai98 alancai98 Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| NULL # ColConstrNull
| uniqueSpec # ColConstrUnique
| checkConstraintDef # ColConstrCheck
;

checkConstraintDef
: CHECK PAREN_LEFT searchCondition PAREN_RIGHT
;

uniqueSpec
: PRIMARY KEY # PrimaryKey
| UNIQUE # Unique
;

uniqueConstraintDef
: uniqueSpec PAREN_LEFT columnName (COMMA columnName)* PAREN_RIGHT
;

// <search condition> ::= <boolean term> | <search condition> OR <boolean term>
// we cannot do exactly that for the way expression precedence is structured in the grammar file.
// but we at least can eliminate SFW query here.
searchCondition : exprOr;

/**
*
* DATA MANIPULATION LANGUAGE (DML)
Expand Down Expand Up @@ -192,9 +217,6 @@ conflictTarget
: PAREN_LEFT symbolPrimitive (COMMA symbolPrimitive)* PAREN_RIGHT
| ON CONSTRAINT constraintName;

constraintName
: symbolPrimitive;

conflictAction
: DO NOTHING
| DO REPLACE doReplace
Expand Down
Loading
Loading