diff --git a/CHANGELOG.md b/CHANGELOG.md index d0114311c..2cca98ae5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Thank you to all who have contributed! --> +## [0.14.9] + +### Added + +### Changed +- With full, closed schema, the planner will now give a plan-time warning when it can prove an exclude path will never +exclude a value (relevant issue -- https://github.com/partiql/partiql-lang/issues/91). + +### Deprecated + +### Fixed + +### Removed + +### Security + +### Contributors +- @alancai98 + ## [0.14.8] ### Added @@ -1110,7 +1129,8 @@ breaking changes if migrating from v0.9.2. The breaking changes accidentally int ### Added Initial alpha release of PartiQL. -[Unreleased]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.8...HEAD +[Unreleased]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.9...HEAD +[0.14.8]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.8...v0.14.9 [0.14.8]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.7...v0.14.8 [0.14.7]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.6...v0.14.7 [0.14.6]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.5...v0.14.6 diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/Errors.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/Errors.kt index 73058ede2..fc29fdfe3 100644 --- a/partiql-planner/src/main/kotlin/org/partiql/planner/Errors.kt +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/Errors.kt @@ -178,6 +178,12 @@ public sealed class PlanningProblemDetails( ProblemSeverity.ERROR, { "Exclude expression given an unresolvable root '$root'" } ) + + public data class InvalidExcludePath(val path: String) : + PlanningProblemDetails( + ProblemSeverity.WARNING, + { "Exclude path '$path' does not exclude any values" } + ) } private fun quotationHint(caseSensitive: Boolean) = diff --git a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/ExcludeUtils.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/ExcludeUtils.kt new file mode 100644 index 000000000..0da013ef3 --- /dev/null +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/ExcludeUtils.kt @@ -0,0 +1,146 @@ +package org.partiql.planner.internal.typer + +import org.partiql.errors.Problem +import org.partiql.errors.ProblemCallback +import org.partiql.errors.UNKNOWN_PROBLEM_LOCATION +import org.partiql.planner.PlanningProblemDetails +import org.partiql.planner.internal.ir.Identifier +import org.partiql.planner.internal.ir.Identifier.CaseSensitivity +import org.partiql.planner.internal.ir.Rel +import org.partiql.planner.internal.ir.Rex +import org.partiql.types.AnyOfType +import org.partiql.types.CollectionType +import org.partiql.types.StaticType +import org.partiql.types.StructType + +internal object ExcludeUtils { + /** + * Checks for every exclude path in [paths], that a value in the [bindings] is excluded. If there is no value excluded, + * a warning is added to the [onProblem] callback. + */ + internal fun checkForInvalidExcludePaths(bindings: List, paths: List, onProblem: ProblemCallback) { + paths.forEach { excludePath -> + val root = excludePath.root + val steps = excludePath.steps + val excludesSomething = bindings.any { binding -> + when (root) { + is Rex.Op.Var.Unresolved -> { + when (val id = root.identifier) { + is Identifier.Symbol -> { + if (id.isEquivalentTo(binding.name)) { + binding.type.checkExclude(steps) + } else { + false + } + } + is Identifier.Qualified -> { + if (id.root.isEquivalentTo(binding.name)) { + binding.type.checkExclude(steps) + } else { + false + } + } + } + } + is Rex.Op.Var.Resolved -> false // root should be unresolved + } + } + // If nothing is excluded by `excludePath`, add a warning + if (!excludesSomething) { + onProblem( + Problem( + sourceLocation = UNKNOWN_PROBLEM_LOCATION, + details = PlanningProblemDetails.InvalidExcludePath( + excludePath.toProblemString() + ) + ) + ) + } + } + } + + /** + * Checks whether [steps] will exclude a value from [this] [StaticType]. + */ + private fun StaticType.checkExclude(steps: List): Boolean { + return when (this) { + is StructType -> this.checkExclude(steps) + is CollectionType -> this.checkExclude(steps) + is AnyOfType -> this.types.any { it.checkExclude(steps) } + else -> steps.isEmpty() + } + } + + /** + * Checks whether [steps] will exclude a value from [this] [StructType]. + */ + private fun StructType.checkExclude(steps: List): Boolean { + // Ignore open structs + if (steps.isEmpty() || !this.contentClosed) { + return true + } + val step = steps.first() + return fields.any { field -> + when (step) { + is Rel.Op.Exclude.Step.StructField -> { + step.symbol.isEquivalentTo(field.key) && field.value.checkExclude(steps.drop(1)) + } + is Rel.Op.Exclude.Step.StructWildcard -> field.value.checkExclude(steps.drop(1)) + else -> false + } + } + } + + /** + * Checks whether [steps] will exclude a value from [this] [CollectionType]. + */ + private fun CollectionType.checkExclude(steps: List): Boolean { + if (steps.isEmpty()) { + return true + } + return when (steps.first()) { + is Rel.Op.Exclude.Step.CollIndex, is Rel.Op.Exclude.Step.CollWildcard -> { + val e = this.elementType + e.checkExclude(steps.drop(1)) + } + else -> false + } + } + + // `EXCLUDE` path printing functions for problem printing + private fun Rel.Op.Exclude.Item.toProblemString(): String { + val root = when (root) { + is Rex.Op.Var.Resolved -> root.ref.toString() + is Rex.Op.Var.Unresolved -> root.identifier.toProblemString() + } + val steps = steps.map { + when (it) { + is Rel.Op.Exclude.Step.CollIndex -> "[${it.index}]" + is Rel.Op.Exclude.Step.CollWildcard -> "[*]" + is Rel.Op.Exclude.Step.StructField -> ".${it.symbol.toProblemString()}" + is Rel.Op.Exclude.Step.StructWildcard -> ".*" + } + } + return root + steps.joinToString(separator = "") + } + + private fun Identifier.toProblemString(): String { + return when (val id = this) { + is Identifier.Symbol -> { + id.toProblemString() + } + is Identifier.Qualified -> { + val root = id.root.toProblemString() + val steps = id.steps.map { it.toProblemString() } + root + steps.joinToString(separator = "") + } + } + } + + private fun Identifier.Symbol.toProblemString(): String { + return when (this.caseSensitivity) { + CaseSensitivity.SENSITIVE -> "\"${symbol}\"" + CaseSensitivity.INSENSITIVE -> symbol + } + } +} 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 55c023e6b..249ba0cdd 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 @@ -406,8 +406,6 @@ internal class PlanTyper( * - Excluding collection wildcards (e.g. t.a[*].b) * * There are still discussion points regarding the following edge cases: - * - EXCLUDE on a tuple attribute that doesn't exist -- give an error/warning? - * - currently no error * - EXCLUDE on a tuple attribute that has duplicates -- give an error/warning? exclude one? exclude both? * - currently excludes both w/ no error * - EXCLUDE on a collection index as the last step -- mark element type as optional? @@ -427,8 +425,9 @@ internal class PlanTyper( val input = visitRel(node.input, ctx) // apply exclusions to the input schema - val init = input.type.schema.map { it.copy() } - val schema = node.items.fold((init)) { bindings, item -> excludeBindings(bindings, item) } + val initBindings = input.type.schema.map { it.copy() } + ExcludeUtils.checkForInvalidExcludePaths(initBindings, node.items, onProblem) + val schema = node.items.fold((initBindings)) { bindings, item -> excludeBindings(bindings, item) } // rewrite val type = ctx!!.copy(schema = schema) @@ -1645,7 +1644,14 @@ internal class PlanTyper( it } } - is Identifier.Qualified -> it + is Identifier.Qualified -> if (id.root.isEquivalentTo(it.name)) { + matchedRoot = true + // recompute the StaticType of this binding after apply the exclusions + val type = it.type.exclude(item.steps, false) + it.copy(type = type) + } else { + it + } } } is Rex.Op.Var.Resolved -> it 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 d83c45de5..624e84d28 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 @@ -241,7 +241,7 @@ internal fun CollectionType.exclude(steps: List, lastStepOp * @param other * @return */ -private fun Identifier.Symbol.isEquivalentTo(other: String): Boolean = when (caseSensitivity) { +internal fun Identifier.Symbol.isEquivalentTo(other: String): Boolean = when (caseSensitivity) { Identifier.CaseSensitivity.SENSITIVE -> symbol.equals(other) Identifier.CaseSensitivity.INSENSITIVE -> symbol.equals(other, ignoreCase = true) } 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 index 95099c687..7a598e693 100644 --- 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 @@ -13,6 +13,7 @@ import org.junit.jupiter.params.provider.ArgumentsProvider import org.junit.jupiter.params.provider.ArgumentsSource import org.junit.jupiter.params.provider.MethodSource import org.partiql.errors.Problem +import org.partiql.errors.ProblemSeverity import org.partiql.errors.UNKNOWN_PROBLEM_LOCATION import org.partiql.parser.PartiQLParser import org.partiql.plan.Identifier @@ -39,7 +40,6 @@ import org.partiql.types.NumberConstraint import org.partiql.types.SexpType import org.partiql.types.StaticType import org.partiql.types.StaticType.Companion.MISSING -import org.partiql.types.StaticType.Companion.NULL import org.partiql.types.StaticType.Companion.unionOf import org.partiql.types.StringType import org.partiql.types.StructType @@ -1683,9 +1683,8 @@ class PlanTyperTestsPorted { ) ) ), - // EXCLUDE regression test (behavior subject to change pending RFC) SuccessTestCase( - name = "EXCLUDE with non-existent attribute reference", + name = "EXCLUDE with non-existent attribute reference -- warning", key = key("exclude-25"), expected = BagType( StructType( @@ -1699,7 +1698,13 @@ class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.attr_does_not_exist") + ) + } ), // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( @@ -1825,9 +1830,8 @@ class PlanTyperTestsPorted { ) ) ), - // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( - name = "invalid exclude collection wildcard", + name = "invalid exclude collection wildcard -- warning", key = key("exclude-29"), expected = BagType( elementType = StructType( @@ -1857,11 +1861,16 @@ class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.a[*]") + ) + } ), - // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( - name = "invalid exclude collection index", + name = "invalid exclude collection index -- warning", key = key("exclude-30"), expected = BagType( elementType = StructType( @@ -1891,11 +1900,16 @@ class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.a[1]") + ) + } ), - // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( - name = "invalid exclude tuple attr", + name = "invalid exclude tuple attr -- warning", key = key("exclude-31"), expected = BagType( elementType = StructType( @@ -1917,11 +1931,16 @@ class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.a.b") + ) + } ), - // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( - name = "invalid exclude tuple wildcard", + name = "invalid exclude tuple wildcard - warning", key = key("exclude-32"), expected = BagType( elementType = StructType( @@ -1943,11 +1962,16 @@ class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.a.*") + ) + } ), - // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( - name = "invalid exclude tuple attr step", + name = "invalid exclude tuple attr step - warning", key = key("exclude-33"), expected = BagType( elementType = StructType( @@ -1969,7 +1993,13 @@ class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.b") + ) + } ), // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning ErrorTestCase( @@ -2085,6 +2115,139 @@ class PlanTyperTestsPorted { ) ) ), + SuccessTestCase( + name = "EXCLUDE with case-sensitive tuple reference not matching - warning", + key = key("exclude-37"), + expected = BagType( + StructType( + fields = mapOf( + "a" to StructType( + fields = mapOf( + "B" to StructType( + fields = mapOf( + "c" to StaticType.INT4, + "d" to StaticType.STRING + ), + contentClosed = true, + constraints = setOf( + TupleConstraint.Open(false), + TupleConstraint.UniqueAttrs(true) + ) + ), + ), + contentClosed = true, + constraints = setOf(TupleConstraint.Open(false), TupleConstraint.UniqueAttrs(true)) + ), + ), + contentClosed = true, + constraints = setOf( + TupleConstraint.Open(false), + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.\"a\".\"b\".c") + ) + } + ), + SuccessTestCase( + name = "EXCLUDE with case-sensitive tuple reference not matching earlier step - warning", + key = key("exclude-38"), + expected = BagType( + StructType( + fields = mapOf( + "a" to StructType( + fields = mapOf( + "B" to StructType( + fields = mapOf( + "c" to StaticType.INT4, + "d" to StaticType.STRING + ), + contentClosed = true, + constraints = setOf( + TupleConstraint.Open(false), + TupleConstraint.UniqueAttrs(true) + ) + ), + ), + contentClosed = true, + constraints = setOf(TupleConstraint.Open(false), TupleConstraint.UniqueAttrs(true)) + ), + ), + contentClosed = true, + constraints = setOf( + TupleConstraint.Open(false), + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ) + ), + warnings = assertProblemExists { + Problem( + UNKNOWN_PROBLEM_LOCATION, + PlanningProblemDetails.InvalidExcludePath("t.\"A\".\"b\".c") + ) + } + ), + SuccessTestCase( + name = "EXCLUDE with an open struct - no warning or error", + catalog = CATALOG_B, + key = key("exclude-39"), + expected = BagType( + elementType = StructType( + fields = mapOf( + "b" to StructType( + fields = mapOf( + "b" to StaticType.INT4 + ), + contentClosed = false, + constraints = setOf( + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ), + ), + contentClosed = false, + constraints = setOf( + TupleConstraint.Open(true), + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ) + ) + ), + SuccessTestCase( + name = "EXCLUDE with an open struct; nonexistent attribute in the open struct - no warning or error", + catalog = CATALOG_B, + key = key("exclude-40"), + expected = BagType( + elementType = StructType( + fields = mapOf( + "b" to StructType( + fields = mapOf( + "b" to StaticType.INT4 + ), + contentClosed = false, + constraints = setOf( + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ), + "c" to StaticType.INT4 + ), + contentClosed = false, + constraints = setOf( + TupleConstraint.Open(true), + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ) + ) + ), ) @JvmStatic @@ -3818,9 +3981,11 @@ class PlanTyperTestsPorted { val plan = infer(input, session, collector) when (val statement = plan.statement) { is Statement.Query -> { - assert(collector.problems.isEmpty()) { + val errors = collector.problems.filter { it.details.severity == ProblemSeverity.ERROR } + val warnings = collector.problems.filter { it.details.severity == ProblemSeverity.WARNING } + assert(errors.isEmpty()) { buildString { - appendLine(collector.problems.toString()) + appendLine(errors.toString()) appendLine() PlanPrinter.append(this, statement) } @@ -3835,6 +4000,10 @@ class PlanTyperTestsPorted { PlanPrinter.append(this, statement) } } + if (warnings.isNotEmpty()) { + assert(tc.warnings != null) { "Expected no warnings but warnings were found: $warnings" } + tc.warnings?.handle(warnings, true) + } } } } diff --git a/partiql-planner/src/testFixtures/resources/catalogs/default/b/b/b_open.ion b/partiql-planner/src/testFixtures/resources/catalogs/default/b/b/b_open.ion new file mode 100644 index 000000000..cace9c81c --- /dev/null +++ b/partiql-planner/src/testFixtures/resources/catalogs/default/b/b/b_open.ion @@ -0,0 +1,23 @@ +{ + type: "struct", + constraints: [ unique, ordered ], // open struct + fields: [ + { + name: "b", + type: { + type: "struct", + constraints: [ unique, ordered ], // open struct + fields: [ + { + name: "b", + type: "int32", + } + ] + } + }, + { + name: "c", + type: "int32", + } + ] +} diff --git a/partiql-planner/src/testFixtures/resources/inputs/schema_inferencer/exclude.sql b/partiql-planner/src/testFixtures/resources/inputs/schema_inferencer/exclude.sql index a6742e73c..30a65068a 100644 --- a/partiql-planner/src/testFixtures/resources/inputs/schema_inferencer/exclude.sql +++ b/partiql-planner/src/testFixtures/resources/inputs/schema_inferencer/exclude.sql @@ -470,4 +470,40 @@ FROM << >> AS t; --#[exclude-36] -SELECT * EXCLUDE t.c FROM b.b.b AS t; \ No newline at end of file +SELECT * EXCLUDE t.c FROM b.b.b AS t; + +-- exclude path does not exclude value +--#[exclude-37] +SELECT * EXCLUDE t."a"."b".c +FROM << + { + 'a': { + 'B': { + 'c': 0, + 'd': 'foo' + } + } + } + >> AS t; + +-- exclude path does not exclude value +--#[exclude-38] +SELECT * EXCLUDE t."A"."b".c +FROM << + { + 'a': { + 'B': { + 'c': 0, + 'd': 'foo' + } + } + } + >> AS t; + +-- EXCLUDE on a struct with open content +--#[exclude-39] +SELECT * EXCLUDE t.c FROM b.b.b_open AS t; + +-- EXCLUDE on a struct with open content; nonexistent attribute in the open struct +--#[exclude-40] +SELECT * EXCLUDE t.non_existent_attr FROM b.b.b_open AS t;