Skip to content

Commit

Permalink
Port: Adds warnings for EXCLUDE paths that don't exclude any value (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
alancai98 authored and johnedquinn committed Jan 9, 2025
1 parent 6846cac commit f426c2c
Show file tree
Hide file tree
Showing 11 changed files with 427 additions and 57 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)
}
}
Original file line number Diff line number Diff line change
@@ -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<Rel.Binding>, paths: List<Rel.Op.Exclude.Path>, 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<Rel.Op.Exclude.Step>): 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<Rel.Op.Exclude.Step>): 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<Rel.Op.Exclude.Step>): 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()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand Down Expand Up @@ -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)
}
Loading

0 comments on commit f426c2c

Please sign in to comment.