Updates catalog definitions and tests to fix assemble. #1514
Conversation
RCHowell
force-pushed
the
v1-metadata-planner
branch
from
2aec92c to
ad93e3c
Compare
| testImplementation(project(":plugins:partiql-local")) | ||
| testImplementation(project(":plugins:partiql-memory")) | ||
| // Test Fixtures | ||
| testFixturesImplementation(project(":partiql-spi")) |
There was a problem hiding this comment.
partiql-planner no long depends on spi which means we cannot use any of the plugins.
|
|
||
| @JvmStatic | ||
| public fun default(): PartiQLPlanner = PartiQLPlannerBuilder().build() | ||
| public class Builder { |
There was a problem hiding this comment.
The recurring pattern is to namespace all builders under the interface class.
| 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(), | ||
| ) |
There was a problem hiding this comment.
Session is moved to an interface.
| 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 |
There was a problem hiding this comment.
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.
|
|
||
| val name = this.name ?: throw IllegalArgumentException("Catalog name must be provided") | ||
|
|
||
| return object : Catalog { |
There was a problem hiding this comment.
Replacement for partiql-memory plugin
| properties: Properties = DEFAULT_PROPERTIES, | ||
| ): Scalar = object : Scalar { | ||
| override fun getName(): String = name | ||
| override fun getParameters(): Array<Parameter> = parameters.toTypedArray() | ||
| override fun getReturnType(): PType.Kind = returnType | ||
| override fun getReturnType(): PType = returnType |
There was a problem hiding this comment.
For now use PType to minimize changes.
| val signature: FnSignature, | ||
| val signature: Routine, |
There was a problem hiding this comment.
You'll see Routine replaces FnSignature everywhere.
| return tables[name.getName()] | ||
| } | ||
|
|
||
| override fun getTable(session: Session, identifier: Identifier): Table? { |
There was a problem hiding this comment.
Wasn't this just deleted from the interface?
| @@ -13,9 +13,9 @@ public interface Catalogs { | |||
| /** | |||
| * Returns a catalog by name (single identifier). | |||
| */ | |||
| public fun get(name: String, ignoreCase: Boolean = false): Catalog? { | |||
| public fun get(identifier: Identifier): Catalog? { | |||
There was a problem hiding this comment.
Non-blocking as the code pre-exists: shouldn't you be using list() in this scenario?
| @@ -9,7 +12,7 @@ package org.partiql.planner.catalog | |||
| public class Identifier private constructor( | |||
| private val qualifier: Array<Part>, | |||
| private val identifier: Part, | |||
| ) { | |||
| ) : Iterable<Identifier.Part> { | |||
There was a problem hiding this comment.
While this PR doesn't address this, I'm only leaving it since I didn't review previous PRs. Routines in SQL are either functions or procedures (any SQL statement, not required to return a value). So, Function might be a better name -- and operator and scalar could be variants.
There was a problem hiding this comment.
I agree and can go back to Function. I was only avoiding it because most often function is taken to mean scalar function.
But I do like,
aggregation functionscalar functionoperator function
Where an operator function is an operator backed by a function a'la postgres.
| // include("**/Nodes.kt") | ||
| // into("src/main/kotlin") | ||
| // } | ||
| tasks.register<Copy>("copyNodes") { |
There was a problem hiding this comment.
Hmm I think should remain commented out or possibly removed? Some custom logic in Nodes.kt is getting overwritten.
There was a problem hiding this comment.
we can fix Nodes.kt after all the internal plan nodes are more finalized.
There was a problem hiding this comment.
Yes, this was my intention as it is being changes frequently.
Description
This PR stubs out all planner and tests so that I can begin implementing and executing planner tests.
CONTRIBUTING.md)
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.