Adds a built-in PartiQL System catalog, support for SQL-Path, and SPI cleanup #1670
Conversation
partiql-system/src/main/java/org/partiql/system/PartiQLSession.java
Outdated
Show resolved
Hide resolved
Removes partiql-system package Adds systemCatalog() method to Session.Builder
| @@ -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.
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,
Session.builder()....build() // standard builder comes with standard system
Session.system(catalog)...builder() // .system() is a named *builder* factory which replaces the standard system.
There was a problem hiding this comment.
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.
I just reverted and added the PartiQL system catalog to Session.empty(). See the last three commits. LMK what you think.
|
|
||
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
make singleton with private constructor and have a public static final String for "$system"
So it's like
protected final class System implements Catalog {
static final INSTANCE
static final NAME = "\$system"
}
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.
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.
Removes Catalog#empty() method Session#empty() no longer adds an empty catalog
johnedquinn
changed the title
Relevant Issues
Description
Catalog#getTable()andCatalog#resolveTable()License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.