From 4ae74d351fafa02bd82919b2a2c434d0a2fcfa17 Mon Sep 17 00:00:00 2001 From: John Ed Quinn Date: Fri, 17 May 2024 10:25:09 -0700 Subject: [PATCH] Addresses PR comments --- .../planner/internal/typer/DynamicTyper.kt | 4 ++- .../planner/internal/typer/FnResolver.kt | 4 +-- .../planner/internal/typer/PlanTyper.kt | 34 +++++-------------- .../planner/internal/typer/TypeUtils.kt | 24 +++---------- .../internal/typer/PartiQLTyperTestBase.kt | 2 +- .../kotlin/org/partiql/planner/util/Utils.kt | 6 +--- .../resources/catalogs/default/pql/t_item.ion | 13 ------- .../resources/inputs/basics/case.sql | 15 -------- .../resources/inputs/basics/nullif.sql | 4 +-- .../kotlin/org/partiql/types/StaticType.kt | 31 ++++++++--------- .../org/partiql/plugins/local/LocalSchema.kt | 1 + 11 files changed, 38 insertions(+), 100 deletions(-) diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt index dc846b7069..eebbf36421 100644 --- a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt @@ -4,6 +4,8 @@ package org.partiql.planner.internal.typer import org.partiql.planner.internal.ir.Rex import org.partiql.types.StaticType +import org.partiql.value.MissingValue +import org.partiql.value.NullValue import org.partiql.value.PartiQLValueExperimental import org.partiql.value.PartiQLValueType import org.partiql.value.PartiQLValueType.ANY @@ -71,7 +73,7 @@ internal class DynamicTyper { */ private fun Rex.isLiteralAbsent(): Boolean { val op = this.op - return op is Rex.Op.Lit && (op.value.type == PartiQLValueType.MISSING || op.value.type == PartiQLValueType.NULL) + return op is Rex.Op.Lit && (op.value is MissingValue || op.value is NullValue) } /** diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/FnResolver.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/FnResolver.kt index b4d0cd1229..d595a051b2 100644 --- a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/FnResolver.kt +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/FnResolver.kt @@ -408,9 +408,9 @@ internal class FnResolver(private val header: Header) { // This does not imply the ability to CAST; this defines function resolution behavior. private val precedence: Map = listOf( @Suppress("DEPRECATION") - PartiQLValueType.NULL, // TODO: Remove + PartiQLValueType.NULL, // TODO: Remove once functions no longer specify parameter/return types with the NULL type. @Suppress("DEPRECATION") - PartiQLValueType.MISSING, // TODO: Remove + PartiQLValueType.MISSING, // TODO: Remove once functions no longer specify parameter/return types with the MISSING type. BOOL, INT8, INT16, 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 index 7adb377007..ba5c9cb15b 100644 --- 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 @@ -91,8 +91,8 @@ import org.partiql.types.StructType import org.partiql.types.TupleConstraint import org.partiql.types.function.FunctionSignature import org.partiql.value.BoolValue +import org.partiql.value.MissingValue import org.partiql.value.PartiQLValueExperimental -import org.partiql.value.PartiQLValueType import org.partiql.value.TextValue import org.partiql.value.boolValue import org.partiql.value.stringValue @@ -475,8 +475,8 @@ internal class PlanTyper( } (type as CollectionType).elementType }.toSet() - val finalType = unionOf(elementTypes).flatten() - return rex(finalType.swallowAny(), rexOpPathIndex(root, key)) + val finalType = unionOf(elementTypes) + return rex(finalType, rexOpPathIndex(root, key)) } override fun visitRexOpPathKey(node: Rex.Op.Path.Key, ctx: StaticType?): Rex { @@ -516,8 +516,7 @@ internal class PlanTyper( handleAlwaysMissing() return rex(ANY, rexOpPathKey(root, key)) } - // TODO: SwallowAny should happen by default - return rex(unionOf(elementType).swallowAny(), rexOpPathKey(root, key)) + return rex(unionOf(elementType), rexOpPathKey(root, key)) } override fun visitRexOpPathSymbol(node: Rex.Op.Path.Symbol, ctx: StaticType?): Rex { @@ -551,8 +550,7 @@ internal class PlanTyper( handleAlwaysMissing() return rex(ANY, Rex.Op.Path.Symbol(root, node.key)) } - // TODO: Flatten() should occur by default - else -> unionOf(paths.map { it.type }.toSet()).flatten() + else -> unionOf(paths.map { it.type }.toSet()) } // replace step only if all are disambiguated @@ -562,19 +560,7 @@ internal class PlanTyper( true -> firstPathOp false -> rexOpPathSymbol(root, node.key) } - return rex(type.swallowAny(), replacementOp) - } - - /** - * "Swallows" ANY. If ANY is one of the types in the UNION type, we return ANY. If not, we flatten and return - * the [type]. - */ - private fun StaticType.swallowAny(): StaticType { - val flattened = this.flatten() - return when (flattened.allTypes.any { it is AnyType }) { - true -> ANY - false -> flattened - } + return rex(type, replacementOp) } private fun rexString(str: String) = rex(STRING, rexOpLit(stringValue(str))) @@ -633,13 +619,13 @@ internal class PlanTyper( // Check literal missing inputs val argAlwaysMissing = args.any { val op = it.op as? Rex.Op.Lit ?: return@any false - op.value.type == PartiQLValueType.MISSING + op.value is MissingValue } if (argAlwaysMissing) { // TODO: The V1 branch has support for isMissable and isMissingCall. This codebase, however, does not // have support for these concepts yet. This specific commit (see Git blame) does not seek to add this // functionality. Below is a work-around for the lack of "isMissable" and "isMissingCall" - if (match.signature.name !in listOf("is_null", "is_missing", "eq")) { + if (match.signature.name !in listOf("is_null", "is_missing", "eq", "and", "or", "not")) { handleAlwaysMissing() } } @@ -1163,9 +1149,7 @@ internal class PlanTyper( isClosed && isOrdered -> { struct.fields.firstOrNull { entry -> binding.isEquivalentTo(entry.key) }?.let { (sensitive(it.key) to it.value) - } ?: run { - return null - } + } ?: return null } // 2. Struct is closed isClosed -> { diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/TypeUtils.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/TypeUtils.kt index f27319bac7..bf3bd07116 100644 --- a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/TypeUtils.kt +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/TypeUtils.kt @@ -28,22 +28,8 @@ import org.partiql.types.TimestampType import org.partiql.value.PartiQLValueExperimental import org.partiql.value.PartiQLValueType -@Suppress("DEPRECATION") -@Deprecated( - message = "This will be removed in a future major-version bump.", - replaceWith = ReplaceWith("ANY") -) -internal fun StaticType.isNullOrMissing(): Boolean = (this is NullType || this is MissingType) - internal fun StaticType.isText(): Boolean = (this is SymbolType || this is StringType) -@Suppress("DEPRECATION") -@Deprecated( - message = "This will be removed in a future major-version bump.", - replaceWith = ReplaceWith("ANY") -) -internal fun StaticType.isUnknown(): Boolean = (this.isNullOrMissing() || this == StaticType.NULL_OR_MISSING) - /** * Returns whether [this] *may* be of a specific type. AKA: is it the type? Is it a union that holds the type? */ @@ -52,16 +38,16 @@ internal inline fun StaticType.mayBeType(): Boolean { } /** - * For each type in [this] [StaticType.allTypes], the [block] will be invoked. Non-null outputs to the [block] will be - * returned. + * For each type in [this] type's [StaticType.allTypes], the [block] will be invoked. Non-null outputs of the [block]'s + * invocation will be returned. */ internal fun StaticType.inferListNotNull(block: (StaticType) -> StaticType?): List { return this.flatten().allTypes.mapNotNull { type -> block(type) } } /** - * For each type in [this] [StaticType.allTypes], the [block] will be invoked. Non-null outputs to the [block] will be - * returned. + * For each type in [this] type's [StaticType.allTypes], the [block] will be invoked. Non-null outputs of the [block]'s + * invocation will be returned. */ internal fun StaticType.inferRexListNotNull(block: (StaticType) -> Rex?): List { return this.flatten().allTypes.mapNotNull { type -> block(type) } @@ -195,7 +181,7 @@ internal fun StructType.exclude(steps: List, lastStepOption val output = fields.map { field -> val newField = if (steps.size == 1) { if (lastStepOptional) { - StructType.Field(field.key, field.value) // TODO: double check this + StructType.Field(field.key, field.value) } else { null } diff --git a/partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PartiQLTyperTestBase.kt b/partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PartiQLTyperTestBase.kt index 243c384ef6..a1867f4a39 100644 --- a/partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PartiQLTyperTestBase.kt +++ b/partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PartiQLTyperTestBase.kt @@ -101,7 +101,7 @@ abstract class PartiQLTyperTestBase { val actualType = root.type assert(actualType == StaticType.ANY) { buildString { - this.appendLine(" expected Type is : MISSING") + this.appendLine(" expected Type is : ANY") this.appendLine("actual Type is : $actualType") PlanPrinter.append(this, result.plan) } diff --git a/partiql-planner/src/test/kotlin/org/partiql/planner/util/Utils.kt b/partiql-planner/src/test/kotlin/org/partiql/planner/util/Utils.kt index adcd569fa9..a5da57fa49 100644 --- a/partiql-planner/src/test/kotlin/org/partiql/planner/util/Utils.kt +++ b/partiql-planner/src/test/kotlin/org/partiql/planner/util/Utils.kt @@ -29,11 +29,7 @@ fun cartesianProduct(a: List, b: List, vararg lists: List): Set = mapOf()): StaticType = - unionOf(types.toSet(), metas).flatten() + unionOf(types.toSet(), metas) /** * Creates a new [StaticType] as a union of the passed [types]. The values typed by the returned type - * are defined as the union of all values typed as [types] + * are defined as the union of all values typed as [types]. The returned type is flattened. * * @param types [StaticType] to be unioned. * @return [StaticType] representing the union of [types] @@ -33,12 +34,8 @@ public sealed class StaticType { @Suppress("DEPRECATION") public fun unionOf( types: Set, - metas: Map = mapOf(), - errorOnEmptyTypes: Boolean = false + metas: Map = mapOf() ): StaticType { - if (errorOnEmptyTypes && types.isEmpty()) { - throw IllegalStateException("Cannot make a union of zero types.") - } return when (types.isEmpty()) { true -> ANY false -> AnyOfType(types, metas).flatten() @@ -47,7 +44,7 @@ public sealed class StaticType { /** * Creates a new [StaticType] as a union of the passed [types]. The values typed by the returned type - * are defined as the union of all values typed as [types] + * are defined as the union of all values typed as [types]. The returned type is flattened. * * @param types [StaticType] to be unioned. * @return [StaticType] representing the union of [types] @@ -151,7 +148,8 @@ public sealed class StaticType { */ @Suppress("DEPRECATION") @Deprecated( - message = "This will be removed in a future major-version bump.", + message = "This will be removed in a future major-version bump. All types include the null value. Therefore," + + " this method is redundant.", replaceWith = ReplaceWith("") ) public fun asNullable(): StaticType = @@ -168,7 +166,8 @@ public sealed class StaticType { */ @Suppress("DEPRECATION") @Deprecated( - message = "This will be removed in a future major-version bump.", + message = "This will be removed in a future major-version bump. All types include the missing value." + + " Therefore, this method is redundant.", replaceWith = ReplaceWith("") ) public fun asOptional(): StaticType = @@ -214,7 +213,8 @@ public sealed class StaticType { */ @Suppress("DEPRECATION") @Deprecated( - message = "This will be removed in a future major-version bump.", + message = "This will be removed in a future major-version bump. All types are considered nullable. Therefore" + + " this method is redundant.", replaceWith = ReplaceWith("true") ) public fun isNullable(): Boolean = @@ -231,7 +231,8 @@ public sealed class StaticType { */ @Suppress("DEPRECATION") @Deprecated( - message = "This will be removed in a future major-version bump.", + message = "This will be removed in a future major-version bump. All types are considered missable. Therefore," + + " this method is redundant.", replaceWith = ReplaceWith("true") ) public fun isMissable(): Boolean = @@ -245,10 +246,6 @@ public sealed class StaticType { * Type is optional if it is Any, or Missing, or an AnyOfType that contains Any or Missing type */ @Suppress("DEPRECATION") - @Deprecated( - message = "This will be removed in a future major-version bump.", - replaceWith = ReplaceWith("true") - ) private fun isOptional(): Boolean = when (this) { is AnyType, MissingType -> true // Any includes Missing type diff --git a/plugins/partiql-local/src/main/kotlin/org/partiql/plugins/local/LocalSchema.kt b/plugins/partiql-local/src/main/kotlin/org/partiql/plugins/local/LocalSchema.kt index 1603c0cc28..03897ca917 100644 --- a/plugins/partiql-local/src/main/kotlin/org/partiql/plugins/local/LocalSchema.kt +++ b/plugins/partiql-local/src/main/kotlin/org/partiql/plugins/local/LocalSchema.kt @@ -82,6 +82,7 @@ public fun StringElement.toStaticType(): StaticType = when (textValue) { "list" -> error("`list` is not an atomic type") "sexp" -> error("`sexp` is not an atomic type") "struct" -> error("`struct` is not an atomic type") + "null", "missing" -> error("Absent values ($textValue) do not have a corresponding type.") else -> error("Invalid type `$textValue`") }