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

Updates catalog definitions and tests to fix assemble. #1514

Closed
wants to merge 15 commits into from
Closed
23 changes: 9 additions & 14 deletions partiql-planner/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,13 @@ plugins {
}

dependencies {
api(project(":partiql-ast"))
api(project(":partiql-plan"))
api(project(":partiql-types"))
implementation(project(":partiql-ast"))
implementation(Deps.dotlin)
implementation(Deps.ionElement)
// Test
testImplementation(project(":partiql-parser"))
testImplementation(project(":plugins:partiql-local"))
testImplementation(project(":plugins:partiql-memory"))
// Test Fixtures
testFixturesImplementation(project(":partiql-spi"))
Comment on lines -34 to -37
Copy link
Member Author

Choose a reason for hiding this comment

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

partiql-planner no long depends on spi which means we cannot use any of the plugins.

}

tasks.register("generateResourcePath") {
Expand Down Expand Up @@ -93,7 +89,6 @@ tasks.register<Exec>("codegen") {
"--poems", "builder",
"--poems", "util",
"--opt-in", "org.partiql.value.PartiQLValueExperimental",
"--opt-in", "org.partiql.spi.fn.FnExperimental",
"./src/main/resources/partiql_plan_internal.ion"
)
}
Expand All @@ -112,14 +107,14 @@ tasks.register<Copy>("copyUtils") {
//
// !! IMPORTANT !! — only run manually, as this will overwrite the existing ir/Nodes.kt.
//
// tasks.register<Copy>("copyNodes") {
// includeEmptyDirs = false
// dependsOn("codegen")
// filter { it.replace(Regex("public (?!(override|(fun visit)))"), "internal ") }
// from("$buildDir/tmp")
// include("**/Nodes.kt")
// into("src/main/kotlin")
// }
tasks.register<Copy>("copyNodes") {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I think should remain commented out or possibly removed? Some custom logic in Nodes.kt is getting overwritten.

Copy link
Member

Choose a reason for hiding this comment

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

we can fix Nodes.kt after all the internal plan nodes are more finalized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was my intention as it is being changes frequently.

includeEmptyDirs = false
dependsOn("codegen")
filter { it.replace(Regex("public (?!(override|(fun visit)))"), "internal ") }
from("$buildDir/tmp")
include("**/Nodes.kt")
into("src/main/kotlin")
}

tasks.register("generate") {
dependsOn("codegen", "copyUtils")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import org.partiql.ast.Statement
import org.partiql.errors.Problem
import org.partiql.errors.ProblemCallback
import org.partiql.plan.PartiQLPlan
import org.partiql.spi.connector.ConnectorMetadata
import java.time.Instant
import org.partiql.planner.catalog.Catalogs
import org.partiql.planner.catalog.Session
import org.partiql.planner.internal.PartiQLPlannerDefault
import org.partiql.planner.internal.PlannerFlag

/**
* PartiQLPlanner is responsible for transforming an AST into PartiQL's logical query plan.
Expand All @@ -32,30 +34,44 @@ public interface PartiQLPlanner {
public val problems: List<Problem>,
)

/**
* From [org.partiql.lang.planner.transforms]
*
* @property queryId
* @property userId
* @property currentCatalog
* @property currentDirectory
* @property catalogs
* @property instant
*/
public class Session(
public val queryId: String,
public val userId: String,
public val currentCatalog: String,
public val currentDirectory: List<String> = emptyList(),
public val catalogs: Map<String, ConnectorMetadata> = emptyMap(),
public val instant: Instant = Instant.now(),
)
Comment on lines -45 to -52
Copy link
Member Author

Choose a reason for hiding this comment

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

Session is moved to an interface.

public companion object {

@JvmStatic
public fun builder(): PartiQLPlannerBuilder = PartiQLPlannerBuilder()
public fun builder(): Builder = Builder()
}

@JvmStatic
public fun default(): PartiQLPlanner = PartiQLPlannerBuilder().build()
public class Builder {
Copy link
Member Author

Choose a reason for hiding this comment

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

The recurring pattern is to namespace all builders under the interface class.


private val flags: MutableSet<PlannerFlag> = mutableSetOf()
private var catalogs: Catalogs? = null

/**
* Build the builder, return an implementation of a [PartiQLPlanner].
*
* @return
*/
public fun build(): PartiQLPlanner {
assert(catalogs != null) { "The `catalogs` field cannot be null, set with .catalogs(...)" }
return PartiQLPlannerDefault(catalogs!!, flags)
}

/**
* Adds a catalog provider to this planner builder.
*/
public fun catalogs(catalogs: Catalogs): Builder {
this.catalogs = catalogs
return this
}

/**
* Java style method for setting the planner to signal mode
*/
public fun signal(signal: Boolean = true): Builder = this.apply {
if (signal) {
this.flags.add(PlannerFlag.SIGNAL_MODE)
} else {
this.flags.remove(PlannerFlag.SIGNAL_MODE)
}
}
}
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.partiql.planner.catalog

import org.partiql.types.PType

/**
* Catalog interface for access to tables and routines.
*
Expand All @@ -16,23 +18,19 @@ public interface Catalog {

/**
* Get a table by name.
*
* @param name The case-sensitive [Table] name.
* @return The [Table] or null if not found.
*/
public fun getTable(session: Session, name: Name): Table? = null

/**
* Get a table by identifier; note that identifiers may be case-insensitive.
*/
public fun getTable(session: Session, identifier: Identifier): Table? = null
Copy link
Member Author

Choose a reason for hiding this comment

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

Consider getTable(session: Session, name: Name) and searchTable(session, identifier: Identifier) or something similar.

We have no way of knowing which is the catalog/namespace, the table name, and the path expression — so we must do searching.


/**
* List top-level tables.
*/
public fun listTables(session: Session): Collection<Name> = listTables(session, Namespace.root())

/**
* List all tables under this namespace.
*
* @param namespace
*/
public fun listTables(session: Session, namespace: Namespace): Collection<Name> = emptyList()

Expand All @@ -55,4 +53,97 @@ public interface Catalog {
* @return A collection of all [Routine]s in the current namespace with this name.
*/
public fun getRoutines(session: Session, name: Name): Collection<Routine> = emptyList()

/**
* Factory methods and builder.
*/
public companion object {

@JvmStatic
public fun builder(): Builder = Builder()
}

/**
* Java-style builder for a default [Catalog] implementation.
*
*/
public class Builder {

private var name: String? = null
private var tables = mutableMapOf<String, Table>()

public fun name(name: String): Builder {
this.name = name
return this
}

public fun createTable(name: String, schema: PType): Builder {
this.tables[name] = Table.of(name, schema)
return this
}

public fun createTable(name: Name, schema: PType): Builder {
if (name.hasNamespace()) {
error("Table name must not have a namespace: $name")
}
this.tables[name.getName()] = Table.of(name.getName(), schema)
return this
}

public fun build(): Catalog {

val name = this.name ?: throw IllegalArgumentException("Catalog name must be provided")

return object : Catalog {
Copy link
Member Author

Choose a reason for hiding this comment

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

Replacement for partiql-memory plugin


override fun getName(): String = name

override fun getTable(session: Session, name: Name): Table? {
if (name.hasNamespace()) {
return null
}
return tables[name.getName()]
}

override fun getTable(session: Session, identifier: Identifier): Table? {
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this just deleted from the interface?

if (identifier.hasQualifier()) {
return null
}
var match: Table? = null
val id = identifier.getIdentifier()
for (table in tables.values) {
if (id.matches(table.getName())) {
if (match == null) {
match = table
} else {
error("Ambiguous table name: $name")
}
}
}
return match
}

override fun listTables(session: Session): Collection<Name> {
return tables.values.map { Name.of(it.getName()) }
}

override fun listTables(session: Session, namespace: Namespace): Collection<Name> {
if (!namespace.isEmpty()) {
return emptyList()
}
return tables.values.map { Name.of(it.getName()) }
}

override fun listNamespaces(session: Session): Collection<Namespace> {
return emptyList()
}

override fun listNamespaces(session: Session, namespace: Namespace): Collection<Namespace> {
return emptyList()
}

override fun getRoutines(session: Session, name: Name): Collection<Routine> = emptyList()
}
}
}
}
Loading
Loading