Repository: partiql/partiql-lang-kotlin (kotlin)

Issue:
[v1] bag constructor allows for whitespace between angle brackets
Something I noticed when trying to add some special operator parsing rules in the v1 branch is that it currently allows whitespace between the angle brackets:

```
Welcome to the PartiQL shell!
Typing mode: LEGACY
Using version: 1.0.0-SNAPSHOT-4dd09721
PartiQL> <<1>>;
===' 
<<
  1
>>
--- 
OK!
PartiQL> < < 1 > >;
===' 
<<
  1
>>
--- 
OK!
```

whereas the previous parsing behavior on `main` and all prior releases require the angle brackets to be together:
```
Welcome to the PartiQL shell!
Typing mode: LEGACY
Using version: 0.14.5-ab65143c
PartiQL> <<1>>;
==='
<<
  1
>>
---
OK!
PartiQL> < < 1 > >;

mismatched input '<' expecting {'ANY', 'AVG', 'BIT_LENGTH', 'CASE', 'CAST', 'CHARACTER_LENGTH', 'CHAR_LENGTH', 'COALESCE', 'COUNT', 'CREATE', 'CURRENT_DATE', 'CURRENT_USER', 'DATE', 'DELETE', 'DROP', 'EVERY', 'EXCLUDED', 'EXEC', 'EXISTS', 'EXPLAIN', 'EXTRACT', 'DATE_ADD', 'DATE_DIFF', 'FALSE', 'FROM', 'INSERT', 'LOWER', 'MAX', 'MIN', 'NOT', 'NULL', 'NULLIF', 'OCTET_LENGTH', 'OVERLAY', 'POSITION', 'REPLACE', 'SELECT', 'SET', 'SIZE', 'SOME', 'SUBSTRING', 'SUM', 'TIME', 'TIMESTAMP', 'TRIM', 'TRUE', 'UPDATE', 'UPPER', 'UPSERT', 'VALUES', 'LAG', 'LEAD', 'CAN_CAST', 'CAN_LOSSLESS_CAST', 'MISSING', 'PIVOT', 'REMOVE', 'LIST', 'SEXP', '+', '-', '@', '<<', '[', '{', '(', '?', LITERAL_STRING, LITERAL_INTEGER, LITERAL_DECIMAL, IDENTIFIER, IDENTIFIER_QUOTED, ION_CLOSURE}
```

Seems like https://github.com/partiql/partiql-lang-kotlin/pull/1449 inadvertently introduced this parsing change. Will need to confirm w/ team whether this behavior should exist in the parsing.

Pull request description:
## Relevant Issues
- Fixes #1498 

## Description
- Previous behavior on `v1` allowed for inserting whitespace and comments between the angle brackets of a bag constructor. This PR corrects the behavior and gives an error for these scenarios.
- This PR follows what the Definitive ANTLR 4 Reference describes in Chapter 12 regarding double right angle brackets:
>  To address this, either we can add semantic predicates to the grammar or
we can check the parse tree afterward using a listener or visitor to ensure that
the > token column numbers are adjacent for shift operators. It’d be inefficient
to use predicates during the parse, so it’s better to check the right shift operators
after the parse. Most language applications need to walk the parse tree anyway.

I looked at some other alternatives such as
- pushing the typing to a separate `mode` in the lexer (similar to what Ion lexer rules we currently have)
  - this had some drawbacks in some duplicated lexer rules
  - we could always revisit this approach in the future
- semantic predicates
  - potential slowdown in the lexer step

but decided to keep the recommendation of the ANTLR reference book.

## Other Information
- Updated Unreleased Section in CHANGELOG: **[NO]**
  - No, on `v1` branch

- Any backward-incompatible changes? **[NO]**

- Any new external dependencies? **[NO]**

