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 partiql-eval APIs to Java and split compile phase #1618

Open
wants to merge 9 commits into
base: v1
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
15 changes: 3 additions & 12 deletions partiql-cli/src/main/kotlin/org/partiql/cli/Main.kt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import com.amazon.ionelement.api.loadAllElements
import org.partiql.cli.io.Format
import org.partiql.cli.pipeline.Pipeline
import org.partiql.cli.shell.Shell
import org.partiql.eval.PartiQLResult
import org.partiql.plugins.memory.MemoryCatalog
import org.partiql.plugins.memory.MemoryTable
import org.partiql.spi.catalog.Catalog
Expand Down Expand Up @@ -171,17 +170,9 @@ internal class MainCommand : Runnable {

// TODO add format support
checkFormat(format)

when (result) {
is PartiQLResult.Error -> {
error(result.cause.stackTrace)
}
is PartiQLResult.Value -> {
val writer = PartiQLValueTextWriter(System.out)
writer.append(result.value.toPartiQLValue()) // TODO: Create a Datum writer
println()
}
}
val writer = PartiQLValueTextWriter(System.out)
writer.append(result.toPartiQLValue()) // TODO: Create a Datum writer
println()
}

private fun session() = Session.builder()
Expand Down
19 changes: 10 additions & 9 deletions partiql-cli/src/main/kotlin/org/partiql/cli/pipeline/Pipeline.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,24 @@ import org.partiql.ast.Statement
import org.partiql.errors.Problem
import org.partiql.errors.ProblemCallback
import org.partiql.errors.ProblemSeverity
import org.partiql.eval.PartiQLEngine
import org.partiql.eval.PartiQLResult
import org.partiql.eval.Mode
import org.partiql.eval.PartiQLEvaluator
import org.partiql.parser.PartiQLParser
import org.partiql.plan.Plan
import org.partiql.planner.PartiQLPlanner
import org.partiql.spi.catalog.Session
import org.partiql.spi.value.Datum

