-
Notifications
You must be signed in to change notification settings - Fork 62
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
Adds a built-in PartiQL System catalog, support for SQL-Path, and SPI cleanup #1670
Changes from 3 commits
896e295
11b1496
42cc72e
a712fba
c91977f
0573517
531659d
628c1db
d1b3305
57fb8b3
4110b4b
aa2a6f2
8d9ded8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
package org.partiql.spi.catalog; | ||
|
||
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
import org.partiql.spi.function.Aggregation; | ||
import org.partiql.spi.function.Builtins; | ||
import org.partiql.spi.function.Function; | ||
|
||
import java.util.Collection; | ||
|
||
/** | ||
* <p> | ||
* This implements the PartiQL System Catalog. | ||
* </p> | ||
* <p> | ||
* It provides the implementation for the PartiQL System Catalog, which is a built-in catalog | ||
* that provides access to the PartiQL language and its built-in functions and aggregations. | ||
* </p> | ||
* @see Session.Builder | ||
*/ | ||
final class PartiQLSystemCatalog implements Catalog { | ||
|
||
/** | ||
* TODO | ||
*/ | ||
@NotNull | ||
private final String name; | ||
|
||
/** | ||
* Creates a new PartiQL System Catalog with the given name. | ||
* @param name the name of the PartiQL System Catalog | ||
*/ | ||
PartiQLSystemCatalog(@NotNull String name) { | ||
this.name = name; | ||
} | ||
|
||
@NotNull | ||
@Override | ||
public String getName() { | ||
return this.name; | ||
} | ||
|
||
@Nullable | ||
@Override | ||
public Table getTable(@NotNull Session session, @NotNull Name name) { | ||
return null; | ||
} | ||
|
||
@Nullable | ||
@Override | ||
public Table getTable(@NotNull Session session, @NotNull Identifier identifier) { | ||
return null; | ||
} | ||
|
||
@NotNull | ||
@Override | ||
public Collection<Function> getFunctions(@NotNull Session session, @NotNull String name) { | ||
return Builtins.INSTANCE.getFunctions(name); | ||
} | ||
|
||
@NotNull | ||
@Override | ||
public Collection<Aggregation> getAggregations(@NotNull Session session, @NotNull String name) { | ||
return Builtins.INSTANCE.getAggregations(name); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,8 @@ public interface Session { | |
public companion object { | ||
|
||
/** | ||
* Returns a [Session] with only the "empty" catalog implementation. | ||
* Returns a [Session] with only the "empty" catalog implementation. Note that this does NOT add the | ||
* PartiQL System Catalog. | ||
*/ | ||
@JvmStatic | ||
public fun empty(): Session = object : Session { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Session.empty() is used in several places so would need the psystem available. I don't believe we need bare() factory method either. I was referring to a builder method in the previous PR, not a factory. Think about it this way,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the latest revision has this. I can just revert the last commit (the addition of the bare method). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just reverted and added the PartiQL system catalog to |
||
|
@@ -53,6 +54,19 @@ public interface Session { | |
override fun getNamespace(): Namespace = Namespace.empty() | ||
} | ||
|
||
/** | ||
* Returns a [Session] with only the PartiQL System Catalog. Therefore, this provides PartiQL's built-in functions | ||
* and aggregations. | ||
* @param systemCatalogName the name to assign the system catalog | ||
*/ | ||
@JvmStatic | ||
public fun bare(systemCatalogName: String): Session = object : Session { | ||
override fun getIdentity(): String = "unknown" | ||
override fun getCatalog(): String = systemCatalogName | ||
override fun getCatalogs(): Catalogs = Catalogs.builder().add(PartiQLSystemCatalog(systemCatalogName)).build() | ||
override fun getNamespace(): Namespace = Namespace.empty() | ||
} | ||
|
||
@JvmStatic | ||
public fun builder(): Builder = Builder() | ||
} | ||
|
@@ -64,6 +78,8 @@ public interface Session { | |
|
||
private var identity: String = "unknown" | ||
private var catalog: String? = null | ||
private var systemCatalogName: String = "\$pql_system" | ||
private var systemCatalog: Catalog = PartiQLSystemCatalog(systemCatalogName) | ||
johnedquinn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private var catalogs: Catalogs.Builder = Catalogs.builder() | ||
private var namespace: Namespace = Namespace.empty() | ||
private var properties: MutableMap<String, String> = mutableMapOf() | ||
|
@@ -98,6 +114,16 @@ public interface Session { | |
return this | ||
} | ||
|
||
/** | ||
* Adds and designates a catalog to always be on the SQL-Path. This [catalog] provides all built-in functions | ||
* to the system at hand. | ||
* If this is never invoked, a default system catalog is provided. | ||
*/ | ||
public fun systemCatalog(catalog: Catalog): Builder { | ||
this.systemCatalogName = catalog.getName() | ||
return this | ||
} | ||
|
||
/** | ||
* Adds catalogs to this session. | ||
*/ | ||
|
@@ -110,16 +136,24 @@ public interface Session { | |
|
||
public fun build(): Session = object : Session { | ||
|
||
private val _catalogs = catalogs.build() | ||
private val _catalogs: Catalogs | ||
private val systemCatalogNamespace: Namespace = Namespace.of(systemCatalogName) | ||
|
||
init { | ||
require(catalog != null) { "Session catalog must be set" } | ||
catalogs.add(systemCatalog) | ||
_catalogs = catalogs.build() | ||
} | ||
|
||
override fun getIdentity(): String = identity | ||
override fun getCatalog(): String = catalog!! | ||
override fun getCatalogs(): Catalogs = _catalogs | ||
override fun getNamespace(): Namespace = namespace | ||
|
||
override fun getPath(): Path { | ||
val currentNamespace = getNamespace() | ||
return Path.of(currentNamespace, systemCatalogNamespace) | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make singleton with private constructor and have a public static final String for "$system"
So it's like
like that. Also I know that protected is different than package-private, but do prefer an explicit modifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't make a protected top-level class. Updated the Javadocs to make the visibility more apparent, since there is no package-private modifier. And added the singleton. See latest commit: d1b3305
If we want to expose this class in the future, sure. But we don't need to right now.