feat: add WITH clause and QUERY_ID property to INSERT INTO statements #6553

spena · 2020-10-30T20:11:17Z

Description

Implements #6533

Add a WITH clause to the INSERT statement to allow users specify a custom query QUERY_ID.
For intance:

ksql> insert into s1 with (query_id='my_insert_id') select * from s1;

 Message                            
------------------------------------
 Created query with ID MY_INSERT_ID 
------------------------------------

ksql> show queries;

 Query ID     | Query Type | Status    | Sink Name | Sink Kafka Topic | Query String                                              
----------------------------------------------------------------------------------------------------------------------------------
 MY_INSERT_ID | PERSISTENT | RUNNING:1 | S1        | t1               | insert into s1 with (query_id='my_insert_id') select * from s1; 
----------------------------------------------------------------------------------------------------------------------------------
For detailed information on a Query run: EXPLAIN <Query ID>;

ksql> terminate my_insert_id;

 Message           
-------------------
 Query terminated. 
-------------------

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

spena · 2020-10-30T20:13:11Z

ksqldb-common/src/main/java/io/confluent/ksql/properties/with/InsertIntoConfigs.java

+ * 'With Clause' properties for 'INSERT INTO' statements.
+ */
+public final class InsertIntoConfigs {
+  public static final String QUERY_ID_PROPERTY = "ID";


I chose ID as the best name for this property. However, if it is confusing with the ID keyword in the schema (i.e. ID id int), then we can consider these options.

with (id = 'my_id') with (query_id = 'my_id') with (name = 'my_id') with (alias = 'my_id')

I think originally I liked id, but now my preference is query_id 😂 it gives a little bit more clarity into what exactly it is. cc @colinhicks

agavra

LGTM! Some minor comments inline

agavra · 2020-11-03T23:06:55Z

ksqldb-common/src/main/java/io/confluent/ksql/properties/with/InsertIntoConfigs.java

+ * 'With Clause' properties for 'INSERT INTO' statements.
+ */
+public final class InsertIntoConfigs {
+  public static final String QUERY_ID_PROPERTY = "ID";


I think originally I liked id, but now my preference is query_id 😂 it gives a little bit more clarity into what exactly it is. cc @colinhicks

agavra · 2020-11-03T23:07:18Z

ksqldb-common/src/main/java/io/confluent/ksql/properties/with/InsertIntoConfigs.java

+          ConfigDef.Type.STRING,
+          null,
+          ConfigDef.Importance.LOW,
+          "Custom query ID to use for INSERT INTO queries"


let's have some more detailed documentation here. What is it used for? what happens if it isn't supplied? what are valid values? is it case insensitive?

agavra · 2020-11-03T23:08:09Z

ksqldb-engine/src/main/java/io/confluent/ksql/engine/QueryIdUtil.java


  private QueryIdUtil() {
  }

+  private static void validateWithQueryId(final String queryId) {


we should make this a ConfigDef.Validator and validate it as part of the config def

I initially thought of this, but I decided to do the validation in the QueryIdUtil to make sure that whoever wants to use that class, then they pass the right QueryID string. I don't think nobody else will use it, but I think is a better design as the buildId is the one will check IDs are correct.

Unless you have another reason to use the Validator?

yeah that makes sense

agavra · 2020-11-03T23:12:41Z

ksqldb-engine/src/main/java/io/confluent/ksql/engine/QueryIdUtil.java

-      final boolean createOrReplaceEnabled) {
+      final boolean createOrReplaceEnabled,
+      final Optional<String> withQueryId) {
+    if (withQueryId.isPresent()) {


should we also validate to make sure they don't register the same queryID twice? (please add a test for this)

Done. I added the validation in the EngineExecutor 'cause is the one that knows about IDs by calling the engineContext.getPersistentQuery.

agavra · 2020-11-03T23:14:03Z

ksqldb-parser/src/main/java/io/confluent/ksql/parser/tree/QueryContainer.java

@@ -33,4 +35,11 @@
   * @return the sink info.
   */
  Sink getSink();
+
+  /**
+   * Return an optional query ID if specified by the user


let's specify here that this is only intended to be used by INSERT INTO - that way we can remove it when we remove INSERT INTO

agavra · 2020-11-03T23:14:59Z

ksqldb-engine/src/test/java/io/confluent/ksql/engine/KsqlEngineTest.java

@@ -347,6 +347,31 @@ public void shouldThrowWhenInsertIntoSchemaDoesNotMatch() {
        is("insert into bar select orderTime, itemid from orders;")));
  }

+  @Test
+  public void shouldExecuteInsertIntoWithCustomQueryId() {


Can we add a test in RecoveryTest to make sure we can recover create/terminate with custom query IDs?

spena · 2020-11-04T20:24:20Z

Thanks @agavra. I changed the ID to QUERY_ID. I liked it better too.

- Change ID to QUERY_ID - Better comments in InsertIntoConfigs - Check if new QueryId already exists - Add RecoverTest test

spena added the enhancement label Oct 30, 2020

spena added this to the 0.15.0 milestone Oct 30, 2020

spena requested a review from a team October 30, 2020 20:11

spena commented Oct 30, 2020

View reviewed changes

spena force-pushed the insert_with_clause branch from e2f2f6e to 0c24324 Compare November 2, 2020 16:42

agavra approved these changes Nov 3, 2020

View reviewed changes

spena force-pushed the insert_with_clause branch from 0c24324 to 3b66921 Compare November 4, 2020 20:20

spena force-pushed the insert_with_clause branch 2 times, most recently from f9d7d70 to 66ce5e7 Compare November 5, 2020 21:49

spena added 3 commits November 6, 2020 08:51

feat: add WITH clause and ID property to INSERT INTO statements

e06547c

chore: work on PR feedback and Almog's suggestions

5e6d37e

- Change ID to QUERY_ID - Better comments in InsertIntoConfigs - Check if new QueryId already exists - Add RecoverTest test

chore: fix test

075bc02

spena force-pushed the insert_with_clause branch from 66ce5e7 to 075bc02 Compare November 6, 2020 14:52

spena changed the title ~~feat: add WITH clause and ID property to INSERT INTO statements~~ feat: add WITH clause and QUERY_ID property to INSERT INTO statements Nov 6, 2020

spena merged commit a344114 into confluentinc:master Nov 6, 2020

spena deleted the insert_with_clause branch November 6, 2020 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add WITH clause and QUERY_ID property to INSERT INTO statements #6553

feat: add WITH clause and QUERY_ID property to INSERT INTO statements #6553

spena commented Oct 30, 2020 •

edited by colinhicks

Loading

spena Oct 30, 2020

agavra Nov 3, 2020

agavra left a comment

agavra Nov 3, 2020

agavra Nov 3, 2020

spena Nov 4, 2020

agavra Nov 3, 2020

spena Nov 4, 2020

agavra Nov 4, 2020

agavra Nov 3, 2020

spena Nov 4, 2020

agavra Nov 3, 2020

spena Nov 4, 2020

agavra Nov 3, 2020

spena Nov 4, 2020

spena commented Nov 4, 2020

feat: add WITH clause and QUERY_ID property to INSERT INTO statements #6553

feat: add WITH clause and QUERY_ID property to INSERT INTO statements #6553

Conversation

spena commented Oct 30, 2020 • edited by colinhicks Loading

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena commented Nov 4, 2020

spena commented Oct 30, 2020 •

edited by colinhicks

Loading