Fixes parser for the top level tokens #369

abhikuhikar · 2021-02-26T01:07:17Z

Issue #363

Fixes parser by traversing and validating the parse tree to ensure that the top level tokens are not present at the level(s) below.

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

codecov-io · 2021-02-26T01:09:14Z

Codecov Report

Merging #369 (f375d2d) into master (a8c13ea) will decrease coverage by 0.00%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master     #369      +/-   ##
============================================
- Coverage     81.76%   81.75%   -0.01%     
- Complexity     1237     1243       +6     
============================================
  Files           162      162              
  Lines          9592     9648      +56     
  Branches       1560     1566       +6     
============================================
+ Hits           7843     7888      +45     
- Misses         1272     1281       +9     
- Partials        477      479       +2

Flag	Coverage Δ	Complexity Δ
CLI	`18.30% <ø> (ø)`	`0.00 <ø> (ø)`
EXAMPLES	`74.85% <ø> (ø)`	`0.00 <ø> (ø)`
LANG	`84.37% <71.42%> (-0.03%)`	`0.00 <7.00> (ø)`
PTS	`100.00% <ø> (ø)`	`0.00 <ø> (ø)`
TEST_SCRIPT	`79.68% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lang/src/org/partiql/lang/errors/ErrorCode.kt	`83.25% <ø> (+0.48%)`	`7.00 <0.00> (+1.00)`
lang/src/org/partiql/lang/syntax/SqlParser.kt	`80.15% <71.42%> (-0.25%)`	`243.00 <7.00> (+5.00)`	⬇️
lang/src/org/partiql/lang/domains/util.kt	`65.00% <0.00%> (-1.67%)`	`0.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8c13ea...f375d2d. Read the comment docs.

alancai98

The approach lgtm overall. Could add more top-level token checks to EXEC and other DML/DDL ops. Also may need more tests.

alancai98 · 2021-03-01T17:36:49Z

lang/test/org/partiql/lang/syntax/SqlParserTest.kt

