feat: expose execution plans from the ksql engine API #3482
Conversation
| @@ -88,6 +92,34 @@ | |||
| */ | |||
| PreparedStatement<?> prepare(ParsedStatement stmt); | |||
|
|
|||
| /** | |||
| * Executes a query using the engine's primary service context. | |||
There was a problem hiding this comment.
Shouldn't this comment indicate that this is executing it using a supplied service context? Also, should there be another method for the primary service context?
TransientQueryMetadata executeQuery( ConfiguredStatement<Query> statement );
There was a problem hiding this comment.
Shouldn't this comment indicate that this is executing it using a supplied service context?
good catch
should there be another method for the primary service context?
I don't want to add it until there's something to use it.
rodesai
force-pushed
the
add-engine-interface-rebase
branch
2 times, most recently
from
858823e
to
28d810b
Compare
| /** | ||
| * Executes a query using the supplied service context. | ||
| */ | ||
| TransientQueryMetadata executeQuery( |
There was a problem hiding this comment.
nit:
| TransientQueryMetadata executeQuery( | |
| TransientQueryMetadata executeTransientQuery( |
Since this is explicitly returning a TransientQueryMetadata object, it'd be nice to have the function name indicate that. The function naming would be fine if the return type is changed to QueryMetadata.
ksql-engine/src/main/java/io/confluent/ksql/engine/KsqlEngine.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public ExecuteResult execute(final ConfiguredStatement<?> statement) { | ||
| return execute( | ||
| ConfiguredKsqlPlan.of(plan(statement), statement.getOverrides(), statement.getConfig())); |
There was a problem hiding this comment.
nit: could the creation of the ConfiguredKsqlPlan be formatted similar to the execute function below in terms of the indentations.
ksql-engine/src/main/java/io/confluent/ksql/engine/KsqlEngine.java
Outdated
Show resolved
Hide resolved
ksql-engine/src/main/java/io/confluent/ksql/engine/SandboxedExecutionContext.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public KsqlPlan plan( | ||
| final ServiceContext serviceContext, | ||
| final ConfiguredStatement<?> statement) { |
There was a problem hiding this comment.
nit:
) { should be on next line
| /** | ||
| * Executes a KSQL plan using the engine's primary service context. | ||
| */ | ||
| ExecuteResult execute(ConfiguredKsqlPlan plan); |
There was a problem hiding this comment.
I started to remove thie primary service context from this class to avoid the execute without it. I think you should remove this method in this PR.
| /** | ||
| * Computes a plan for executing a DDL/DML statement using the engine's primary service context. | ||
| */ | ||
| KsqlPlan plan(ConfiguredStatement<?> statement); |
There was a problem hiding this comment.
I started to remove thie primary service context from this class to avoid the execute without it. I think you should remove this method in this PR.
rodesai
force-pushed
the
add-engine-interface-rebase
branch
2 times, most recently
from
3a7f98a
to
9adace9
Compare
This patch adds interfaces to build and execute plans in the KSQL engine API, and changes StatementExecutor to use these interfaces to run statements. It also exposes a method executeQuery that the query endpoint can use to build transient queries.
rodesai
force-pushed
the
add-engine-interface-rebase
branch
from
9adace9
to
083e15d
Compare
|
@stevenpyzhang @spena can I get another look at this old PR? |
Description
This patch adds interfaces to build and execute plans in the KSQL engine
API, and changes StatementExecutor to use these interfaces to run statements.
It also exposes a method executeQuery that the query endpoint can use to build
transient queries.