Add to non-reserved keywords; rework functionName and symbolPrimitive parse rules
#1457
Conversation
Conformance comparison report-Cross Engine
Number failing in both: 229 Number passing in legacy engine but fail in eval engine: 312 Number failing in legacy engine but pass in eval engine: 207 Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 435 Number passing in Base (2879f3a) but now fail: 1 Number failing in Base (2879f3a) but now pass: 0 Click here to see
Conformance comparison report-Cross Commit-EVAL
Number failing in both: 541 Number passing in Base (2879f3a) but now fail: 1 Number failing in Base (2879f3a) but now pass: 1 Click here to see
Click here to see
|
partiql-lang/src/test/kotlin/org/partiql/lang/errors/ParserErrorsTest.kt
Show resolved
Hide resolved
| override fun visitQuotedIdentifier(ctx: org.partiql.parser.internal.antlr.PartiQLParser.QuotedIdentifierContext): Identifier.Symbol = translate(ctx) { | ||
| identifierSymbol( | ||
| ctx.IDENTIFIER_QUOTED().getStringValue(), | ||
| Identifier.CaseSensitivity.SENSITIVE | ||
| ) | ||
| } | ||
|
|
||
| override fun visitUnquotedIdentifier(ctx: org.partiql.parser.internal.antlr.PartiQLParser.UnquotedIdentifierContext): Identifier.Symbol = translate(ctx) { | ||
| identifierSymbol( | ||
| ctx.text, | ||
| Identifier.CaseSensitivity.INSENSITIVE | ||
| ) | ||
| } |
There was a problem hiding this comment.
Hmm, why do we need the visitAs when we have explicit return types? I wonder if adding the two alternatives caused this. I wonder if we should define our own getIdentifier that is more efficient than visitAs.
So it would be like
fun identifier(ctx: ParserRuleContext): Identifier = when (ctx) {
is IdentifierQuotedContext -> identifier(ctx)
is IdentifierUnquotedContext -> identifier(ctx)
}
fun identifier(ctx: IdentifierQuotedContext): Identifier = ....
fun identifier(ctx: IdentifierUquotedContext): Identifier.Symbol = ....We also should take care where qualified names appear and make sure our grammar rules follow this.
There was a problem hiding this comment.
Like, I think we should be replacing visitSymbolPrimitive with visitIdentifierUnquoted and it shouldn't be anymore than a name change. Is there a reason this did not work?
There was a problem hiding this comment.
There wasn't a reason this did not work previously. I was just following the visitAs<...> () pattern I often saw in the visitors. I'll update to use a getIdentifier (or the like) rather than visitAs.
There was a problem hiding this comment.
New commit 445be01 uses the private visitSymbolPrimitive function which calls the corresponding visitIdentifierQuoted and visitIdentifierUnquoted. Also changed some existing usages of visitAs<Identifier.Symbol> to use visitSymbolPrimitive.
| @@ -766,8 +768,7 @@ functionCall | |||
|
|
|||
| // SQL-99 10.4 — <routine name> ::= [ <schema name> <period> ] <qualified identifier> | |||
| functionName | |||
| : (qualifier+=symbolPrimitive PERIOD)* name=( CHAR_LENGTH | CHARACTER_LENGTH | OCTET_LENGTH | BIT_LENGTH | UPPER | LOWER | SIZE | EXISTS | COUNT | MOD ) # FunctionNameReserved | |||
There was a problem hiding this comment.
(self-review) -- this is the rule we were aiming to remove from the ANTLR grammar in this PR
| | qualifier=AT_SIGN? key=nonReserved # VariableKeyword | ||
| ; | ||
|
|
||
| nonReserved |
There was a problem hiding this comment.
(self-review) -- the non-reserved words were taken directly from the SQL-99 grammar. At the end are some, PartiQL specific-keywords marked as non-reserved.
If you'll notice, postgresql along with other SQL dialects use their own set of non-reserved keywords that diverges from the SQL standards. Also between different SQL standards, there are some changes to the reserved vs non-reserved keywords. We choose to follow SQL-99 as we do in other places in the project.
| functionName | ||
| : (qualifier+=symbolPrimitive PERIOD)* name=( CHAR_LENGTH | CHARACTER_LENGTH | OCTET_LENGTH | BIT_LENGTH | UPPER | LOWER | SIZE | EXISTS | COUNT | MOD ) # FunctionNameReserved | ||
| | (qualifier+=symbolPrimitive PERIOD)* name=symbolPrimitive # FunctionNameSymbol | ||
| : (qualifier+=symbolPrimitive PERIOD)* name=symbolPrimitive |
There was a problem hiding this comment.
I believe you can go one step further and replace uses of functionName with qualifiedName.
There was a problem hiding this comment.
Ah you're right. Didn't realize the definition was the same. Replaced usages in 7b615f8
partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt
Show resolved
Hide resolved
Description
functionName-- gets rid of hard-coded reserved keywords as function names blocksymbolPrimitive-- addsnonReservedkeywords to this ruleOther Information
Updated Unreleased Section in CHANGELOG: [NO]
Any backward-incompatible changes? [NO?]
Any new external dependencies? [NO]
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES]
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.