Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ports v0.14.9 changes #1702

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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))
Comment on lines +1136 to +1145
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we can leave out the CHANGELOG changes from the cherry-pick?

[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,122 @@
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
}
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
}
}
}
}

/**
* 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
}
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
}
}
}

// `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.Simple.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 Expand Up @@ -1278,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
Loading
Loading