Updates UndefinedVariable class, deprecates methods, and updates tests

johnedquinn · johnedquinn · commit 91443cd60433 · 2024-03-21T12:04:46.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,10 +31,11 @@ Thank you to all who have contributed!
 
 ### Changed
 - Change `StaticType.AnyOfType`'s `.toString` to not perform `.flatten()`
-- Function resolution logic: Now the function resolver would match all possible candidate (based on if the argument can be coerced to the Signature parameter type). If there are multiple match it will first attempt to pick the one requires the least cast, then pick the function with the highest precedence.
 - **Behavioral change**: The COUNT aggregate function now returns INT64.
 
 ### Deprecated
+- 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
 - Fixes aggregations of attribute references to values of union types. This fix also allows for proper error handling by passing the UnknownAggregateFunction problem to the ProblemCallback. Please note that, with this change, the planner will no longer immediately throw an IllegalStateException for this exact scenario.
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
@@ -449,8 +449,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)
         }
@@ -1349,15 +1349,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"))
                     )
                 }
             ),
@@ -2707,7 +2719,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("main", true)
+                        PlanningProblemDetails.UndefinedVariable(id(sensitive("pql"), sensitive("main")), setOf())
                     )
                 }
             ),
@@ -2720,7 +2732,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pql", true)
+                        PlanningProblemDetails.UndefinedVariable(sensitive("pql"), setOf())
                     )
                 }
             ),
@@ -2964,27 +2976,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(
@@ -2993,8 +3006,9 @@ class PlanTyperTestsPorted {
                             TupleConstraint.Ordered
                         )
                     )
-                )
-            ),
+                ),
+                catalog = "aggregations"
+            )
         )
 
         @JvmStatic
@@ -3246,47 +3260,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() {
@@ -3601,7 +3574,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pets", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("pets"), emptySet())
                     )
                 }
             ),
@@ -3634,7 +3607,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pets", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("pets"), emptySet())
                     )
                 }
             ),
@@ -3701,7 +3674,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("pets", false)
+                        PlanningProblemDetails.UndefinedVariable(id(insensitive("ddb"), insensitive("pets")), emptySet())
                     )
                 }
             ),
@@ -3971,7 +3944,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("non_existing_column", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("non_existing_column"), emptySet())
                     )
                 }
             ),
@@ -4026,7 +3999,7 @@ class PlanTyperTestsPorted {
                 problemHandler = assertProblemExists {
                     Problem(
                         UNKNOWN_PROBLEM_LOCATION,
-                        PlanningProblemDetails.UndefinedVariable("unknown_col", false)
+                        PlanningProblemDetails.UndefinedVariable(insensitive("unknown_col"), setOf("orders"))
                     )
                 }
             ),
