Being more straightforward on variables vs identifiers in the AST

CHANGELOG.md

@@ -77,6 +77,10 @@ Thank you to all who have contributed!
   `LetBinding`, `FromSource.Scan`, `FromSource.Unpivot`, `GraphLabelSpec.GraphLabelName`, 
   `GraphMatchPatternPart.Node`, `GraphMatchPatternPart.Edge`, `GraphMatchPattern`, `GroupBy`, `GroupKey`, 
   `DmlOp.Insert`, `TableDefPart.ColumnDeclaration`, `ColumnConstraint`, `Type.CustomType`. 
+- **Breaking** The AST node for the expression that refers to variables got renamed from `Expr.Id` to `Expr.Vr`
+  and its internal structure has changed: instead of fields `name` of type `SymbolPrimitive` and 
+  `case` of type `CaseSensitivity`, it now contains a single field `id` of type `Id`, which carries 
+  the same information. `Id` is the new name for the node previously named `Identifier`. 
 
 ### Deprecated
 

examples/src/main/kotlin/org/partiql/examples/PartialEvaluationVisitorTransform.kt

@@ -17,8 +17,8 @@ import java.io.PrintStream
  * simply `7`. (That is, `(+ 1 (* (lit 2) (lit 3)))` would be transformed to `(lit 7)`) (Note: s-expression AST is
  * shown without wrapping `term` or `meta`.)
  *
