Unused Returning Clause in Parser

[johnedquinn]

Description

While working on replacing the SqlParser with our new PartiQLParser on branch ANTLR, I came across a weird scenario in the existing parser.

Essentially, as part of DML, the UPDATE keyword allows you to wrap any base DML (such as INSERT) with a RETURNING clause. Oddly, however, INSERT also allows for a RETURNING clause -- which made me think of what it would result in if we were to use both in a single query.

See Example #001 below:

PartiQL> UPDATE a INSERT INTO B VALUE C RETURNING MODIFIED NEW D RETURNING MODIFIED NEW E
   | !!

and here's the result:

(
  dml
  (
    operations
    (
      dml_op_list
      (
        insert_value
        (
          id
          B
          (
            case_insensitive
          )
          (
            unqualified
          )
        )
        (
          id
          C
          (
            case_insensitive
          )
          (
            unqualified
          )
        )
        null
        null
      )
    )
  )
  (
    from
    (
      scan
      (
        id
        a
        (
          case_insensitive
        )
        (
          unqualified
        )
      )
      null
      null
      null
    )
  )
  (
    returning
    (
      returning_expr
      (
        returning_elem
        (
          modified_new
        )
        (
          returning_column
          (
            id
            E
            (
              case_insensitive
            )
            (
              unqualified
            )
          )
        )
      )
    )
  )
)

As you can see, no reference to RETURNING ... D.

What's also interesting is that we also mess up collecting the remaining children. See Example #002:

PartiQL> UPDATE a INSERT INTO B VALUE C RETURNING MODIFIED NEW E INSERT INTO F VALUE G
   | !!

This results in the following exception:

org.partiql.lang.syntax.ParserException: Unprocessed components remaining
	Parser Error: at line <UNKNOWN>, column <UNKNOWN>: Internal error - malformed parse tree detected, <UNKNOWN> : <UNKNOWN>

	at org.partiql.lang.syntax.SqlParser$ParseNode.errMalformedParseTree(SqlParser.kt:256)
	at org.partiql.lang.syntax.SqlParser.malformedIfNotEmpty(SqlParser.kt:272)
	at org.partiql.lang.syntax.SqlParser.toDmlOperation(SqlParser.kt:1361)
	at org.partiql.lang.syntax.SqlParser.access$toDmlOperation(SqlParser.kt:79)
	at org.partiql.lang.syntax.SqlParser$toAstDml$1.invoke(SqlParser.kt:733)
        ....

Running this through the debugger, withing toDmlOperation, our unconsumed children includes the RETURNING node.

Recommendation

So, there are a couple things wrong. My recommendation (for the ANTLR project) would be to only allow RETURNING after the baseDml -- not within Dml (INSERT INTO .. VALUE) itself. This helps with ambiguity, and our AST only allows a single returning anyways (unless multiple column references).

This will likely be resolved with ANTLR's implementation, but it's something to keep in mind.