Updates UndefinedVariable class, deprecates methods, and updates tests

johnedquinn · johnedquinn · commit 234518c25177 · 2024-04-01T14:22:57.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,6 +38,8 @@ Thank you to all who have contributed!
 
 ### Deprecated
 - The current SqlBlock, SqlDialect, and SqlLayout are marked as deprecated and will be slightly changed in the next release. 
+- Deprecates constructor and properties `variableName` and `caseSensitive` of `org.partiql.planner.PlanningProblemDetails.UndefinedVariable`
+  in favor of newly added constructor and properties `name` and `inScopeVariables`.
 
 ### Fixed
 - `StaticType.flatten()` on an `AnyOfType` with `AnyType` will return `AnyType`
diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/Errors.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/Errors.kt
@@ -2,6 +2,7 @@ package org.partiql.planner
 
 import org.partiql.errors.ProblemDetails
 import org.partiql.errors.ProblemSeverity
+import org.partiql.plan.Identifier
 import org.partiql.types.StaticType
 
 /**
@@ -24,15 +25,69 @@ public sealed class PlanningProblemDetails(
     public data class CompileError(val errorMessage: String) :
         PlanningProblemDetails(ProblemSeverity.ERROR, { errorMessage })
 
-    public data class UndefinedVariable(val variableName: String, val caseSensitive: Boolean) :
-        PlanningProblemDetails(
-            ProblemSeverity.ERROR,
-            {
-                "Undefined variable '$variableName'." +
-                    quotationHint(caseSensitive)
+    public data class UndefinedVariable(
+        val name: Identifier,
+        val inScopeVariables: Set<String>
+    ) : PlanningProblemDetails(
+        ProblemSeverity.ERROR,
+        {
+            "Variable ${pretty(name)} does not exist in the database environment and is not an attribute of the following in-scope variables $inScopeVariables." +
+                quotationHint(isSymbolAndCaseSensitive(name))
+        }
+    ) {
+
+        @Deprecated("This will be removed in a future major version release.", replaceWith = ReplaceWith("name"))
+        val variableName: String = when (name) {
+            is Identifier.Symbol -> name.symbol
+            is Identifier.Qualified -> when (name.steps.size) {
+                0 -> name.root.symbol
+                else -> name.steps.last().symbol
+            }
+        }
+
+        @Deprecated("This will be removed in a future major version release.", replaceWith = ReplaceWith("name"))
+        val caseSensitive: Boolean = when (name) {
+            is Identifier.Symbol -> name.caseSensitivity == Identifier.CaseSensitivity.SENSITIVE
+            is Identifier.Qualified -> when (name.steps.size) {
+                0 -> name.root.caseSensitivity == Identifier.CaseSensitivity.SENSITIVE
+                else -> name.steps.last().caseSensitivity == Identifier.CaseSensitivity.SENSITIVE
             }
+        }
+
+        @Deprecated("This will be removed in a future major version release.", replaceWith = ReplaceWith("UndefinedVariable(Identifier, Set<String>)"))
+        public constructor(variableName: String, caseSensitive: Boolean) : this(
+            Identifier.Symbol(
+                variableName,
+                when (caseSensitive) {
+                    true -> Identifier.CaseSensitivity.SENSITIVE
+                    false -> Identifier.CaseSensitivity.INSENSITIVE
+                }
+            ),
+            emptySet()
         )
 
+        private companion object {
+            /**
+             * Used to check whether the [id] is an [Identifier.Symbol] and whether it is case-sensitive. This is helpful
+             * for giving the [quotationHint] to the user.
+             */
+            private fun isSymbolAndCaseSensitive(id: Identifier): Boolean = when (id) {
+                is Identifier.Symbol -> id.caseSensitivity == Identifier.CaseSensitivity.SENSITIVE
+                is Identifier.Qualified -> false
+            }
+
+            private fun pretty(id: Identifier): String = when (id) {
+                is Identifier.Symbol -> pretty(id)
+                is Identifier.Qualified -> (listOf(id.root) + id.steps).joinToString(".") { pretty(it) }
+            }
+
+            private fun pretty(id: Identifier.Symbol): String = when (id.caseSensitivity) {
+                Identifier.CaseSensitivity.INSENSITIVE -> id.symbol
+                Identifier.CaseSensitivity.SENSITIVE -> "\"${id.symbol}\""
+            }
+        }
+    }
+
     public data class UndefinedDmlTarget(val variableName: String, val caseSensitive: Boolean) :
         PlanningProblemDetails(
             ProblemSeverity.ERROR,
diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/PlanTyper.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/PlanTyper.kt
@@ -451,8 +451,8 @@ internal class PlanTyper(
             }
             val resolvedVar = env.resolve(path, locals, strategy)
             if (resolvedVar == null) {
-                handleUndefinedVariable(path.steps.last())
-                return rex(ANY, rexOpErr("Undefined variable ${node.identifier}"))
+                val details = handleUndefinedVariable(node.identifier, locals.schema.map { it.name }.toSet())
+                return rex(ANY, rexOpErr(details.message))
             }
             return visitRex(resolvedVar, null)
         }
