Addresses PR comments

Adds PType static factory methods with default parameters Renames factory methods for creating Datum and PartiQLValue Adds AST-to-Plan typing logic of integers to the Ion Variant Reverts the IonDatumReader changes to focus on limited scope Adds a lowerSafe() internal utility method for reducing code size Updates some Javadocs & comments
partiql · Dec 20, 2024 · 0f565af · 0f565af
1 parent f70aa7a
commit 0f565af
Show file tree

Hide file tree

Showing 29 changed files with 227 additions and 165 deletions.
diff --git a/partiql-cli/src/main/kotlin/org/partiql/cli/Main.kt b/partiql-cli/src/main/kotlin/org/partiql/cli/Main.kt
@@ -24,7 +24,7 @@ import org.partiql.spi.catalog.Session
 import org.partiql.spi.catalog.Table
 import org.partiql.spi.value.Datum
 import org.partiql.spi.value.DatumReader
-import org.partiql.spi.value.DatumUtils
+import org.partiql.spi.value.ValueUtils
 import org.partiql.spi.value.io.PartiQLValueTextWriter
 import picocli.CommandLine
 import java.io.File
@@ -205,7 +205,7 @@ internal class MainCommand : Runnable {
         // TODO add format support
         checkFormat(format)
         val writer = PartiQLValueTextWriter(System.out)
-        val p = DatumUtils.toPartiQLValue(result)
+        val p = ValueUtils.newPartiQLValue(result)
         writer.append(p) // TODO: Create a Datum writer
         println()
     }

diff --git a/partiql-cli/src/main/kotlin/org/partiql/cli/shell/Shell.kt b/partiql-cli/src/main/kotlin/org/partiql/cli/shell/Shell.kt
@@ -31,7 +31,7 @@ import org.jline.utils.InfoCmp
 import org.joda.time.Duration
 import org.partiql.cli.pipeline.Pipeline
 import org.partiql.spi.catalog.Session
-import org.partiql.spi.value.DatumUtils
+import org.partiql.spi.value.ValueUtils
 import org.partiql.spi.value.io.PartiQLValueTextWriter
 import java.io.Closeable
 import java.io.PrintStream