internal class Pipeline private constructor(
private val parser: PartiQLParser,
private val planner: PartiQLPlanner,
private val engine: PartiQLEngine,
private val evaluator: PartiQLEvaluator,
) {

/**
* TODO replace with the ResultSet equivalent?
*/
fun execute(statement: String, session: Session): PartiQLResult {
fun execute(statement: String, session: Session): Datum {
val ast = parse(statement)
val plan = plan(ast, session)
return execute(plan, session)
Expand All @@ -41,7 +42,7 @@ internal class Pipeline private constructor(
TODO("Add V1 planner to the CLI")
}

private fun execute(plan: Plan, session: Session): PartiQLResult {
private fun execute(plan: Plan, session: Session): Datum {
// val statement = engine.prepare(plan, session.mode, session.planner())
// return engine.execute(statement)
TODO("Add V1 planner to the CLI")
Expand All @@ -61,15 +62,15 @@ internal class Pipeline private constructor(
fun default(): Pipeline {
val parser = PartiQLParser.standard()
val planner = PartiQLPlanner.standard()
val engine = PartiQLEngine.standard()
return Pipeline(parser, planner, engine)
val evaluator = PartiQLEvaluator.standard(Mode.PERMISSIVE())
return Pipeline(parser, planner, evaluator)
}

fun strict(): Pipeline {
val parser = PartiQLParser.standard()
val planner = PartiQLPlanner.builder().signal().build()
val engine = PartiQLEngine.standard()
return Pipeline(parser, planner, engine)
val evaluator = PartiQLEvaluator.standard()
return Pipeline(parser, planner, evaluator)
}
}
}
16 changes: 5 additions & 11 deletions partiql-cli/src/main/kotlin/org/partiql/cli/shell/Shell.kt
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import org.jline.utils.AttributedStyle.BOLD
import org.jline.utils.InfoCmp
import org.joda.time.Duration
import org.partiql.cli.pipeline.Pipeline
import org.partiql.eval.PartiQLResult
import org.partiql.spi.catalog.Session
import org.partiql.value.PartiQLValueExperimental
import org.partiql.value.io.PartiQLValueTextWriter
Expand Down Expand Up @@ -264,16 +263,11 @@ internal class Shell(
}
} else {
val result = pipeline.execute(line, session)
when (result) {
is PartiQLResult.Error -> throw result.cause
is PartiQLResult.Value -> {
val writer = PartiQLValueTextWriter(out)
writer.append(result.value.toPartiQLValue()) // TODO: Create a Datum writer
out.appendLine()
out.appendLine()
out.info("OK!")
}
}
val writer = PartiQLValueTextWriter(out)
writer.append(result.toPartiQLValue()) // TODO: Create a Datum writer
out.appendLine()
out.appendLine()
out.info("OK!")
}
}
} catch (ex: Exception) {
Expand Down
66 changes: 24 additions & 42 deletions partiql-eval/api/partiql-eval.api
Original file line number Diff line number Diff line change
@@ -1,54 +1,36 @@
public abstract interface class org/partiql/eval/PartiQLEngine {
public static final field Companion Lorg/partiql/eval/PartiQLEngine$Companion;
public static fun builder ()Lorg/partiql/eval/builder/PartiQLEngineBuilder;
public abstract fun prepare (Lorg/partiql/plan/Plan;Lorg/partiql/eval/PartiQLEngine$Mode;Lorg/partiql/spi/catalog/Session;)Lorg/partiql/eval/PartiQLStatement;
public static fun standard ()Lorg/partiql/eval/PartiQLEngine;
public class org/partiql/eval/Mode {
public static final field PERMISSIVE I
public static final field STRICT I
public static fun PERMISSIVE ()Lorg/partiql/eval/Mode;
public static fun STRICT ()Lorg/partiql/eval/Mode;
public fun code ()I
}

public final class org/partiql/eval/PartiQLEngine$Companion {
public final fun builder ()Lorg/partiql/eval/builder/PartiQLEngineBuilder;
public final fun standard ()Lorg/partiql/eval/PartiQLEngine;
public abstract interface class org/partiql/eval/PartiQLCompiler {
public static fun builder ()Lorg/partiql/eval/PartiQLCompiler$Builder;
public abstract fun prepare (Lorg/partiql/plan/Plan;)Lorg/partiql/eval/Statement;
public static fun standard ()Lorg/partiql/eval/PartiQLCompiler;
public static fun standard (Lorg/partiql/eval/Mode;)Lorg/partiql/eval/PartiQLCompiler;
}

public final class org/partiql/eval/PartiQLEngine$Mode : java/lang/Enum {
public static final field PERMISSIVE Lorg/partiql/eval/PartiQLEngine$Mode;
public static final field STRICT Lorg/partiql/eval/PartiQLEngine$Mode;
public static fun getEntries ()Lkotlin/enums/EnumEntries;
public static fun valueOf (Ljava/lang/String;)Lorg/partiql/eval/PartiQLEngine$Mode;
public static fun values ()[Lorg/partiql/eval/PartiQLEngine$Mode;
public class org/partiql/eval/PartiQLCompiler$Builder {
public fun build ()Lorg/partiql/eval/PartiQLCompiler;
public fun mode (Lorg/partiql/eval/Mode;)Lorg/partiql/eval/PartiQLCompiler$Builder;
}

public abstract interface class org/partiql/eval/PartiQLResult {
public abstract interface class org/partiql/eval/PartiQLEvaluator {
public static fun builder ()Lorg/partiql/eval/PartiQLEvaluator$Builder;
public abstract fun eval (Lorg/partiql/plan/Plan;)Lorg/partiql/spi/value/Datum;
public static fun standard ()Lorg/partiql/eval/PartiQLEvaluator;
public static fun standard (Lorg/partiql/eval/Mode;)Lorg/partiql/eval/PartiQLEvaluator;
}

public final class org/partiql/eval/PartiQLResult$Error : org/partiql/eval/PartiQLResult {
public fun <init> (Ljava/lang/Throwable;)V
public final fun component1 ()Ljava/lang/Throwable;
public final fun copy (Ljava/lang/Throwable;)Lorg/partiql/eval/PartiQLResult$Error;
public static synthetic fun copy$default (Lorg/partiql/eval/PartiQLResult$Error;Ljava/lang/Throwable;ILjava/lang/Object;)Lorg/partiql/eval/PartiQLResult$Error;
public fun equals (Ljava/lang/Object;)Z
public final fun getCause ()Ljava/lang/Throwable;
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
public class org/partiql/eval/PartiQLEvaluator$Builder {
public fun build ()Lorg/partiql/eval/PartiQLEvaluator;
public fun compiler (Lorg/partiql/eval/PartiQLCompiler;)Lorg/partiql/eval/PartiQLEvaluator$Builder;
}

public final class org/partiql/eval/PartiQLResult$Value : org/partiql/eval/PartiQLResult {
public fun <init> (Lorg/partiql/spi/value/Datum;)V
public final fun component1 ()Lorg/partiql/spi/value/Datum;
public final fun copy (Lorg/partiql/spi/value/Datum;)Lorg/partiql/eval/PartiQLResult$Value;
public static synthetic fun copy$default (Lorg/partiql/eval/PartiQLResult$Value;Lorg/partiql/spi/value/Datum;ILjava/lang/Object;)Lorg/partiql/eval/PartiQLResult$Value;
public fun equals (Ljava/lang/Object;)Z
public final fun getValue ()Lorg/partiql/spi/value/Datum;
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

public abstract interface class org/partiql/eval/PartiQLStatement {
public abstract fun execute (Lorg/partiql/spi/catalog/Session;)Lorg/partiql/eval/PartiQLResult;
}

public final class org/partiql/eval/builder/PartiQLEngineBuilder {
public fun <init> ()V
public final fun build ()Lorg/partiql/eval/PartiQLEngine;
public abstract interface class org/partiql/eval/Statement {
public abstract fun execute ()Lorg/partiql/spi/value/Datum;
}

38 changes: 38 additions & 0 deletions partiql-eval/src/main/java/org/partiql/eval/Mode.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package org.partiql.eval;

/**
* PartiQL Execution Mode.
*/
public class Mode {

/**
* Strict execution mode.
*/
public static final int STRICT = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Nice! We've been leaving a spot for UNKNOWN = 0 which may help with protecting against forward compatibility. You don't need to add it here, but you could make STRICT = 1 and PERMISSIVE = 2.


/**
* Permissive execution mode.
*/
public static final int PERMISSIVE = 1;

/**
* Internal enum code.
*/
private final int code;

private Mode(int code) {
this.code = code;
}

public int code() {
return this.code;
}

public static Mode STRICT() {
return new Mode(STRICT);
}

public static Mode PERMISSIVE() {
return new Mode(PERMISSIVE);
}
}
73 changes: 73 additions & 0 deletions partiql-eval/src/main/java/org/partiql/eval/PartiQLCompiler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package org.partiql.eval;

import org.jetbrains.annotations.NotNull;
import org.partiql.plan.Plan;

/**
* A [PartiQLCompiler] is responsible for produces physical plans from logical plans.
*/
public interface PartiQLCompiler {

/**
* Compiles the given plan into an executable PartiQL statement.
*
* @param plan The plan to compile.
* @return The prepared statement.
*/
@NotNull
public Statement prepare(@NotNull Plan plan);

/**
* @return A new [PartiQLCompilerBuilder].
*/
@NotNull
public static Builder builder() {
return new Builder();
}

/**
* @return A new [PartiQLCompiler] with the default (strict) compilation mode.
*/
@NotNull
public static PartiQLCompiler standard() {
return standard(Mode.STRICT());
}

/**
* @param mode The compilation mode.
* @return A new [PartiQLCompiler] with the given compilation mode.
*/
@NotNull
public static PartiQLCompiler standard(Mode mode) {
return new StandardCompiler(mode);
}

/**
* Builder class for the [PartiQLCompiler] interface.
*/
public static class Builder {

// builder state
private Mode mode;

private Builder() {
// empty
}

/**
* @param mode The mode.
* @return This builder.
*/
public Builder mode(Mode mode) {
this.mode = mode;
return this;
}

/**
* @return A new [PartiQLCompiler].
*/
public PartiQLCompiler build() {
return new StandardCompiler(mode);
}
}
}
68 changes: 68 additions & 0 deletions partiql-eval/src/main/java/org/partiql/eval/PartiQLEvaluator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package org.partiql.eval;

import org.jetbrains.annotations.NotNull;
import org.partiql.plan.Plan;
import org.partiql.spi.value.Datum;

/**
* TODO JAVADOC
*/
public interface PartiQLEvaluator {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced we need this -- since the StandardEvaluator implementation is a single line.

This is a "core" package. I'm of the opinion that some other package could provide easier-to-use APIs. If this is the intention, maybe in partiql-lang, we expose an API that takes in a String query and returns the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The standard may be a single line, but consider introducing a plan cache. It's not necessarily about an easier API, but about splitting the responsibility of the preparation phase from invoking.

The compiler returns a function, the evaluator invokes that function. I could put these both into PartiQLEvaluator, but then we'd have to extend the "evaluator" with compilation strategies for choosing physical operators.

So it's really not about a "easier API" but a responsibility split, where the default custom need not interact with the compilation phase.

Copy link
Member

Choose a reason for hiding this comment

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

It's not necessarily about an easier API, but about splitting the responsibility of the preparation phase from invoking... The compiler returns a function, the evaluator invokes that function.

In this implementation, the PartiQLEvaluator doesn't invoke the function returned by the PartiQLCompiler. The responsibilities here overlap. The PartiQLEvaluator here compiles the plan and executes. The PartiQLCompiler here compiles the plan and does not execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler returns a compiled "function" aka a prepared statement to be invoked via "execute". The PartiQLEvaluator does call the "function/statement" returned by the compiler.

The responsibility of the evaluator indeed overlaps the compiler by design. The split exists so that customers can supply their own compilation phase, but don't need to.

The idea here was to make an entry-point for custom compilation, but independent of the evaluator. Do you have some ideas on how to best split these responsibilities?

We could also make evaluator drive the datum for customers who don't want the lazy behavior?


/**
* @param plan The plan to evaluate.
* @return The result of evaluating the plan.
*/
@NotNull
public Datum eval(@NotNull Plan plan);

/**
* @return A new [PartiQLCompilerBuilder].
*/
@NotNull
public static Builder builder() {
return new Builder();
}

/**
* @return A new [PartiQLEvaluator] with the default (strict) evaluation mode.
*/
@NotNull
public static PartiQLEvaluator standard() {
return standard(Mode.STRICT());
}

/**
* @param mode The evaluation mode.
* @return A new [PartiQLEvaluator] with the given evaluation mode.
*/
@NotNull
public static PartiQLEvaluator standard(Mode mode) {
return new StandardEvaluator(PartiQLCompiler.standard(mode));
}

/**
* Builder class for the [PartiQLCompiler] interface.
*/
public static class Builder {

// builder state
private PartiQLCompiler compiler = PartiQLCompiler.standard();

private Builder() {
// empty
}

public Builder compiler(PartiQLCompiler compiler) {
this.compiler = compiler;
return this;
}

/**
* @return A new [PartiQLCompiler].
*/
public PartiQLEvaluator build() {
return new StandardEvaluator(compiler);
}
}
}
20 changes: 20 additions & 0 deletions partiql-eval/src/main/java/org/partiql/eval/Statement.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.partiql.eval;

import org.jetbrains.annotations.NotNull;
import org.partiql.spi.value.Datum;

/**
* An executable statement.
* <br>
* Developer Note: Consider `Datum execute(Parameters parameters)` for DML.
*/
public interface Statement {

/**
* Executes the statement with no parameters.
*
* @return Datum execution result.
*/
@NotNull
public Datum execute();
}
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I like some of the modeling exposed by JDBC. They have a sort of fat interface, but they don't expose a type -- which would actually prove quite useful for customers.

Suggested change
public interface Statement {
/**
* Executes the statement with no parameters.
*
* @return Datum execution result.
*/
@NotNull
public Datum execute();
}
public interface PreparedStatement {
/**
* The type of the executable statement.
*/
int type;
/**
* If a prepared statement is of this type, [executeQuery] may be invoked.
*/
public static final int QUERY = 1;
/**
* Executes the query.
*
* @return Datum execution result.
*/
@NotNull
public Datum executeQuery();
/**
* Executes an UPDATE.
*
* @return UpdateDetails the details of the update
*/
@NotNull
public UpdateDetails executeUpdate();
}

Also, I don't think we need these comments regarding parameters. We've discussed this, but you can't plan without the parameters already injected, the parameters are (likely) a JDBC library feature (before sending the request to the server) and are outside of the compiler/engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but you can't plan without the parameters already injected

You can, you just need the number of parameters and optionally the types (aka the formal arguments of a function).

(likely) a JDBC library feature

Not a JDBC thing because these parameters aren't stringified. Stringified params makes sense for JDBC because it supports multiple backends, but I think my other answers show how this adds formal arguments to a compiled statement.

ACK on the factoring of prepared statement being fat-interface like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was looking into the JDBC prepared statement. We can't have an uninitialized field like in your code sample, but they may be something more interesting here. The use of executeQuery() vs executeUpdate() have to do with differing function return types; the former returns a ResultSet and the latter an int. There's also executeLargeUpdate() which returns a long. It also reads like the "executeUpdate" was added first, customers realized the limitation/bug, and the executeLargeUpdate() had to be added because a function cannot be defined with two different return types. This seems to indicate that the PreparedStatement methods don't necessarily correspond to the underlying SQL statement type, but are more about the return type.

We have two options here:

  1. Datum can represent both a ResultSet and an int, long, etc; so no need to have different execution return types.
  2. We introduce generics so we have Statement<T> with T execute().

I'm working on the mental model of "A PartiQL compiler returns a statement which can be invoked with zero or more parameters". It produces functions from the input environment and input string, then you can invoke that function.

Interestingly the JDBC statement has no notion of a statement type. I'm not against fat interfaces, but we have to keep in mind that they were a solution before generics existed and are advised against in OO design.

Loading
Loading