- * The query `foo + 2 * 3` would be transformed to `foo + 6` (`(+ (id foo case_insensitive) (* 2 3)))`
- * becomes `(+ (id foo case_insensitive) (lit 6))`
+ * The query `foo + 2 * 3` would be transformed to `foo + 6` (`(+ (vr foo case_insensitive) (* 2 3)))`
+ * becomes `(+ (vr foo case_insensitive) (lit 6))`
  *
  * This example just shows the partial evaluation for addition. Once these operations are better modeled in the
  * PIG domain (https://github.com/partiql/partiql-lang-kotlin/issues/241), this can be expanded to all NAry operations
@@ -36,7 +36,7 @@ import java.io.PrintStream
  * (+
  *     (lit 1)
  *     (lit 2)
- *     (id foo case_insensitive)
+ *     (vr foo case_insensitive)
  *     (lit 3)
  *     (lit 4))
  * ```
@@ -46,7 +46,7 @@ import java.io.PrintStream
  * ```
  * (+
  *   (lit 3)
- *   (id foo case_insensitive)
+ *   (vr foo case_insensitive)
  *   (lit 7))
  * ```
  *

examples/src/test/kotlin/org/partiql/examples/ParserExampleTest.kt

@@ -22,10 +22,13 @@ Serialized AST
             (
               project_expr
               (
-                id
-                exampleField
+                vr
                 (
-                  case_insensitive
+                  id
+                  exampleField
+                  (
+                    case_insensitive
+                  )
                 )
                 (
                   unqualified
@@ -40,10 +43,13 @@ Serialized AST
           (
             scan
             (
-              id
-              exampleTable
+              vr
               (
-                case_insensitive
+                id
+                exampleTable
+                (
+                  case_insensitive
+                )
               )
               (
                 unqualified
@@ -59,10 +65,13 @@ Serialized AST
           (
             gt
             (
-              id
-              anotherField
+              vr
               (
-                case_insensitive
+                id
+                anotherField
+                (
+                  case_insensitive
+                )
               )
               (
                 unqualified

examples/src/test/kotlin/org/partiql/examples/ParserJavaExampleTest.kt

@@ -22,10 +22,13 @@ Serialized AST
             (
               project_expr
               (
-                id
-                exampleField
+                vr
                 (
-                  case_insensitive
+                  id
+                  exampleField
+                  (
+                    case_insensitive
+                  )
                 )
                 (
                   unqualified
@@ -40,10 +43,13 @@ Serialized AST
           (
             scan
             (
-              id
-              exampleTable
+              vr
               (
-                case_insensitive
+                id
+                exampleTable
+                (
+                  case_insensitive
+                )
               )
               (
                 unqualified
@@ -59,10 +65,13 @@ Serialized AST
           (
             gt
             (
-              id
-              anotherField
+              vr
               (
-                case_insensitive
+                id
+                anotherField
+                (
+                  case_insensitive
+                )
               )
               (
                 unqualified

lib/sprout/src/main/kotlin/org/partiql/sprout/model/Model.kt

@@ -105,7 +105,7 @@ sealed class TypeRef(
         val type: ScalarType,
         nullable: Boolean = false,
     ) : TypeRef(
-        id = type.toString().lowercase(),
+        id = type.toString().lowercase(), // TODO Is this normalization of a regular identifier? If so, needs to become Ident.normalizeRegular() or equivalent.
         nullable = nullable,
     )
 

partiql-ast/src/main/kotlin/org/partiql/ast/helpers/ToLegacyAst.kt

@@ -211,15 +211,15 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
         val name = node.symbol
         val case = node.caseSensitivity.toLegacyCaseSensitivity()
         // !! NOT AN EXPRESSION!!
-        identifier(name, case, metas)
+        id(name, case, metas)
     }
 
     fun visitIdentifierSymbolAsExpr(node: Identifier.Symbol) = translate(node) { metas ->
         val name = node.symbol
         val case = node.caseSensitivity.toLegacyCaseSensitivity()
         val scope = unqualified()
         // !! ID EXPRESSION!!
-        id(name, case, scope, metas)
+        vr(id(name, case), scope, metas)
     }
 
     override fun visitIdentifierQualified(node: Identifier.Qualified, ctx: Ctx) = translate(node) { metas ->
@@ -285,7 +285,7 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
         val name = v.symbol
         val case = v.caseSensitivity.toLegacyCaseSensitivity()
         val qualifier = node.scope.toLegacyScope()
-        id(name, case, qualifier, metas)
+        vr(id(name, case), qualifier, metas)
     }
 
     override fun visitExprCall(node: Expr.Call, ctx: Ctx) = translate(node) { metas ->
@@ -401,7 +401,7 @@ private class AstTranslator(val metas: Map<String, MetaContainer>) : AstBaseVisi
         val index = visitExpr(node.key, ctx)
         // Legacy AST marks every index step as CaseSensitive
         val case = when (index) {
-            is PartiqlAst.Expr.Id -> index.case
+            is PartiqlAst.Expr.Vr -> index.id.case
             else -> PartiqlAst.CaseSensitivity.CaseSensitive()
         }
         pathExpr(index, case, metas)

partiql-ast/src/main/pig/partiql.ion

@@ -87,7 +87,7 @@ may then be further optimized by selecting better implementations of each operat
             (lit value::ion)
 
             // A variable reference
-            (id name::symbol case::case_sensitivity qualifier::scope_qualifier)
+            (vr id::id qualifier::scope_qualifier)
 
             // A parameter, i.e. `?`
             (parameter index::int)
@@ -464,21 +464,21 @@ may then be further optimized by selecting better implementations of each operat
 
         // A data definition operation.
         (sum ddl_op
-            // `CREATE TABLE <symbol> ( <table_def> )?`
-            (create_table table_name::identifier def::(? table_def))
+            // `CREATE TABLE <id> ( <table_def> )?`
+            (create_table table_name::id def::(? table_def))
 
-            // `DROP TABLE <identifier>`
-            (drop_table table_name::identifier)
+            // `DROP TABLE <id>`
+            (drop_table table_name::id)
 
-            // `CREATE INDEX ON <identifier> (<expr> [, <expr>]...)`
+            // `CREATE INDEX ON <id> (<expr> [, <expr>]...)`
             // TODO: add optional table name
-            (create_index index_name::identifier fields::(* expr 1))
+            (create_index index_name::id fields::(* expr 1))
 
-            // DROP INDEX <identifier> ON <identifier>
-            // In Statement, first <identifier> represents keys, second represents table
+            // DROP INDEX <id> ON <id>
+            // In Statement, first <id> represents keys, second represents table
             (drop_index
-                (table identifier)
-                (keys identifier)))
+                (table id)
+                (keys id)))
 
         (product table_def parts::(* table_def_part 1))
 
@@ -513,23 +513,14 @@ may then be further optimized by selecting better implementations of each operat
             (all_old)
         )
 
-        // `identifier` is a "reference identifier" for referring to / looking up an entity
+        // `id` is a "reference identifier" for referring to / looking up an entity
         // that is elsewhere bound to a 'defnid', a "definitional identifier".
-        // `identifier` can be used for names that need to be looked up with a notion of case-sensitivity.
-
-        // For both `create_index` and `create_table`, there is no notion of case-sensitivity
-        // for table identifiers since they are *defining* new identifiers.  However, for `drop_index` and
-        // `drop_table` *do* have the notion of case sensitivity since they are referring to existing names.
-        // Identifiers with case-sensitivity is already modeled with the `id` variant of `expr`,
-        // but there is no way to specify to PIG that we want to only allow a single variant of a sum as
-        // an element of a type.  (Even though in the Kotlin code each varaint is its own type.)  Hence, we
-        // define an `identifier` type above which can be used without opening up an element's domain to all of
-        // `expr`.
-        (product identifier name::symbol case::case_sensitivity)
+        // `id` can be used for names that need to be looked up with a notion of case-sensitivity.
+        (product id symb::symbol case::case_sensitivity)
 
         // A "definitional identifier" is for use in places that introduce, or bind, a name/identifier.
         // This does not keep track of "case sensitivity", in the current design of identifiers.
-        // Upon transition to SQL-conforming identifiers, `defnid` will merge with `identifier`.
+        // Upon transition to SQL-conforming identifiers, `defnid` will merge with `id`.
         (product defnid symb::symbol)
 
         // Represents `<expr> = <expr>` in a DML SET operation.  Note that in this case, `=` is representing
@@ -673,13 +664,13 @@ may then be further optimized by selecting better implementations of each operat
                 // Example as a struct-union. Given a global environment with `foo` bound to `{ a: 42 }` and `bar`
                 // bound to `{ b: 43}`, then:
                 //     (struct
-                //         (struct_fields (id foo))
-                //         (struct_fields (id bar)))
+                //         (struct_fields (vr foo))
+                //         (struct_fields (vr bar)))
                 // Returns { a: 42, b: 43 }
                 // Note that `struct_field` and `struct_fields` may be used in combination:
                 //     (struct
-                //         (struct_fields (id foo))
-                //         (struct_fields (id bar))
+                //         (struct_fields (vr foo))
+                //         (struct_fields (vr bar))
                 //         (struct_field (lit c) (lit 44)))
                 // Returns { a: 42, b: 43, c: 44 }
                 //
@@ -840,7 +831,7 @@ may then be further optimized by selecting better implementations of each operat
                     // consisting of only literal path steps (and with no wildcard or unpivot operators).
                     // Note: partiql_ast uses the `expr` sum type for this, which is too broad.  We're not
                     // changing that at this time because `partiql_ast` is established public API.
-                    target::identifier
+                    target::id
                     operation::dml_operation
                     rows::expr
                 )
@@ -904,7 +895,7 @@ may then be further optimized by selecting better implementations of each operat
         (with expr
             // At this point there should be no undefined variables in the plan.  All variables are rewritten to
             // `local_id`, `global_id`.
-            (exclude id)
+            (exclude vr)
             (include
                 // A resolved reference to a variable that was defined within a query.  Otherwise known as a local
                 // variable. "Resolved" means that the variable is guaranteed to exist and we know its register index.

partiql-ast/src/test/kotlin/org/partiql/ast/helpers/ToLegacyAstTest.kt

@@ -207,31 +207,31 @@ class ToLegacyAstTest {
 
         @JvmStatic
         fun identifiers() = listOf(
-            expect("(id 'a' (case_sensitive) (unqualified))") {
+            expect("(vr (id 'a' (case_sensitive)) (unqualified))") {
                 exprVar {
                     identifier = identifierSymbol("a", Identifier.CaseSensitivity.SENSITIVE)
                     scope = Expr.Var.Scope.DEFAULT
                 }
             },
-            expect("(id 'a' (case_insensitive) (unqualified))") {
+            expect("(vr (id 'a' (case_insensitive)) (unqualified))") {
                 exprVar {
                     identifier = identifierSymbol("a", Identifier.CaseSensitivity.INSENSITIVE)
                     scope = Expr.Var.Scope.DEFAULT
                 }
             },
-            expect("(id 'a' (case_sensitive) (locals_first))") {
+            expect("(vr (id 'a' (case_sensitive)) (locals_first))") {
                 exprVar {
                     identifier = identifierSymbol("a", Identifier.CaseSensitivity.SENSITIVE)
                     scope = Expr.Var.Scope.LOCAL
                 }
             },
-            expect("(id 'a' (case_insensitive) (locals_first))") {
+            expect("(vr (id 'a' (case_insensitive)) (locals_first))") {
                 exprVar {
                     identifier = identifierSymbol("a", Identifier.CaseSensitivity.INSENSITIVE)
                     scope = Expr.Var.Scope.LOCAL
                 }
             },
-            expect("(identifier 'a' (case_insensitive))") {
+            expect("(id 'a' (case_insensitive))") {
                 identifierSymbol("a", Identifier.CaseSensitivity.INSENSITIVE)
             },
             //
@@ -520,7 +520,7 @@ class ToLegacyAstTest {
             expect(
                 """
                 (project_list
-                    (project_all (id 'a' (case_sensitive) (unqualified)))
+                    (project_all (vr (id 'a' (case_sensitive)) (unqualified)))
                     (project_expr (lit 1) (defnid 'x'))
                 )
              """

partiql-lang/src/main/kotlin/org/partiql/lang/CompilerPipeline.kt

@@ -237,7 +237,7 @@ internal class CompilerPipelineImpl(
         functions,
         customDataTypes.map { customType ->
             (customType.aliases + customType.name).map { alias ->
-                Pair(alias.lowercase(), customType.typedOpParameter)
+                Pair(Ident.normalizeRegular(alias), customType.typedOpParameter)
             }
         }.flatten().toMap(),
         procedures,
@@ -265,7 +265,7 @@ internal class CompilerPipelineImpl(
                                 customFunctionSignatures = functions.values.map { it.signature },
                                 customTypedOpParameters = customDataTypes.map { customType ->
                                     (customType.aliases + customType.name).map { alias ->
-                                        Pair(alias.lowercase(), customType.typedOpParameter)
+                                        Pair(Ident.normalizeRegular(alias), customType.typedOpParameter)
                                     }
                                 }.flatten().toMap()
                             )

partiql-lang/src/main/kotlin/org/partiql/lang/Ident.kt

@@ -38,7 +38,8 @@ class Ident private constructor (
         // TODO This is intended to be the one place to encapsulate the design choice of whether
         // regular identifiers normalize to lower or upper case. Transitioning to this is just in its beginning --
         // there are still many sites sprinkled around where this is done by ad hoc string lower-casing.
-        private fun normalizeRegular(str: String): String =
+        // TODO Make this private when its uses outside of this class are phased out.
+        fun normalizeRegular(str: String): String =
             str.lowercase()
 
         /** Create a semantic identifier corresponding to a lexical SQL *delimited* identifier. */

partiql-lang/src/main/kotlin/org/partiql/lang/ast/passes/StatementRedactor.kt

@@ -35,7 +35,7 @@ fun skipRedaction(node: PartiqlAst.Expr, safeFieldNames: Set<String>): Boolean {
     }
 
     return when (node) {
-        is PartiqlAst.Expr.Id -> safeFieldNames.contains(node.name.text)
+        is PartiqlAst.Expr.Vr -> safeFieldNames.contains(node.id.symb.text)
         is PartiqlAst.Expr.Lit -> {
             when (node.value) {
                 is StringElement -> safeFieldNames.contains(node.value.stringValue)
@@ -282,7 +282,7 @@ private class StatementRedactionVisitor(
     }
 
     private fun redactTypes(typed: PartiqlAst.Expr.IsType) {
-        if (typed.value is PartiqlAst.Expr.Id && !skipRedaction(typed.value, safeFieldNames)) {
+        if (typed.value is PartiqlAst.Expr.Vr && !skipRedaction(typed.value, safeFieldNames)) {
             val sourceLocation = typed.type.metas.sourceLocation ?: error("Cannot redact due to missing source location")
             sourceLocationMetaForRedaction.add(sourceLocation)
         }