@@ -1399,15 +1399,42 @@ internal class PlanTyper(
 
     // ERRORS
 
-    private fun handleUndefinedVariable(name: BindingName) {
+    /**
+     * Invokes [onProblem] with a newly created [PlanningProblemDetails.UndefinedVariable] and returns the
+     * [PlanningProblemDetails.UndefinedVariable].
+     */
+    private fun handleUndefinedVariable(name: Identifier, locals: Set<String>): PlanningProblemDetails.UndefinedVariable {
+        val planName = name.toPlan()
+        val details = PlanningProblemDetails.UndefinedVariable(planName, locals)
         onProblem(
             Problem(
                 sourceLocation = UNKNOWN_PROBLEM_LOCATION,
-                details = PlanningProblemDetails.UndefinedVariable(name.name, name.bindingCase == BindingCase.SENSITIVE)
+                details = details
             )
         )
+        return details
+    }
+
+    private fun Identifier.CaseSensitivity.toPlan(): org.partiql.plan.Identifier.CaseSensitivity = when (this) {
+        Identifier.CaseSensitivity.SENSITIVE -> org.partiql.plan.Identifier.CaseSensitivity.SENSITIVE
+        Identifier.CaseSensitivity.INSENSITIVE -> org.partiql.plan.Identifier.CaseSensitivity.INSENSITIVE
+    }
+
+    private fun Identifier.toPlan(): org.partiql.plan.Identifier = when (this) {
+        is Identifier.Symbol -> this.toPlan()
+        is Identifier.Qualified -> this.toPlan()
     }
 
+    private fun Identifier.Symbol.toPlan(): org.partiql.plan.Identifier.Symbol = org.partiql.plan.Identifier.Symbol(
+        this.symbol,
+        this.caseSensitivity.toPlan()
+    )
+
+    private fun Identifier.Qualified.toPlan(): org.partiql.plan.Identifier.Qualified = org.partiql.plan.Identifier.Qualified(
+        this.root.toPlan(),
+        this.steps.map { it.toPlan() }
+    )
+
     private fun handleUnexpectedType(actual: StaticType, expected: Set<StaticType>) {
         onProblem(
             Problem(
diff --git a/partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PlanTyperTestsPorted.kt b/partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PlanTyperTestsPorted.kt
@@ -15,6 +15,7 @@ import org.junit.jupiter.params.provider.MethodSource
 import org.partiql.errors.Problem
 import org.partiql.errors.UNKNOWN_PROBLEM_LOCATION
 import org.partiql.parser.PartiQLParser
+import org.partiql.plan.Identifier
 import org.partiql.plan.PartiQLPlan
 import org.partiql.plan.Statement
 import org.partiql.plan.debug.PlanPrinter
@@ -104,6 +105,18 @@ class PlanTyperTestsPorted {
             }
         }
 
+        private fun id(vararg parts: Identifier.Symbol): Identifier {
+            return when (parts.size) {
+                0 -> error("Identifier requires more than one part.")
+                1 -> parts.first()
+                else -> Identifier.Qualified(parts.first(), parts.drop(1))
+            }
+        }
+
+        private fun sensitive(part: String): Identifier.Symbol = Identifier.Symbol(part, Identifier.CaseSensitivity.SENSITIVE)
+
+        private fun insensitive(part: String): Identifier.Symbol = Identifier.Symbol(part, Identifier.CaseSensitivity.INSENSITIVE)
+
         /**
          * MemoryConnector.Factory from reading the resources in /resource_path.txt for Github CI/CD.
          */
@@ -131,7 +144,6 @@ class PlanTyperTestsPorted {
                 }
             }
             map.entries.map {
-                println("Map Entry: ${it.key} to ${it.value}")
                 it.key to MemoryConnector.Metadata.of(*it.value.toTypedArray())
             }
         }
@@ -805,7 +817,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("a", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("a"), setOf("t1", "t2"))
                     )
                 }
             ),
