Addresses PR comments

johnedquinn · Jun 11, 2024 · 145991e · 145991e
1 parent cc3654a
commit 145991e
Show file tree

Hide file tree

Showing 2 changed files with 6 additions and 7 deletions.
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
@@ -87,7 +87,6 @@ import org.partiql.value.MissingValue
 import org.partiql.value.PartiQLValueExperimental
 import org.partiql.value.TextValue
 import org.partiql.value.boolValue
-import org.partiql.value.missingValue
 import org.partiql.value.stringValue
 import kotlin.math.max
 
@@ -597,7 +596,7 @@ internal class PlanTyper(private val env: Env) {
             return rex(unionOf(elementTypes), rexOpPathIndex(root, key))
         }
 
-        private fun Rex.isLiteralMissing(): Boolean = this.op is Rex.Op.Lit && this.op.value.withoutAnnotations() == missingValue()
+        private fun Rex.isLiteralMissing(): Boolean = this.op is Rex.Op.Lit && this.op.value is MissingValue
 
         override fun visitRexOpPathKey(node: Rex.Op.Path.Key, ctx: StaticType?): Rex {
             val root = visitRex(node.root, node.root.type)

diff --git a/partiql-planner/src/test/kotlin/org/partiql/planner/PlannerErrorReportingTests.kt b/partiql-planner/src/test/kotlin/org/partiql/planner/PlannerErrorReportingTests.kt
@@ -182,14 +182,16 @@ internal class PlannerErrorReportingTests {
             ),
             // Chained, demostrate missing trace.
             // TODO: We currently don't have a good way to retain missing value information. The following test
-            //  should have 2 errors.
+            //  could have 2 warnings. One for executing a path operation on a literal missing. And one for
+            //  executing a path operation on an expression that is known to result in the missing value.
             TestCase(
                 "MISSING['a'].a",
                 false,
                 assertOnProblemCount(1, 0)
             ),
             // TODO: We currently don't have a good way to retain missing value information. The following test
-            //  should have 2 errors.
+            //  could have 2 errors. One for executing a path operation on a literal missing. And one for
+            //  executing a path operation on an expression that is known to result in the missing value.
             TestCase(
                 "MISSING['a'].a",
                 true,
@@ -258,17 +260,15 @@ internal class PlannerErrorReportingTests {
 
             // 1 + not_a_function(1)
             //  The continuation will return all numeric type
-            // TODO: Should the warning count be 1? Does it matter if it is zero?
             TestCase(
                 "1 + not_a_function(1)",
                 false,
                 assertOnProblemCount(0, 1),
                 StaticType.unionOf(StaticType.INT4, StaticType.INT8, StaticType.INT, StaticType.FLOAT, StaticType.DECIMAL),
             ),
-            // TODO: Should the warning count be 1? Does it matter if it is zero?
             TestCase(
                 "1 + not_a_function(1)",
-                false,
+                true,
                 assertOnProblemCount(0, 1),
                 StaticType.unionOf(StaticType.INT4, StaticType.INT8, StaticType.INT, StaticType.FLOAT, StaticType.DECIMAL),
             ),