- Do your changes comply with the [Contributing Guidelines](https://github.com/partiql/partiql-lang-kotlin/blob/main/CONTRIBUTING.md)
  and [Code Style Guidelines](https://github.com/partiql/partiql-lang-kotlin/blob/main/CODE_STYLE.md)? **[YES]**

## License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Patch:
diff --git a/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigParser.kt b/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigParser.kt
index 94fa824e5c..10b6a017b4 100644
--- a/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigParser.kt
+++ b/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigParser.kt
@@ -95,7 +95,7 @@ internal class PartiQLPigParser(val customTypes: List<CustomType> = listOf()) :
         val tokenStream = createTokenStream(queryStream)
         val parser = parserInit(tokenStream)
         val tree = parser.root()
-        val visitor = PartiQLPigVisitor(customTypes, tokenStream.parameterIndexes)
+        val visitor = PartiQLPigVisitor(tokenStream, customTypes, tokenStream.parameterIndexes)
         return visitor.visit(tree) as PartiqlAst.Statement
     }
 
diff --git a/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt b/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt
index 69971bed5c..b56599ab61 100644
--- a/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt
+++ b/partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt
@@ -33,6 +33,7 @@ import com.amazon.ionelement.api.ionNull
 import com.amazon.ionelement.api.ionString
 import com.amazon.ionelement.api.ionSymbol
 import com.amazon.ionelement.api.loadSingleElement
+import org.antlr.v4.runtime.CommonTokenStream
 import org.antlr.v4.runtime.ParserRuleContext
 import org.antlr.v4.runtime.Token
 import org.antlr.v4.runtime.tree.TerminalNode
@@ -62,6 +63,7 @@ import org.partiql.lang.util.getPrecisionFromTimeString
 import org.partiql.lang.util.unaryMinus
 import org.partiql.parser.internal.antlr.PartiQLParser
 import org.partiql.parser.internal.antlr.PartiQLParserBaseVisitor
+import org.partiql.parser.internal.antlr.PartiQLTokens
 import org.partiql.pig.runtime.SymbolPrimitive
 import org.partiql.value.datetime.DateTimeException
 import org.partiql.value.datetime.TimeZone
@@ -116,6 +118,7 @@ import java.time.format.DateTimeParseException
  * There could be clever ways of exploiting this, to avoid the dispatch via `visit()`.
  */
 internal class PartiQLPigVisitor(
+    private val tokens: CommonTokenStream,
     val customTypes: List<CustomType> = listOf(),
     private val parameterIndexes: Map<Int, Int> = mapOf(),
 ) :
@@ -1507,6 +1510,12 @@ internal class PartiQLPigVisitor(
      */
 
     override fun visitBag(ctx: PartiQLParser.BagContext) = PartiqlAst.build {
+        // Prohibit hidden characters between angle brackets
+        val startTokenIndex = ctx.start.tokenIndex
+        val endTokenIndex = ctx.stop.tokenIndex
+        if (tokens.getHiddenTokensToRight(startTokenIndex, PartiQLTokens.HIDDEN) != null || tokens.getHiddenTokensToLeft(endTokenIndex, PartiQLTokens.HIDDEN) != null) {
+            throw ParserException("Invalid bag expression", ErrorCode.PARSE_INVALID_QUERY)
+        }
         val exprList = ctx.expr().map { visitExpr(it) }
         bag(exprList, ctx.ANGLE_LEFT(0).getSourceMetaContainer())
     }
diff --git a/partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt b/partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt
index 83b362e6bd..19bdef2d9e 100644
--- a/partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt
+++ b/partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt
@@ -425,6 +425,7 @@ internal class PartiQLParserDefault : PartiQLParser {
      */
     @OptIn(PartiQLValueExperimental::class)
     private class Visitor(
+        private val tokens: CommonTokenStream,
         private val locations: SourceLocations.Mutable,
         private val parameters: Map<Int, Int> = mapOf(),
     ) : PartiQLParserBaseVisitor<AstNode>() {
@@ -442,7 +443,7 @@ internal class PartiQLParserDefault : PartiQLParser {
                 tree: GeneratedParser.RootContext,
             ): PartiQLParser.Result {
                 val locations = SourceLocations.Mutable()
-                val visitor = Visitor(locations, tokens.parameterIndexes)
+                val visitor = Visitor(tokens, locations, tokens.parameterIndexes)
                 val root = visitor.visitAs<AstNode>(tree) as Statement
                 return PartiQLParser.Result(
                     source = source,
@@ -2022,6 +2023,12 @@ internal class PartiQLParserDefault : PartiQLParser {
          */
 
         override fun visitBag(ctx: GeneratedParser.BagContext) = translate(ctx) {
+            // Prohibit hidden characters between angle brackets
+            val startTokenIndex = ctx.start.tokenIndex
+            val endTokenIndex = ctx.stop.tokenIndex
+            if (tokens.getHiddenTokensToRight(startTokenIndex, GeneratedLexer.HIDDEN) != null || tokens.getHiddenTokensToLeft(endTokenIndex, GeneratedLexer.HIDDEN) != null) {
+                throw error(ctx, "Invalid bag expression")
+            }
             val expressions = visitOrEmpty<Expr>(ctx.expr())
             exprCollection(Expr.Collection.Type.BAG, expressions)
         }


Test patch:
diff --git a/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTest.kt b/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTest.kt
index 9a8c14819e..98fc988f04 100644
--- a/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTest.kt
+++ b/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTest.kt
@@ -5019,4 +5019,38 @@ class PartiQLParserTest : PartiQLParserTestBase() {
             lit(ionInt(1))
         )
     }
+
+    // regression tests for bag constructor angle bracket
+    @Test
+    fun testBagConstructor() = assertExpression("<<<<1>>>>") {
+        bag(
+            bag(
+                lit(ionInt(1))
+            )
+        )
+    }
+
+    @Test
+    fun testSpacesInBagConstructor() = checkInputThrowingParserException(
+        "< < < < 1 > > > >",
+        ErrorCode.PARSE_UNEXPECTED_TOKEN, // partiql-ast parser ErrorCode
+        expectErrorContextValues = mapOf(
+            Property.LINE_NUMBER to 1L,
+            Property.COLUMN_NUMBER to 1L,
+            Property.TOKEN_DESCRIPTION to PartiQLParser.ANGLE_LEFT.getAntlrDisplayString(),
+            Property.TOKEN_VALUE to ION.newSymbol("<")
+        )
+    )
+
+    @Test
+    fun testCommentsInBagConstructor() = checkInputThrowingParserException(
+        "</* some comment */<<<1>>>>",
+        ErrorCode.PARSE_UNEXPECTED_TOKEN, // partiql-ast parser ErrorCode
+        expectErrorContextValues = mapOf(
+            Property.LINE_NUMBER to 1L,
+            Property.COLUMN_NUMBER to 1L,
+            Property.TOKEN_DESCRIPTION to PartiQLParser.ANGLE_LEFT.getAntlrDisplayString(),
+            Property.TOKEN_VALUE to ION.newSymbol("<")
+        )
+    )
 }
diff --git a/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTestBase.kt b/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTestBase.kt
index 802974a413..1f4896bdd8 100644
--- a/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTestBase.kt
+++ b/partiql-lang/src/test/kotlin/org/partiql/lang/syntax/PartiQLParserTestBase.kt
@@ -152,7 +152,6 @@ abstract class PartiQLParserTestBase : TestBase() {
         input: String,
         errorCode: ErrorCode,
         expectErrorContextValues: Map<Property, Any>,
-        targets: Array<ParserTarget> = arrayOf(ParserTarget.DEFAULT),
         assertContext: Boolean = true,
     ): Unit = forEachTarget {
         softAssert {
@@ -160,7 +159,8 @@ abstract class PartiQLParserTestBase : TestBase() {
                 parser.parseAstStatement(input)
                 fail("Expected ParserException but there was no Exception")
             } catch (ex: ParserException) {
-                // split parser target does not use ErrorCode
+                // NOTE: only perform error code and error context checks for `ParserTarget.EXPERIMENTAL` (partiql-ast
+                // parser).
                 if (assertContext && (this@forEachTarget == ParserTarget.EXPERIMENTAL)) {
                     checkErrorAndErrorContext(errorCode, ex, expectErrorContextValues)
                 }