-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Ksql 386 ksql functions non static #370
Changes from 12 commits
0aca9a7
2c82044
ed1ee1e
27a0e1f
06b59a9
181fa50
8b299f3
bcec9eb
93634bf
a4c863c
ac3b527
e03d4d1
984e51a
c9b3f75
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 |
---|---|---|
|
@@ -17,8 +17,11 @@ | |
package io.confluent.ksql; | ||
|
||
import io.confluent.ksql.metastore.MetaStore; | ||
import io.confluent.ksql.util.KafkaTopicClient; | ||
import io.confluent.ksql.util.KafkaTopicClientImpl; | ||
import io.confluent.ksql.util.KsqlConfig; | ||
|
||
import org.apache.kafka.clients.admin.AdminClient; | ||
import org.apache.kafka.streams.StreamsConfig; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
@@ -38,9 +41,19 @@ public class KsqlContext { | |
private final KsqlEngine ksqlEngine; | ||
private static final String APPLICATION_ID_OPTION_DEFAULT = "ksql_standalone_cli"; | ||
private static final String KAFKA_BOOTSTRAP_SERVER_OPTION_DEFAULT = "localhost:9092"; | ||
private final KafkaTopicClientImpl topicClient; | ||
private final AdminClient adminClient; | ||
private final KafkaTopicClient topicClient; | ||
|
||
|
||
public static KsqlContext create() { | ||
return new KsqlContext(); | ||
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. delegation, i.e, 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. Good point, made the changes. |
||
} | ||
|
||
public KsqlContext() { | ||
public static KsqlContext create(Map<String, Object> streamsProperties) { | ||
return new KsqlContext(streamsProperties); | ||
} | ||
|
||
private KsqlContext() { | ||
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. can remove this 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. Done! |
||
this(null); | ||
} | ||
|
||
|
@@ -50,7 +63,7 @@ public KsqlContext() { | |
* | ||
* @param streamsProperties | ||
*/ | ||
public KsqlContext(Map<String, Object> streamsProperties) { | ||
private KsqlContext(Map<String, Object> streamsProperties) { | ||
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. put the logic in here into the 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. Done! |
||
if (streamsProperties == null) { | ||
streamsProperties = new HashMap<>(); | ||
} | ||
|
@@ -61,11 +74,17 @@ public KsqlContext(Map<String, Object> streamsProperties) { | |
streamsProperties.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, KAFKA_BOOTSTRAP_SERVER_OPTION_DEFAULT); | ||
} | ||
KsqlConfig ksqlConfig = new KsqlConfig(streamsProperties); | ||
|
||
topicClient = new KafkaTopicClientImpl(ksqlConfig.getKsqlAdminClientConfigProps()); | ||
adminClient = AdminClient.create(ksqlConfig.getKsqlAdminClientConfigProps()); | ||
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. As it stands we can't test this class without starting up kafka etc. I suggest we change it so that we can unit test the class. We could add one or two static factory methods, i.e, 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. Good point! I'll make the changes. |
||
topicClient = new KafkaTopicClientImpl(adminClient); | ||
ksqlEngine = new KsqlEngine(ksqlConfig, topicClient); | ||
} | ||
|
||
protected KsqlContext(KsqlEngine ksqlEngin, AdminClient adminClient, KafkaTopicClient | ||
topicClient) { | ||
this.ksqlEngine = ksqlEngin; | ||
this.adminClient = adminClient; | ||
this.topicClient = topicClient; | ||
} | ||
|
||
public MetaStore getMetaStore() { | ||
return ksqlEngine.getMetaStore(); | ||
|
@@ -98,5 +117,6 @@ public void sql(String sql) throws Exception { | |
public void close() throws IOException { | ||
ksqlEngine.close(); | ||
topicClient.close(); | ||
adminClient.close(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,8 +126,9 @@ private PlanNode buildQueryLogicalPlan(final Query query, final MetaStore tempMe | |
|
||
AggregateAnalysis aggregateAnalysis = new AggregateAnalysis(); | ||
AggregateAnalyzer aggregateAnalyzer = new | ||
AggregateAnalyzer(aggregateAnalysis, analysis); | ||
AggregateExpressionRewriter aggregateExpressionRewriter = new AggregateExpressionRewriter(); | ||
AggregateAnalyzer(aggregateAnalysis, analysis, ksqlEngine.getFunctionRegistry()); | ||
AggregateExpressionRewriter aggregateExpressionRewriter = | ||
new AggregateExpressionRewriter(ksqlEngine.getFunctionRegistry()); | ||
for (Expression expression: analysis.getSelectExpressions()) { | ||
aggregateAnalyzer | ||
.process(expression, new AnalysisContext(null)); | ||
|
@@ -160,7 +161,7 @@ private PlanNode buildQueryLogicalPlan(final Query query, final MetaStore tempMe | |
|
||
|
||
// Build a logical plan | ||
PlanNode logicalPlan = new LogicalPlanner(analysis, aggregateAnalysis).buildPlan(); | ||
PlanNode logicalPlan = new LogicalPlanner(analysis, aggregateAnalysis, ksqlEngine.getFunctionRegistry()).buildPlan(); | ||
if (logicalPlan instanceof KsqlStructuredDataOutputNode) { | ||
KsqlStructuredDataOutputNode ksqlStructuredDataOutputNode = | ||
(KsqlStructuredDataOutputNode) logicalPlan; | ||
|
@@ -217,7 +218,9 @@ public void buildQueryPhysicalPlan(final List<QueryMetadata> physicalPlans, | |
KsqlConfig ksqlConfigClone = ksqlEngine.getKsqlConfig().clone(); | ||
|
||
// Build a physical plan, in this case a Kafka Streams DSL | ||
PhysicalPlanBuilder physicalPlanBuilder = new PhysicalPlanBuilder(builder, ksqlConfigClone, ksqlEngine.getTopicClient(), new MetastoreUtil()); | ||
PhysicalPlanBuilder physicalPlanBuilder = new PhysicalPlanBuilder(builder, ksqlConfigClone, | ||
ksqlEngine.getTopicClient(), | ||
new MetastoreUtil(), ksqlEngine.getFunctionRegistry()); | ||
SchemaKStream schemaKStream = physicalPlanBuilder.buildPhysicalPlan(logicalPlan); | ||
|
||
OutputNode outputNode = physicalPlanBuilder.getPlanSink(); | ||
|
@@ -291,7 +294,8 @@ private QueryMetadata buildPlanForBareQuery(boolean addUniqueTimeSuffix, | |
schemaKStream.getExecutionPlan(""), | ||
schemaKStream.getQueue(), | ||
(sourceSchemaKstream instanceof SchemaKTable) ? | ||
DataSource.DataSourceType.KTABLE : DataSource.DataSourceType.KSTREAM | ||
DataSource.DataSourceType.KTABLE : DataSource.DataSourceType.KSTREAM, | ||
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. Maybe add a 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. Let's discuss this a bit more. |
||
applicationId | ||
); | ||
} | ||
|
||
|
@@ -308,9 +312,15 @@ private QueryMetadata buildPlanForBareQuery(boolean addUniqueTimeSuffix, | |
* @param persistanceQueryPrefix | ||
*/ | ||
private QueryMetadata buildPlanForStructuredOutputNode(boolean addUniqueTimeSuffix, | ||
Pair<String, PlanNode> statementPlanPair, Map<String, Object> overriddenStreamsProperties, | ||
boolean updateMetastore, StreamsBuilder builder, KsqlConfig ksqlConfigClone, SchemaKStream schemaKStream, | ||
KsqlStructuredDataOutputNode outputNode, String serviceId, String persistanceQueryPrefix) { | ||
Pair<String, PlanNode> statementPlanPair, | ||
Map<String, Object> overriddenStreamsProperties, | ||
boolean updateMetastore, | ||
StreamsBuilder builder, | ||
KsqlConfig ksqlConfigClone, | ||
SchemaKStream schemaKStream, | ||
KsqlStructuredDataOutputNode outputNode, | ||
String serviceId, | ||
String persistanceQueryPrefix) { | ||
|
||
long queryId = getNextQueryId(); | ||
|
||
|
@@ -353,7 +363,9 @@ private QueryMetadata buildPlanForStructuredOutputNode(boolean addUniqueTimeSuff | |
streams, outputNode, schemaKStream | ||
.getExecutionPlan(""), queryId, | ||
(schemaKStream instanceof SchemaKTable) ? DataSource | ||
.DataSourceType.KTABLE : DataSource.DataSourceType.KSTREAM); | ||
.DataSourceType.KTABLE : DataSource.DataSourceType | ||
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. ditto here |
||
.KSTREAM, | ||
applicationId); | ||
} | ||
|
||
|
||
|
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.
Do these need to be public? Looks like they could be package protected and don't need to be known by a user?