Updates UndefinedVariable class, deprecates methods, and updates tests

partiql · Mar 21, 2024 · 6cac376 · 6cac376
1 parent 7fc183e
commit 6cac376
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 80 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,7 +31,6 @@ 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

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"))
                     )
                 }
             ),