From f426c2ce9e8d37e2e4f69d5f3fb14646061e09d7 Mon Sep 17 00:00:00 2001 From: Alan Cai Date: Fri, 20 Sep 2024 13:11:09 -0700 Subject: [PATCH 1/3] Port: Adds warnings for `EXCLUDE` paths that don't exclude any value (#1586) --- CHANGELOG.md | 9 + .../org/partiql/planner/internal/PErrors.kt | 16 ++ .../planner/internal/typer/ExcludeUtils.kt | 120 +++++++++ .../planner/internal/typer/PlanTyper.kt | 7 +- .../planner/internal/typer/TypeUtils.kt | 12 + .../internal/typer/PlanTyperTestsPorted.kt | 244 ++++++++++++++---- .../resources/catalogs/default/b/b/b_open.ion | 23 ++ .../inputs/schema_inferencer/exclude.sql | 38 ++- partiql-spi/api/partiql-spi.api | 1 + .../java/org/partiql/spi/errors/PError.java | 13 + .../org/partiql/spi/errors/PErrorCodeTests.kt | 1 + 11 files changed, 427 insertions(+), 57 deletions(-) create mode 100644 partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/ExcludeUtils.kt create mode 100644 partiql-planner/src/testFixtures/resources/catalogs/default/b/b/b_open.ion diff --git a/CHANGELOG.md b/CHANGELOG.md index 610031c2f7..93759a23b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Contributors Thank you to all who have contributed! --> +## [Unreleased] + +- 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). ## [1.0.0-rc.3] - 2024-12-10 @@ -1129,11 +1133,16 @@ breaking changes if migrating from v0.9.2. The breaking changes accidentally int ### Added Initial alpha release of PartiQL. +<<<<<<< HEAD [Unreleased]: https://github.com/partiql/partiql-lang-kotlin/compare/v1.0.0-rc.3...HEAD [1.0.0-rc.3]: https://github.com/partiql/partiql-lang-kotlin/compare/v1.0.0-rc.2...v1.0.0-rc.3 [1.0.0-rc.2]: https://github.com/partiql/partiql-lang-kotlin/compare/v1.0.0-rc.1...v1.0.0-rc.2 [1.0.0-rc.1]: https://github.com/partiql/partiql-lang-kotlin/compare/v1.0.0-perf.1...v1.0.0-rc.1 [0.14.9]: https://github.com/partiql/partiql-lang-kotlin/compare/v0.14.8...v0.14.9 +======= +[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 +>>>>>>> 8634620e2 ([0.14.9-SNAPSHOT] Adds warnings for `EXCLUDE` paths that don't exclude any value (#1586)) [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/internal/PErrors.kt b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/PErrors.kt index 6e8ebdae72..27229165f7 100644 --- a/partiql-planner/src/main/kotlin/org/partiql/planner/internal/PErrors.kt +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/PErrors.kt @@ -184,4 +184,20 @@ internal object PErrors { mapOf("ID" to id, "LOCALS" to locals) ) } + + /** + * @param path see [PError.INVALID_EXCLUDE_PATH] + * @return an error representing [PError.INVALID_EXCLUDE_PATH] + */ + internal fun invalidExcludePath( + path: String + ): PError { + return PError( + PError.INVALID_EXCLUDE_PATH, + Severity.WARNING(), + PErrorKind.SEMANTIC(), + null, + mapOf("PATH" to path) + ) + } } 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 0000000000..9710aa11a3 --- /dev/null +++ b/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/ExcludeUtils.kt @@ -0,0 +1,120 @@ +package org.partiql.planner.internal.typer + +import org.partiql.planner.internal.PErrors +import org.partiql.planner.internal.ir.Rel +import org.partiql.planner.internal.ir.Rex +import org.partiql.spi.catalog.Identifier +import org.partiql.spi.errors.PErrorListener +import org.partiql.types.PType + +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: PErrorListener) { + paths.forEach { excludePath -> + val root = excludePath.root + val steps = excludePath.steps + val excludesSomething = bindings.any { binding -> + when (root) { + is Rex.Op.Var.Unresolved -> { + if (root.identifier.first().matches(binding.name)) { + binding.type.checkExclude(steps) + } else { + false + } + } + else -> false // root should be unresolved + } + } + // If nothing is excluded by `excludePath`, add a warning + if (!excludesSomething) { + onProblem.report(PErrors.invalidExcludePath(excludePath.toProblemString())) + } + } + } + + /** + * Checks whether [steps] will exclude a value from [this]. + */ + private fun PType.checkExclude(steps: List): Boolean { + return when (this.code()) { + PType.ROW -> this.checkRowExclude(steps) + PType.ARRAY, PType.BAG -> this.checkCollectionExclude(steps) + PType.DYNAMIC, PType.VARIANT -> true + else -> steps.isEmpty() + } + } + + /** + * Checks whether [steps] will exclude a value from [this] [PType.ROW]. + */ + private fun PType.checkRowExclude(steps: List): Boolean { + // Ignore open structs + if (steps.isEmpty()) { + return true + } + val step = steps.first() + return fields.any { field -> + when (val type = step.type) { + is Rel.Op.Exclude.Type.StructSymbol -> { + Identifier.regular(type.symbol).first().matches(field.name) && field.type.checkExclude(steps.drop(1)) + } + is Rel.Op.Exclude.Type.StructKey -> { + type.key == field.name && field.type.checkExclude(steps.drop(1)) + } + is Rel.Op.Exclude.Type.StructWildcard -> field.type.checkExclude(steps.drop(1)) + else -> false + } + } + } + + /** + * Checks whether [steps] will exclude a value from [this] [PType.BAG]/[PType.ARRAY]. + */ + private fun PType.checkCollectionExclude(steps: List): Boolean { + if (steps.isEmpty()) { + return true + } + val first = steps.first().type + return when (first) { + is Rel.Op.Exclude.Type.CollIndex, is Rel.Op.Exclude.Type.CollWildcard -> { + val e = this.typeParameter + e.checkExclude(steps.drop(1)) + } + else -> false + } + } + + // `EXCLUDE` path printing functions for problem printing + private fun Rel.Op.Exclude.Path.toProblemString(): String { + val root = when (root) { + is Rex.Op.Var.Unresolved -> root.identifier.toProblemString() + is Rex.Op.Var.Local -> root.ref.toString() + is Rex.Op.Var.Global -> root.ref.toString() + else -> error("This isn't supported.") + } + val steps = steps.map { + when (val type = it.type) { + is Rel.Op.Exclude.Type.CollIndex -> "[${type.index}]" + is Rel.Op.Exclude.Type.CollWildcard -> "[*]" + is Rel.Op.Exclude.Type.StructSymbol -> ".${type.symbol}" + is Rel.Op.Exclude.Type.StructKey -> ".\"${type.key}\"" + is Rel.Op.Exclude.Type.StructWildcard -> ".*" + } + } + return root + steps.joinToString(separator = "") + } + + private fun Identifier.toProblemString(): String { + return this.joinToString("") { it.toProblemString() } + } + + private fun Identifier.Part.toProblemString(): String { + return when (this.isRegular()) { + false -> "\"${getText()}\"" + true -> getText() + } + } +} 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 ff6d4144fd..5bea594ad8 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 @@ -454,8 +454,6 @@ internal class PlanTyper(private val env: Env, config: Context) { * - 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? @@ -477,8 +475,9 @@ internal class PlanTyper(private val env: Env, config: Context) { val input = visitRel(node.input, ctx) // apply exclusions to the input schema - val init = input.type.schema.map { it.copy() } - val schema = node.paths.fold((init)) { bindings, path -> excludeBindings(bindings, path) } + val initBindings = input.type.schema.map { it.copy() } + ExcludeUtils.checkForInvalidExcludePaths(initBindings, node.paths, _listener) + val schema = node.paths.fold((initBindings)) { bindings, item -> excludeBindings(bindings, item) } // rewrite val type = ctx!!.copy(schema = schema) 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 1fb9c1ab4d..f27210c8d5 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 @@ -2,6 +2,7 @@ package org.partiql.planner.internal.typer import org.partiql.planner.internal.ir.Rel import org.partiql.planner.internal.typer.PlanTyper.Companion.toCType +import org.partiql.spi.catalog.Identifier import org.partiql.types.PType /** @@ -110,3 +111,14 @@ internal fun CompilerType.excludeCollection(step: Rel.Op.Exclude.Step, lastStepO else -> throw IllegalStateException() } } + +/** + * Compare an identifier to a struct field; handling case-insensitive comparisons. + * + * @param other + * @return + */ +internal fun Identifier.Part.isEquivalentTo(other: String): Boolean = when (this.isRegular()) { + false -> this.getText() == other + true -> this.getText().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 5cf20ac191..8819dde476 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 @@ -65,7 +65,7 @@ internal class PlanTyperTestsPorted { val catalog: String = "pql", val catalogPath: List = emptyList(), val expected: CompilerType, - val warnings: PErrorListener? = null, + val warnings: ProblemHandler? = null, ) : TestCase() { constructor( @@ -74,7 +74,7 @@ internal class PlanTyperTestsPorted { catalog: String = "pql", catalogPath: List = emptyList(), expected: PType, - warnings: PErrorListener? = null, + warnings: ProblemHandler? = null, ) : this(name, key, null, catalog, catalogPath, expected.toCType(), warnings) constructor( @@ -83,7 +83,7 @@ internal class PlanTyperTestsPorted { catalog: String = "pql", catalogPath: List = emptyList(), expected: PType, - warnings: PErrorListener? = null, + warnings: ProblemHandler? = null, ) : this(name, null, query, catalog, catalogPath, expected.toCType(), warnings) // legacy shim! @@ -94,7 +94,7 @@ internal class PlanTyperTestsPorted { catalog: String = "pql", catalogPath: List = emptyList(), expected: StaticType, - warnings: PErrorListener? = null, + warnings: ProblemHandler? = null, ) : this(name, key, query, catalog, catalogPath, fromStaticType(expected).toCType(), warnings) override fun toString(): String { @@ -215,6 +215,19 @@ internal class PlanTyperTestsPorted { assertTrue(message) { problems.problems.any { errorsEqual(it, problem) } } } + private fun assertWarningExists(problem: PError) = ProblemHandler { problems, _ -> + val message = buildString { + appendLine("Expected problems to include: $problem") + appendLine("Received: [") + problems.problems.forEach { + append("\t") + appendLine(it) + } + appendLine("]") + } + assertTrue(message) { problems.warnings.any { errorsEqual(it, problem) } } + } + // TODO: We don't assert on the properties right now. private fun errorsEqual(lhs: PError, rhs: PError): Boolean { return lhs.code() == rhs.code() && lhs.code() == rhs.code() && lhs.severity == rhs.severity && lhs.location == rhs.location @@ -1844,9 +1857,8 @@ internal 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( @@ -1860,7 +1872,8 @@ internal class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertWarningExists(PErrors.invalidExcludePath("t.attr_does_not_exist")) ), // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning SuccessTestCase( @@ -1959,9 +1972,8 @@ internal 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( @@ -1991,11 +2003,11 @@ internal class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertWarningExists(PErrors.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( @@ -2025,11 +2037,11 @@ internal class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertWarningExists(PErrors.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( @@ -2051,11 +2063,11 @@ internal class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertWarningExists(PErrors.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( @@ -2077,11 +2089,11 @@ internal class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertWarningExists(PErrors.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( @@ -2103,7 +2115,8 @@ internal class PlanTyperTestsPorted { TupleConstraint.Ordered ) ) - ) + ), + warnings = assertWarningExists(PErrors.invalidExcludePath("t.b")) ), // EXCLUDE regression test (behavior subject to change pending RFC); could give error/warning ErrorTestCase( @@ -2155,35 +2168,157 @@ internal class PlanTyperTestsPorted { ) ) ), - // TODO: Actual is bag(struct(b: int4, [Open(value=false), UniqueAttrs(value=true), Ordered])) -// SuccessTestCase( -// name = "EXCLUDE using a catalog", -// catalog = CATALOG_B, -// key = key("exclude-36"), // SELECT * EXCLUDE t.c FROM b.b.b AS t; -// expected = BagType( -// elementType = StructType( -// fields = mapOf( -// "b" to StructType( -// fields = mapOf( -// "b" to StaticType.INT4 -// ), -// contentClosed = true, -// constraints = setOf( -// TupleConstraint.Open(false), -// TupleConstraint.UniqueAttrs(true), -// TupleConstraint.Ordered -// ) -// ), -// ), -// contentClosed = true, -// constraints = setOf( -// TupleConstraint.Open(false), -// TupleConstraint.UniqueAttrs(true), -// TupleConstraint.Ordered -// ) -// ) -// ) -// ), + SuccessTestCase( + name = "EXCLUDE using a catalog", + catalog = CATALOG_B, + key = key("exclude-36"), + expected = BagType( + elementType = StructType( + fields = mapOf( + "b" to StructType( + fields = mapOf( + "b" to StaticType.INT4 + ), + contentClosed = true, + constraints = setOf( + TupleConstraint.Open(false), + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ), + ), + contentClosed = true, + constraints = setOf( + TupleConstraint.Open(false), + TupleConstraint.UniqueAttrs(true), + TupleConstraint.Ordered + ) + ) + ) + ), + 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 = assertWarningExists(PErrors.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 = assertWarningExists(PErrors.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 @@ -3842,7 +3977,7 @@ internal class PlanTyperTestsPorted { val plan = infer(input, session, collector) when (val statement = plan.action) { is Action.Query -> { - assert(collector.problems.isEmpty()) { + assert(collector.errors.isEmpty()) { // Throw internal error for debugging collector.problems.firstOrNull { it.code() == PError.INTERNAL_ERROR }?.let { pError -> pError.getOrNull("CAUSE", Throwable::class.java)?.let { throw it } @@ -3863,6 +3998,11 @@ internal class PlanTyperTestsPorted { PlanPrinter.append(this, plan) } } + val warnings = collector.warnings + if (warnings.isNotEmpty()) { + assert(tc.warnings != null) { "Expected no warnings but warnings were found: $warnings" } + tc.warnings?.handle(collector, 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 0000000000..cace9c81cd --- /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 a6742e73c3..30a65068a4 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; diff --git a/partiql-spi/api/partiql-spi.api b/partiql-spi/api/partiql-spi.api index 95e23cf088..36b9d94ade 100644 --- a/partiql-spi/api/partiql-spi.api +++ b/partiql-spi/api/partiql-spi.api @@ -315,6 +315,7 @@ public final class org/partiql/spi/errors/PError : org/partiql/spi/Enum { public static final field FUNCTION_NOT_FOUND I public static final field FUNCTION_TYPE_MISMATCH I public static final field INTERNAL_ERROR I + public static final field INVALID_EXCLUDE_PATH I public static final field PATH_INDEX_NEVER_SUCCEEDS I public static final field PATH_KEY_NEVER_SUCCEEDS I public static final field PATH_SYMBOL_NEVER_SUCCEEDS I diff --git a/partiql-spi/src/main/java/org/partiql/spi/errors/PError.java b/partiql-spi/src/main/java/org/partiql/spi/errors/PError.java index 6b529602c0..500f441304 100644 --- a/partiql-spi/src/main/java/org/partiql/spi/errors/PError.java +++ b/partiql-spi/src/main/java/org/partiql/spi/errors/PError.java @@ -416,4 +416,17 @@ public static PError INTERNAL_ERROR(@NotNull PErrorKind kind, @Nullable SourceLo * Example error message: [location]: Expression always returns missing. */ public static final int ALWAYS_MISSING = 14; + + /** + * This is a semantic warning, where an exclude path is statically known to + * not exist. For example: {@code SELECT * EXCLUDE some_struct."k" FROM << { 'a': 1 } >> AS some_struct } + *

+ * Potentially available properties: + *
    + *
  • PATH ({@link String}): A string representing the invalid path.
    • + *
+ *
+ * Example error message: [location]: Expression always returns null. + */ + public static final int INVALID_EXCLUDE_PATH = 15; } diff --git a/partiql-spi/src/test/kotlin/org/partiql/spi/errors/PErrorCodeTests.kt b/partiql-spi/src/test/kotlin/org/partiql/spi/errors/PErrorCodeTests.kt index f500680f4b..e7714cee48 100644 --- a/partiql-spi/src/test/kotlin/org/partiql/spi/errors/PErrorCodeTests.kt +++ b/partiql-spi/src/test/kotlin/org/partiql/spi/errors/PErrorCodeTests.kt @@ -31,6 +31,7 @@ class PErrorCodeTests { "VAR_REF_AMBIGUOUS" to 12, "TYPE_UNEXPECTED" to 13, "ALWAYS_MISSING" to 14, + "INVALID_EXCLUDE_PATH" to 15, ) // Preparation From fca3f4496b1375a859683f9590681e4f2951f080 Mon Sep 17 00:00:00 2001 From: John Ed Quinn Date: Wed, 8 Jan 2025 18:41:03 -0800 Subject: [PATCH 2/3] Updates to fix tests --- .../planner/internal/typer/ExcludeUtils.kt | 34 ++--- .../planner/internal/typer/PlanTyper.kt | 10 +- .../planner/internal/typer/TypeUtils.kt | 12 -- .../planner/PlannerPErrorReportingTests.kt | 4 +- .../internal/typer/PlanTyperTestsPorted.kt | 136 +++++++----------- 5 files changed, 82 insertions(+), 114 deletions(-) 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 index 9710aa11a3..e77e60eeea 100644 --- 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 @@ -55,17 +55,18 @@ internal object ExcludeUtils { if (steps.isEmpty()) { return true } - val step = steps.first() - return fields.any { field -> - when (val type = step.type) { - is Rel.Op.Exclude.Type.StructSymbol -> { - Identifier.regular(type.symbol).first().matches(field.name) && field.type.checkExclude(steps.drop(1)) - } - is Rel.Op.Exclude.Type.StructKey -> { - type.key == field.name && field.type.checkExclude(steps.drop(1)) + return steps.all { step -> + fields.any { field -> + when (val type = step.type) { + is Rel.Op.Exclude.Type.StructSymbol -> { + Identifier.regular(type.symbol).first().matches(field.name) && field.type.checkExclude(step.substeps) + } + is Rel.Op.Exclude.Type.StructKey -> { + type.key == field.name && field.type.checkExclude(step.substeps) + } + is Rel.Op.Exclude.Type.StructWildcard -> field.type.checkExclude(step.substeps) + else -> false } - is Rel.Op.Exclude.Type.StructWildcard -> field.type.checkExclude(steps.drop(1)) - else -> false } } } @@ -77,13 +78,14 @@ internal object ExcludeUtils { if (steps.isEmpty()) { return true } - val first = steps.first().type - return when (first) { - is Rel.Op.Exclude.Type.CollIndex, is Rel.Op.Exclude.Type.CollWildcard -> { - val e = this.typeParameter - e.checkExclude(steps.drop(1)) + return steps.all { step -> + when (step.type) { + is Rel.Op.Exclude.Type.CollIndex, is Rel.Op.Exclude.Type.CollWildcard -> { + val e = this.typeParameter + e.checkExclude(step.substeps) + } + else -> false } - else -> false } } 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 5bea594ad8..be1eb012d4 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 @@ -1277,7 +1277,15 @@ internal class PlanTyper(private val env: Env, config: Context) { when (val root = item.root) { is Rex.Op.Var.Unresolved -> { when (root.identifier.hasQualifier()) { - true -> it + true -> { + if (root.identifier.first().matches(it.name)) { + // recompute the StaticType of this binding after apply the exclusions + val type = it.type.exclude(item.steps, false) + it.copy(type = type) + } else { + it + } + } else -> { if (root.identifier.matches(it.name)) { // recompute the PType of this binding after applying the exclusions 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 f27210c8d5..1fb9c1ab4d 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 @@ -2,7 +2,6 @@ package org.partiql.planner.internal.typer import org.partiql.planner.internal.ir.Rel import org.partiql.planner.internal.typer.PlanTyper.Companion.toCType -import org.partiql.spi.catalog.Identifier import org.partiql.types.PType /** @@ -111,14 +110,3 @@ internal fun CompilerType.excludeCollection(step: Rel.Op.Exclude.Step, lastStepO else -> throw IllegalStateException() } } - -/** - * Compare an identifier to a struct field; handling case-insensitive comparisons. - * - * @param other - * @return - */ -internal fun Identifier.Part.isEquivalentTo(other: String): Boolean = when (this.isRegular()) { - false -> this.getText() == other - true -> this.getText().equals(other, ignoreCase = true) -} diff --git a/partiql-planner/src/test/kotlin/org/partiql/planner/PlannerPErrorReportingTests.kt b/partiql-planner/src/test/kotlin/org/partiql/planner/PlannerPErrorReportingTests.kt index 7dc4c9c431..cd36afe20c 100644 --- a/partiql-planner/src/test/kotlin/org/partiql/planner/PlannerPErrorReportingTests.kt +++ b/partiql-planner/src/test/kotlin/org/partiql/planner/PlannerPErrorReportingTests.kt @@ -355,7 +355,7 @@ internal class PlannerPErrorReportingTests { FROM struct_no_missing as t """.trimIndent(), false, - assertOnProblemCount(1, 0), + assertOnProblemCount(2, 0), BagType(closedStruct(StructType.Field("f1", StaticType.INT2))) ), TestCase( @@ -365,7 +365,7 @@ internal class PlannerPErrorReportingTests { FROM struct_no_missing as t """.trimIndent(), true, - assertOnProblemCount(0, 1), + assertOnProblemCount(1, 1), BagType(closedStruct(StructType.Field("f1", StaticType.INT2))) ), // TestCase( 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 8819dde476..8b42ee5e67 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 @@ -2168,34 +2168,35 @@ internal class PlanTyperTestsPorted { ) ) ), - SuccessTestCase( - name = "EXCLUDE using a catalog", - catalog = CATALOG_B, - key = key("exclude-36"), - expected = BagType( - elementType = StructType( - fields = mapOf( - "b" to StructType( - fields = mapOf( - "b" to StaticType.INT4 - ), - contentClosed = true, - constraints = setOf( - TupleConstraint.Open(false), - TupleConstraint.UniqueAttrs(true), - TupleConstraint.Ordered - ) - ), - ), - contentClosed = true, - constraints = setOf( - TupleConstraint.Open(false), - TupleConstraint.UniqueAttrs(true), - TupleConstraint.Ordered - ) - ) - ) - ), + // TODO: Actual is bag(struct(b: int4, [Open(value=false), UniqueAttrs(value=true), Ordered])) +// SuccessTestCase( +// name = "EXCLUDE using a catalog", +// catalog = CATALOG_B, +// key = key("exclude-36"), // SELECT * EXCLUDE t.c FROM b.b.b AS t; +// expected = BagType( +// elementType = StructType( +// fields = mapOf( +// "b" to StructType( +// fields = mapOf( +// "b" to StaticType.INT4 +// ), +// contentClosed = true, +// constraints = setOf( +// TupleConstraint.Open(false), +// TupleConstraint.UniqueAttrs(true), +// TupleConstraint.Ordered +// ) +// ), +// ), +// contentClosed = true, +// constraints = setOf( +// TupleConstraint.Open(false), +// TupleConstraint.UniqueAttrs(true), +// TupleConstraint.Ordered +// ) +// ) +// ) +// ), SuccessTestCase( name = "EXCLUDE with case-sensitive tuple reference not matching - warning", key = key("exclude-37"), @@ -2264,61 +2265,6 @@ internal class PlanTyperTestsPorted { ), warnings = assertWarningExists(PErrors.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 @@ -3795,6 +3741,30 @@ internal class PlanTyperTestsPorted { runTest(tc) } + @Test + @Disabled("In August 2024, the table lookup logic changed. This resolves using the current namespace, causing this to fail. This should be looked at. See https://github.com/partiql/partiql-lang-kotlin/commit/7aeb1bea0ee2599cc4a95c6d6fa067e4a7c0028c#diff-b5c8e5a6d813b88ee2a4d21451f116aa90d57bb04330ea7a36813474eafefb66") + fun excludeWithShadowedGlobalName() { + val tc = SuccessTestCase( + name = "EXCLUDE with an open struct - no warning or error", + catalog = CATALOG_B, + key = key("exclude-39"), + expected = PType.bag(PType.dynamic()) + ) + runTest(tc) + } + + @Test + @Disabled("In August 2024, the table lookup logic changed. This resolves using the current namespace, causing this to fail. This should be looked at. See https://github.com/partiql/partiql-lang-kotlin/commit/7aeb1bea0ee2599cc4a95c6d6fa067e4a7c0028c#diff-b5c8e5a6d813b88ee2a4d21451f116aa90d57bb04330ea7a36813474eafefb66.") + fun excludeWithShadowedGlobalName2() { + val tc = 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 = PType.bag(PType.dynamic()) + ) + runTest(tc) + } + @Test fun developmentTest3() { val tc = From 713259fa682dbf0fc6360a523360434bd2a213c8 Mon Sep 17 00:00:00 2001 From: John Ed Quinn Date: Wed, 8 Jan 2025 18:56:33 -0800 Subject: [PATCH 3/3] Rebases changes --- .../kotlin/org/partiql/planner/internal/typer/ExcludeUtils.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index e77e60eeea..90c0b8dec0 100644 --- 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 @@ -113,7 +113,7 @@ internal object ExcludeUtils { return this.joinToString("") { it.toProblemString() } } - private fun Identifier.Part.toProblemString(): String { + private fun Identifier.Simple.toProblemString(): String { return when (this.isRegular()) { false -> "\"${getText()}\"" true -> getText()