@@ -2022,7 +2034,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("unknown_col", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("unknown_col"), setOf("pets"))
                     )
                 }
             ),
@@ -2920,7 +2932,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("main", true)
+                        PlanningProblemDetails.UndefinedVariable(id(sensitive("pql"), sensitive("main")), setOf())
                     )
                 }
             ),
@@ -2933,7 +2945,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pql", true)
+                        PlanningProblemDetails.UndefinedVariable(sensitive("pql"), setOf())
                     )
                 }
             ),
@@ -3177,27 +3189,28 @@ class PlanTyperTestsPorted {
             SuccessTestCase(
                 name = "AGGREGATE over nullable integers",
                 query = """
-                    SELECT
-                        COUNT(a) AS count_a
-                    FROM <<
-                        { 'a': 1, 'b': 2 }
-                    >> t1 INNER JOIN <<
-                        { 'c': 1, 'd': 3 }
-                    >> t2
-                        ON t1.a = t1.c
-                        AND (
-                            1 = (
-                            SELECT COUNT(e) AS count_e
-                            FROM <<
-                                { 'e': 10 }
-                            >> t3
+                    SELECT T1.a
+                    FROM T1
+                        LEFT JOIN T2 AS T2_1
+                            ON T2_1.d =
+                            (
+                                SELECT
+                                    CASE WHEN COUNT(f) = 1 THEN MAX(f) ELSE 0 END AS e
+                                FROM T3 AS T3_mapping
                             )
-                        );
+                        LEFT JOIN T2 AS T2_2
+                            ON T2_2.d =
+                            (
+                                SELECT
+                                    CASE WHEN COUNT(f) = 1 THEN MAX(f) ELSE 0 END AS e
+                                FROM T3 AS T3_mapping
+                            )
+                    ;
                 """.trimIndent(),
                 expected = BagType(
                     StructType(
                         fields = mapOf(
-                            "count_a" to StaticType.INT8
+                            "a" to StaticType.BOOL
                         ),
                         contentClosed = true,
                         constraints = setOf(
@@ -3206,8 +3219,9 @@ class PlanTyperTestsPorted {
                             TupleConstraint.Ordered
                         )
                     )
-                )
-            ),
+                ),
+                catalog = "aggregations"
+            )
         )
 
         @JvmStatic
@@ -3459,47 +3473,6 @@ class PlanTyperTestsPorted {
     // Parameterized Tests
     //
 
-    @Test
-    fun failingAggTest() {
-        val tc = SuccessTestCase(
-            name = "AGGREGATE over nullable integers",
-            query = """
-                SELECT T1.a
-                FROM T1
-                    LEFT JOIN T2 AS T2_1
-                        ON T2_1.d =
-                            (
-                                SELECT
-                                    CASE WHEN COUNT(f) = 1 THEN MAX(f) ELSE 0 END AS e
-                                FROM T3 AS T3_mapping
-                            )
-                    LEFT JOIN T2 AS T2_2
-                        ON T2_2.d =
-                            (
-                                SELECT
-                                    CASE WHEN COUNT(f) = 1 THEN MAX(f) ELSE 0 END AS e
-                                FROM T3 AS T3_mapping
-                            )
-                        ;
-            """.trimIndent(),
-            expected = BagType(
-                StructType(
-                    fields = mapOf(
-                        "a" to StaticType.BOOL
-                    ),
-                    contentClosed = true,
-                    constraints = setOf(
-                        TupleConstraint.Open(false),
-                        TupleConstraint.UniqueAttrs(true),
-                        TupleConstraint.Ordered
-                    )
-                )
-            ),
-            catalog = "aggregations"
-        )
-        runTest(tc)
-    }
-
     @Test
     @Disabled("The planner doesn't support heterogeneous input to aggregation functions (yet?).")
     fun failingTest() {
@@ -3824,7 +3797,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pets", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("pets"), emptySet())
                     )
                 }
             ),
@@ -3857,7 +3830,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pets", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("pets"), emptySet())
                     )
                 }
             ),
@@ -3924,7 +3897,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pets", false)
+                        PlanningProblemDetails.UndefinedVariable(id(insensitive("ddb"), insensitive("pets")), emptySet())
                     )
                 }
             ),
@@ -4194,7 +4167,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("non_existing_column", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("non_existing_column"), emptySet())
                     )
                 }
             ),
@@ -4249,7 +4222,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("unknown_col", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("unknown_col"), setOf("orders"))
                     )
                 }
             ),