@@ -2636,6 +2636,32 @@ class SqlParserTest : SqlParserTestBase() {
        """
    )

+    @Test
+    fun fromWhereSetDml() = assertExpression(


Is there any difference between this test and fromSetSingleDml in SqlParserTest.kt? They seem to be the same.

Ohh yes. I missed that. Thanks. I will remove it.

alancai98 · 2021-03-01T17:37:27Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+    /**
+     * Checks if the [ParseNode] requires to be only at top level. DML keyword tokens, for example, requires to be top level.
+     */
+    private fun ParseNode.isTopLevelType() = type in listOf(UPDATE, DELETE, INSERT)


Should REMOVE also be added?

alancai98 · 2021-03-01T17:43:05Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+    /**
+     * Checks if the [ParseNode] requires to be only at top level. DML keyword tokens, for example, requires to be top level.
+     */


Could DDL keywords also be added?

Also, there was a previous top-level token implementation that checked for stored procedure calls (EXEC). I think your approach is better. Could the EXEC keyword also be checked here?

Added EXEC.

Sorry, I may have put too much in the first comment, but will DDL keywords also be added?

Yes. I have updated the list to include DDL keywords as well.

alancai98 · 2021-03-01T17:45:35Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+    private fun ParseNode.isTopLevelType() = type in listOf(UPDATE, DELETE, INSERT)
+
+    /**
+     * Validates tree to make sure that the top level tokens are not found below the top level


nit: "below" could be changed to "outside"

Since it is a tree representation, "below" makes sense IMO.

alancai98 · 2021-03-01T17:53:59Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+        // Note that for DML operations, top level parse node is of type 'FROM'. Hence the check level > 1
+        if (level > 1 && node.isTopLevelType()) {
+            node.remaining.err("Type ${node.type} only expected at the top level", PARSE_UNEXPECTED_TERM)
+        }


If we add more top level tokens to check, may need to case on whether the node is a DML op.

I think the logic remains the same for the top level tokens even if it is DML or not.

alancai98 · 2021-03-01T17:55:04Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

@@ -1246,6 +1246,16 @@ class ParserErrorsTest : TestBase() {
            Property.TOKEN_TYPE to TokenType.EOF,
            Property.TOKEN_VALUE to ion.newSymbol("EOF")))

+    @Test


Could add more tests. At least one for each top level token we're checking.

Thanks. Added tests for other top level tokens.

alancai98 · 2021-03-01T18:03:03Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

+            Property.TOKEN_TYPE to TokenType.EOF,
+            Property.TOKEN_VALUE to ion.newSymbol("EOF")))


Shouldn't the unexpected parse term be SET here and not EOF?

You are right. Thanks. I have made changes in the next revision.

alancai98

Just a few minor nits for the comments/kdoc I missed from the first review. I probably overloaded this comment from the first review, but will DDL ops also be included?

alancai98 · 2021-03-04T00:02:58Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

-            Property.TOKEN_VALUE to ion.newSymbol("exec")))
+            Property.COLUMN_NUMBER to 21L,
+            Property.TOKEN_TYPE to TokenType.IDENTIFIER,
+            Property.TOKEN_VALUE to ion.newSymbol("undrop")))


Shouldn't the unexpected term be EXEC here?

It should be. But the associated token with the EXEC ParseNode is the name of the stored procedure. Hence, the issue. To get around with this, I think we can pass the name of the stored procedure in the arguments along with the other arglist or some such. But that's worthy of a separate issue.

If the EXEC parsing should be changed (but won't be changed in this PR), can we add a comment linking to an issue to track this?

alancai98 · 2021-03-04T00:18:19Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

-     *
-     * Currently only checks for `EXEC`. DDL and DML along with corresponding error codes to be added in the future
-     * (https://github.com/partiql/partiql-lang-kotlin/issues/354).
+     * Checks if the [ParseNode] requires to be only at top level. DML keyword tokens, for example, requires to be top level.


super nit: at least for me the kdoc wording is confusing. From the implementation, it checks the ParseNode's parse type is one of the top level types right? The first sentence could be something like:

/** * Returns true if and only if this [ParseType] is one of the top level types... */

For the second sentence, it might be helpful to also mention that DML can have FROM as the top-level node.

I will update it. Thanks.

alancai98 · 2021-03-04T00:27:55Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+     * Validates tree to make sure that the top level tokens are not found below the top level
+     */
+    private fun validateTopLevelNodes(node: ParseNode, level: Int) {
+        // Note that for DML operations, top level parse node is of type 'FROM'. Hence the check level > 1


super nit: the top level parse node could be of type FROM (not necessary for all DML ops)

It is for all DML ops if I am not wrong. I have updated the logic to take this into consideration.

From the partiql.ion grammar for DML ops, the FROM source is optional.

abhikuhikar · 2021-03-04T04:52:20Z

Updated the logic to check for DDL commands as well.
Changed ParseType to include properties isTopLevelType and isDml.
Added tests to cover DDL and DML top level token types.

alancai98 · 2021-03-05T01:18:39Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+            node.token?.err("Type ${node.type} only expected at the top level", PARSE_UNEXPECTED_TERM)
+                ?: throw ParserException("Type ${node.type} only expected at the top level", PARSE_UNEXPECTED_TERM, PropertyValueMap())


nit: I think the word "Type" shouldn't be included in the error message. Maybe it could be replaced with "Keyword"?

So the parseType is different from the keyword or token, fortunately or unfortunately. I guess we can remove the error message - type only expected at top level altogether and just stay with the unexpected term found error message.

Changed the error msg.

alancai98 · 2021-03-05T01:29:29Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

-            }
+    private fun validateTopLevelNodes(node: ParseNode, level: Int) {
+        // Note that for DML operations, top level parse node is of type 'FROM'. Hence the check level > 1
+        if (node.type.isTopLevelType && ((node.type.isDml && level > 1) || (!node.type.isDml && level > 0))) {


nit: This can be a little hard to understand with so many logic symbols. Can this be simplified or broken into multiple if-statements?

It will make the code redundant. I also thought the conditional statement is complicated initially but on the second glance it is not that complicated.

if (node.type.isTopLevelType && level > 0) { if (node.type.isDml) { if (level > 1) { // throw error } } else { // throw error } }

Changed as mentioned above.

alancai98 · 2021-03-05T01:33:18Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+    /** If the property [isTopLevelType] is true, then the parse node of this ParseType will only be valid at top level in the query.
+     *  For example, EXEC, DDL and DML keywords can only be used at the top level in the query.
+     */
+    internal enum class ParseType(val isJoin: Boolean = false, val isTopLevelType: Boolean = false, val isDml: Boolean = false) {


How is the isDml parameter defined? SET is a DML op, yet ParseType.SET isDml is false.

This is a legit observation. However, the SET parseType is not used anywhere. We can actually get rid of it.

Changed the property isDml to true for SET.

alancai98 · 2021-03-05T01:37:04Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

@@ -1246,6 +1246,96 @@ class ParserErrorsTest : TestBase() {
            Property.TOKEN_TYPE to TokenType.EOF,
            Property.TOKEN_VALUE to ion.newSymbol("EOF")))

+    @Test
+    fun updateWithNestedSet() = checkInputThrowingParserException(
+        "UPDATE test SET x = SET test.y = 6",


So when I'm testing this query in the REPL, it gives the following:

PartiQL> UPDATE test SET x = SET test.y = 6; org.partiql.lang.syntax.ParserException: Type UPDATE only expected at the top level Parser Error: at line 1, column 21: unexpected term found, KEYWORD : set

Shouldn't the 2nd line say SET and not UPDATE? I haven't had a chance to test w/ the other top-level tokens.

The associated parse type is UPDATE.
I will change this to - "Keyword ${node.token} only expected at the top level".
It makes more sense I guess and would be less confusing.

alancai98 · 2021-03-05T01:40:55Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+     * Validates tree to make sure that the top level tokens are not found below the top level
+     */
+    private fun validateTopLevelNodes(node: ParseNode, level: Int) {
+        // Note that for DML operations, top level parse node is of type 'FROM'. Hence the check level > 1


From the partiql.ion grammar for DML ops, the FROM source is optional.

abhikuhikar · 2021-03-05T05:06:23Z

Came across interesting edge case where the DML tokens are at top level. Updated the PR to handle these edge cases.

alancai98

Just have a few minor nits and questions. The rest lgtm

alancai98 · 2021-03-08T18:44:59Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

@@ -2244,25 +2253,39 @@ class SqlParser(private val ion: IonSystem) : Parser {
        return ParseNode(ARG_LIST, null, items, rem)
    }

+    private fun ParseNode.throwTopLevelParserError(): Nothing =
+        token?.err("Keyword $token only expected at the top level in the query", PARSE_UNEXPECTED_TERM)


Currently, in the REPL, this outputs the string representation of token:

PartiQL> UPDATE test SET x = SET test.y = 6; org.partiql.lang.syntax.ParserException: Keyword Token(type=KEYWORD, value=set, position=line 1, column 21) only expected at the top level in the query Parser Error: at line 1, column 21: unexpected term found, KEYWORD : set

Should ${token.text} be used here instead of $token to output just the keyword? The following is what's output using ${token.text}:

PartiQL> UPDATE test SET x = SET test.y = 6; org.partiql.lang.syntax.ParserException: Keyword set only expected at the top level in the query Parser Error: at line 1, column 21: unexpected term found, KEYWORD : set

Makes sense. Thanks. Changed it.

alancai98 · 2021-03-08T18:51:08Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

-            Property.TOKEN_VALUE to ion.newSymbol("exec")))
+            Property.COLUMN_NUMBER to 21L,
+            Property.TOKEN_TYPE to TokenType.IDENTIFIER,
+            Property.TOKEN_VALUE to ion.newSymbol("undrop")))


If the EXEC parsing should be changed (but won't be changed in this PR), can we add a comment linking to an issue to track this?

alancai98 · 2021-03-08T18:59:03Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

-     * an error if so.
-     *
-     * Currently only checks for `EXEC`. DDL and DML along with corresponding error codes to be added in the future
-     * (https://github.com/partiql/partiql-lang-kotlin/issues/354).
+     * Validates tree to make sure that the top level tokens are not found below the top level
     */
-    private fun List<Token>.checkUnexpectedTopLevelToken() {
-        for ((index, token) in this.withIndex()) {
-            if (token.keywordText == "exec" && index != 0) {
-                token.err("EXEC call found at unexpected location", PARSE_EXEC_AT_UNEXPECTED_LOCATION)
+    private fun validateTopLevelNodes(node: ParseNode, level: Int, topLevelTokens: Int) {


nit: could add more documentation for what level and topLevelTokens represent. Also, is Int the best representation for what it's used for? Perhaps Boolean is better along with a more descriptive name such as topLevelTokenSeen.

Doesn't make a significant difference but you are right. This can be done with Boolean.

alancai98 · 2021-03-08T19:01:34Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

@@ -2272,6 +2295,9 @@ class SqlParser(private val ion: IonSystem) : Parser {
                else      -> rem.err("Unexpected token after expression", PARSE_UNEXPECTED_TOKEN)
            }
        }
+
+        validateTopLevelNodes(node, 0, 0)


nit: could add names to call args

validateTopLevelNodes(node = node, level = 0, topLevelTokens = 0)

alancai98 · 2021-03-08T19:02:09Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

            }
        }
+        node.children.map { validateTopLevelNodes(it, level + 1, topTokens) }


nit: could add names to call args:

node.children.map { validateTopLevelNodes(node = it, level = level + 1, topLevelTokens = topTokens) }

alancai98 · 2021-03-08T19:03:23Z

lang/test/org/partiql/lang/errors/ParserErrorsTest.kt

+    @Test
+    fun nestedRemove() = checkInputThrowingParserException(
+        "REMOVE REMOVE y",
+        ErrorCode.PARSE_INVALID_PATH_COMPONENT,


Is there a reason this error is not PARSE_UNEXPECTED_TERM?

This is handled actively by the parser while parsing the REMOVE keyword.

alancai98

Couple very minor nits for the kdoc of validateTopLevelNodes. Rest lgtm

alancai98 · 2021-03-08T23:20:38Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

-     * Validates tree to make sure that the top level tokens are not found below the top level
+     * Validates tree to make sure that the top level tokens are not found below the top level.
+     * Top level tokens are the tokens or keywords which are valid to be used only at the top level in the query.
+     * i.e. these tokens cannot be used with a mix of other commands. Hence if more than one top level tokens are found


nit: "i.e. these tokens cannot be used with a mix of other ~~commands~~ top level tokens"? "commands" seems too generalized.

alancai98 · 2021-03-08T23:29:26Z

lang/src/org/partiql/lang/syntax/SqlParser.kt

+     * i.e. these tokens cannot be used with a mix of other commands. Hence if more than one top level tokens are found
+     * in the query then it is invalid.
+     * [level] is the current traversal level in the parse tree.
+     * If [topLevelTokenSeen] is true, it means it has been encountered at least once before while traversing the parse tree.


nit: description of topLevelTokenSeen could be more more specific. Consider "...it means a top level token has been previously encountered while traversing the parse tree"

I believe the variable name itself tells it all.

abhikuhikar added 2 commits February 25, 2021 17:57

Fixed parser for the top level tokens

7d7e2e9

Small changes

caebc9f

abhikuhikar requested review from therapon, dlurton and alancai98 February 26, 2021 01:07

alancai98 requested changes Mar 1, 2021

View reviewed changes

Added more tests and more top level checks

1a8ed3d

alancai98 reviewed Mar 4, 2021

View reviewed changes

Updated algorithm to include DDL parse types

fec50de

alancai98 reviewed Mar 5, 2021

View reviewed changes

Added logic to handle top level DML tokens

a9de025

alancai98 reviewed Mar 8, 2021

View reviewed changes

Addressed comments

f375d2d

alancai98 approved these changes Mar 8, 2021

View reviewed changes

abhikuhikar merged commit 82c0032 into master Mar 8, 2021

abhikuhikar mentioned this pull request Mar 9, 2021

Add more parser error tests to validate top level tokens #373

Merged

dlurton added this to the v0.3.0 milestone Jun 9, 2021

alancai98 deleted the fix-parser-for-top-level-types branch September 13, 2021 18:20

abhikuhikar mentioned this pull request Dec 20, 2021

Add general parser checks for top-level only tokens (i.e. DML and DDL ops) #354

Closed

		Property.TOKEN_TYPE to TokenType.EOF,
		Property.TOKEN_VALUE to ion.newSymbol("EOF")))

		node.token?.err("Type ${node.type} only expected at the top level", PARSE_UNEXPECTED_TERM)
		?: throw ParserException("Type ${node.type} only expected at the top level", PARSE_UNEXPECTED_TERM, PropertyValueMap())

Fixes parser for the top level tokens #369

Fixes parser for the top level tokens #369

Conversation

abhikuhikar commented Feb 26, 2021

codecov-io commented Feb 26, 2021 • edited Loading

Codecov Report

alancai98 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

abhikuhikar Mar 4, 2021 • 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

alancai98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhikuhikar Mar 4, 2021 • 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

abhikuhikar commented Mar 4, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhikuhikar commented Mar 5, 2021

alancai98 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancai98 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

codecov-io commented Feb 26, 2021 •

edited

Loading

abhikuhikar Mar 4, 2021 •

edited

Loading

abhikuhikar Mar 4, 2021 •

edited

Loading