@@ -275,7 +275,7 @@ internal class Shell(
                         out.appendLine()
                         out.info("=== RESULT ===")
                         val writer = PartiQLValueTextWriter(out)
-                        val p = DatumUtils.toPartiQLValue(result)
+                        val p = ValueUtils.newPartiQLValue(result)
                         writer.append(p) // TODO: Create a Datum writer
                         out.appendLine()
                         out.appendLine()

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/helpers/DatumUtils.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/helpers/DatumUtils.kt
@@ -0,0 +1,20 @@
+package org.partiql.eval.internal.helpers
+
+import org.partiql.spi.value.Datum
+import org.partiql.types.PType
+
+internal object DatumUtils {
+
+    /**
+     * Calls [Datum.lower] if the datum is a variant, otherwise returns the datum. If you don't know whether the value
+     * is of type [PType.VARIANT], you should use [Datum.lowerSafe] before invoking whatever methods you intend to use.
+     * This is essentially a workaround for the fact that we currently don't know whether a particular expression will be
+     * [PType.VARIANT] or not. This can eventually be optimized to accommodate this.
+     */
+    internal fun Datum.lowerSafe(): Datum {
+        return when (this.type.code()) {
+            PType.VARIANT -> this.lower()
+            else -> this
+        }
+    }
+}
diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/helpers/ValueUtility.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/helpers/ValueUtility.kt
@@ -18,10 +18,11 @@ internal object ValueUtility {
         if (this.isNull || this.isMissing) {
             return false
         }
-        if (this.type.code() == PType.VARIANT) {
-            return this.lower().isTrue()
+        return when (this.type.code()) {
+            PType.VARIANT -> this.lower().isTrue()
+            PType.BOOL -> this.boolean
+            else -> false
         }
-        return this.type.code() == PType.BOOL && this.boolean
     }
 
     /**

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpIterate.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpIterate.kt
@@ -5,6 +5,7 @@ import org.partiql.eval.Environment
 import org.partiql.eval.ExprRelation
 import org.partiql.eval.ExprValue
 import org.partiql.eval.Row
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.spi.value.Datum
 import org.partiql.types.PType
 
@@ -16,14 +17,9 @@ internal class RelOpIterate(
     private var index: Long = 0
 
     override fun open(env: Environment) {
-        val r = expr.eval(env.push(Row()))
+        val r = expr.eval(env.push(Row())).lowerSafe()
         index = 0
-        iterator = records(r)
-    }
-
-    private fun records(r: Datum): Iterator<Datum> {
-        return when (r.type.code()) {
-            PType.VARIANT -> records(r.lower())
+        iterator = when (r.type.code()) {
             PType.BAG -> {
                 close()
                 throw TypeCheckException()

diff --git a/...iql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpIteratePermissive.kt b/...iql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpIteratePermissive.kt
@@ -4,6 +4,7 @@ import org.partiql.eval.Environment
 import org.partiql.eval.ExprRelation
 import org.partiql.eval.ExprValue
 import org.partiql.eval.Row
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.spi.value.Datum
 import org.partiql.types.PType
 
@@ -16,14 +17,9 @@ internal class RelOpIteratePermissive(
     private var isIndexable: Boolean = true
 
     override fun open(env: Environment) {
-        val r = expr.eval(env.push(Row()))
+        val r = expr.eval(env.push(Row())).lowerSafe()
         index = 0
-        iterator = records(r)
-    }
-
-    private fun records(r: Datum): Iterator<Datum> {
-        return when (r.type.code()) {
-            PType.VARIANT -> records(r.lower())
+        iterator = when (r.type.code()) {
             PType.BAG -> {
                 isIndexable = false
                 r.iterator()

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpScan.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpScan.kt
@@ -5,8 +5,8 @@ import org.partiql.eval.Environment
 import org.partiql.eval.ExprRelation
 import org.partiql.eval.ExprValue
 import org.partiql.eval.Row
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.eval.internal.helpers.RecordValueIterator
-import org.partiql.spi.value.Datum
 import org.partiql.types.PType
 
 internal class RelOpScan(
@@ -16,17 +16,12 @@ internal class RelOpScan(
     private lateinit var records: Iterator<Row>
 
     override fun open(env: Environment) {
-        val r = expr.eval(env.push(Row()))
-        records = r.records()
-    }
-
-    private fun Datum.records(): RecordValueIterator {
-        return when (this.type.code()) {
-            PType.VARIANT -> this.lower().records()
-            PType.ARRAY, PType.BAG -> RecordValueIterator(this.iterator())
+        val r = expr.eval(env.push(Row())).lowerSafe()
+        records = when (r.type.code()) {
+            PType.ARRAY, PType.BAG -> RecordValueIterator(r.iterator())
             else -> {
                 close()
-                throw TypeCheckException("Unexpected type for scan: ${this.type}")
+                throw TypeCheckException("Unexpected type for scan: ${r.type}")
             }
         }
     }

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpScanPermissive.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpScanPermissive.kt
@@ -4,8 +4,8 @@ import org.partiql.eval.Environment
 import org.partiql.eval.ExprRelation
 import org.partiql.eval.ExprValue
 import org.partiql.eval.Row
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.eval.internal.helpers.RecordValueIterator
-import org.partiql.spi.value.Datum
 import org.partiql.types.PType
 
 internal class RelOpScanPermissive(
@@ -15,14 +15,8 @@ internal class RelOpScanPermissive(
     private lateinit var records: Iterator<Row>
 
     override fun open(env: Environment) {
-        val r = expr.eval(env.push(Row()))
-        records = r.records()
-    }
-
-    private fun Datum.records(): Iterator<Row> {
-        val r = this
-        return when (type.code()) {
-            PType.VARIANT -> r.lower().records()
+        val r = expr.eval(env.push(Row())).lowerSafe()
+        records = when (r.type.code()) {
             PType.BAG, PType.ARRAY -> RecordValueIterator(r.iterator())
             else -> iterator { yield(Row(arrayOf(r))) }
         }

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpUnpivot.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelOpUnpivot.kt
@@ -5,6 +5,7 @@ import org.partiql.eval.Environment
 import org.partiql.eval.ExprRelation
 import org.partiql.eval.ExprValue
 import org.partiql.eval.Row
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.spi.value.Datum
 import org.partiql.spi.value.Field
 import org.partiql.types.PType
@@ -58,13 +59,7 @@ internal sealed class RelOpUnpivot : ExprRelation {
     class Strict(private val expr: ExprValue) : RelOpUnpivot() {
 
         override fun struct(): Datum {
-            val v = expr.eval(env.push(Row())).let {
-                if (it.type.code() == PType.VARIANT) {
-                    it.lower()
-                } else {
-                    it
-                }
-            }
+            val v = expr.eval(env.push(Row())).lowerSafe()
             if (v.type.code() != PType.STRUCT && v.type.code() != PType.ROW) {
                 throw TypeCheckException()
             }
@@ -84,13 +79,7 @@ internal sealed class RelOpUnpivot : ExprRelation {
     class Permissive(private val expr: ExprValue) : RelOpUnpivot() {
 
         override fun struct(): Datum {
-            val v = expr.eval(env.push(Row())).let {
-                if (it.type.code() == PType.VARIANT) {
-                    it.lower()
-                } else {
-                    it
-                }
-            }
+            val v = expr.eval(env.push(Row())).lowerSafe()
             if (v.isMissing) {
                 return Datum.struct(emptyList())
             }

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/CastTable.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/CastTable.kt
@@ -71,9 +71,6 @@ internal object CastTable {
         if (target.code() == DYNAMIC) {
             return source
         }
-        if (source.type.code() == VARIANT) {
-            return cast(source.lower(), target)
-        }
         val cast = _table[source.type.code()][target.code()]
             ?: throw TypeCheckException("CAST(${source.type} AS $target) is not supported.")
         return try {
@@ -109,6 +106,7 @@ internal object CastTable {
         registerTimestamp()
         registerDate()
         registerTime()
+        registerVariant()
     }
 
     private fun String.pad(): String {
@@ -497,6 +495,13 @@ internal object CastTable {
         register(TIMEZ, TIMESTAMPZ) { x, _ -> Datum.timestamp(DateTimeValue.timestamp(DateTimeValue.date(1970, 1, 1), x.time)) }
     }
 
+    private fun registerVariant() {
+        PType.codes().forEach { pType ->
+            register(VARIANT, pType) { x, t -> cast(x.lower(), t) }
+        }
+        register(VARIANT, VARIANT) { x, _ -> x }
+    }
+
     private fun register(source: Int, target: Int, cast: (Datum, PType) -> Datum) {
         _table[source][target] = cast
     }

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprCallDynamic.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprCallDynamic.kt
@@ -4,6 +4,7 @@ import org.partiql.errors.TypeCheckException
 import org.partiql.eval.Environment
 import org.partiql.eval.ExprValue
 import org.partiql.eval.Row
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.eval.internal.operator.rex.ExprCallDynamic.Candidate
 import org.partiql.eval.internal.operator.rex.ExprCallDynamic.CoercionFamily.DYNAMIC
 import org.partiql.eval.internal.operator.rex.ExprCallDynamic.CoercionFamily.UNKNOWN
@@ -45,14 +46,7 @@ internal class ExprCallDynamic(
     private val candidates: MutableMap<List<PType>, Candidate> = mutableMapOf()
 
     override fun eval(env: Environment): Datum {
-        val actualArgs = args.map {
-            val arg = it.eval(env)
-            if (arg.type.code() == PType.VARIANT) {
-                arg.lower()
-            } else {
-                arg
-            }
-        }.toTypedArray()
+        val actualArgs = args.map { it.eval(env).lowerSafe() }.toTypedArray()
         val actualTypes = actualArgs.map { it.type }
         var candidate = candidates[actualTypes]
         if (candidate == null) {

diff --git a/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprStructPermissive.kt b/partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprStructPermissive.kt
@@ -2,6 +2,7 @@ package org.partiql.eval.internal.operator.rex
 
 import org.partiql.eval.Environment
 import org.partiql.eval.ExprValue
+import org.partiql.eval.internal.helpers.DatumUtils.lowerSafe
 import org.partiql.spi.value.Datum
 import org.partiql.spi.value.Field
 import org.partiql.types.PType
@@ -10,7 +11,7 @@ internal class ExprStructPermissive(private val fields: List<ExprStructField>) :
     ExprValue {
     override fun eval(env: Environment): Datum {
         val fields = fields.mapNotNull {
-            val key = it.key.eval(env)
+            val key = it.key.eval(env).lowerSafe()
             val keyString = key.getTextOrNull() ?: return@mapNotNull null
             val value = it.value.eval(env)
             when (value.isMissing) {
@@ -32,7 +33,6 @@ internal class ExprStructPermissive(private val fields: List<ExprStructField>) :
                 return null
             }
             return when (this.type.code()) {
-                PType.VARIANT -> this.lower().getTextOrNull()
                 PType.STRING, PType.CHAR -> this.string
                 else -> null
             }

diff --git a/partiql-eval/src/test/kotlin/org/partiql/eval/internal/SuccessTestCase.kt b/partiql-eval/src/test/kotlin/org/partiql/eval/internal/SuccessTestCase.kt
@@ -12,7 +12,7 @@ import org.partiql.spi.catalog.Session
 import org.partiql.spi.catalog.Table
 import org.partiql.spi.value.Datum
 import org.partiql.spi.value.DatumReader
-import org.partiql.spi.value.DatumUtils
+import org.partiql.spi.value.ValueUtils
 import org.partiql.types.StaticType
 import org.partiql.types.fromStaticType
 import org.partiql.value.PartiQLValue
@@ -31,7 +31,7 @@ public class SuccessTestCase(
         expected: PartiQLValue,
         mode: Mode = Mode.PERMISSIVE(),
         globals: List<Global> = emptyList(),
-    ) : this(input, DatumUtils.toDatum(expected), mode, globals)
+    ) : this(input, ValueUtils.newDatum(expected), mode, globals)
 
     private val compiler = PartiQLCompiler.standard()
     private val parser = PartiQLParser.standard()

diff --git a/partiql-eval/src/test/kotlin/org/partiql/eval/internal/TypingTestCase.kt b/partiql-eval/src/test/kotlin/org/partiql/eval/internal/TypingTestCase.kt
@@ -9,7 +9,7 @@ import org.partiql.planner.PartiQLPlanner
 import org.partiql.spi.catalog.Catalog
 import org.partiql.spi.catalog.Session
 import org.partiql.spi.value.Datum
-import org.partiql.spi.value.DatumUtils
+import org.partiql.spi.value.ValueUtils
 import org.partiql.spi.value.io.PartiQLValueIonWriterBuilder
 import org.partiql.types.PType
 import org.partiql.value.PartiQLValue
@@ -29,7 +29,7 @@ public class TypingTestCase(
 
     override fun run() {
         val (permissiveResult, plan) = run(mode = Mode.PERMISSIVE())
-        val permissiveResultPValue = DatumUtils.toPartiQLValue(permissiveResult)
+        val permissiveResultPValue = ValueUtils.newPartiQLValue(permissiveResult)
         val assertionCondition = try {
             expectedPermissive == permissiveResultPValue // TODO: Assert using Datum
         } catch (t: Throwable) {

diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/FnResolver.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/FnResolver.kt
@@ -27,8 +27,6 @@ internal object FnResolver {
     /**
      * Resolution of either a static or dynamic function.
      *
-     * TODO: How do we handle DYNAMIC?
-     *
      * @param variants
      * @param args
      * @return
@@ -46,7 +44,7 @@ internal object FnResolver {
             }
         }
 
-        // 2. If there are DYNAMIC arguments, return all candidates
+        // 2. If there are DYNAMIC/VARIANT arguments, return all candidates
         val isDynamic = args.any { it.code() == PType.DYNAMIC || it.code() == PType.VARIANT }
         if (isDynamic) {
             val orderedMatches = candidates.sortedWith(FnComparator)

diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/casts/CastTable.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/casts/CastTable.kt
@@ -63,6 +63,7 @@ internal class CastTable private constructor(
                     cast(it)
                 }
             }
+            // A VARIANT can be cast to all other types.
             graph[PType.VARIANT] = relationships {
                 PType.codes().map {
                     cast(it)

diff --git a/partiql-spi/src/main/java/org/partiql/spi/value/Datum.java b/partiql-spi/src/main/java/org/partiql/spi/value/Datum.java
@@ -203,7 +203,7 @@ default short getShort() {
      *                                       {@link #isNull()} returns false before attempting to invoke this method.
      */
     default int getInt() {
-        throw new UnsupportedOperationException("Has type: " + getType() + " and class " + this.getClass().getName());
+        throw new UnsupportedOperationException();
     }
 
     /**

diff --git a/partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/internal/Accumulator.kt b/partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/internal/Accumulator.kt
@@ -45,9 +45,6 @@ internal fun comparisonAccumulator(comparator: Comparator<Datum>): (Datum?, Datu
     }
 
 internal fun checkIsNumberType(funcName: String, value: Datum) {
-    if (value.type.code() == PType.VARIANT) {
-        return checkIsNumberType(funcName, value.lower())
-    }
     if (!value.type.isNumber()) {
         throw TypeCheckException("Expected NUMBER but received ${value.type}.")
     }