feat: add CREATE SOURCE TABLE syntax and metadata info #7945

spena · 2021-08-04T17:34:47Z

Description

This is part of #7474 which adds support of source tables and streams.

This PR just adds new syntax to create source tables using CREATE SOURCE TABLE statements. It also marks those tables internally for future reference, such as CreateTable, CreateTableCommand and KsqlTable.

The KsqlTable is the object saved in the metastore. This object contains two new variables, readOnly and materialized. The readOnly just specifies the table is read-only; and materialized specifies this table is materialized (or should be materialized). I split the SOURCE keyword into these two variables because they mean different things. Also, we might support source streams in the future which they are read-only objects, but not materialized.

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 · 2021-08-04T17:35:45Z

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

+  public enum Type {
+    SOURCE,
+    NORMAL
+  }


I had to create the Type inside the CreateTable.Type enum because that gives better meaning to the table type we want to use (source or normal). I could have added to the parent class, CreateSource.Type, but that sounds like source is a table or stream type. I'm still not sure whether I put the type in the right place, so feedback is welcome.

Some ideas:

Put it under CreateSource with a different name, like ownershipType

Add the enum to KsqlConstants in ksqldb-common - CreateTableCommand won't have to use strings anymore

I guess the question is, what is exact purpose of this "type": read/write, materialized/non-materialized?

In the end, we also want to introduce a SOURCE STREAM, so putting it into the parent might make more sense?

spena · 2021-08-04T17:37:12Z

ksqldb-execution/src/main/java/io/confluent/ksql/execution/ddl/commands/CreateTableCommand.java

@@ -36,7 +37,8 @@ public CreateTableCommand(
      @JsonProperty(value = "topicName", required = true) final String topicName,
      @JsonProperty(value = "formats", required = true) final Formats formats,
      @JsonProperty(value = "windowInfo") final Optional<WindowInfo> windowInfo,
-      @JsonProperty(value = "orReplace", defaultValue = "false") final Optional<Boolean> orReplace
+      @JsonProperty(value = "orReplace", defaultValue = "false") final Optional<Boolean> orReplace,
+      @JsonProperty(value = "type", defaultValue = "NORMAL") final Optional<String> type


I used a String instead of the CreateTable.Type because of dependencies. I would have to add ksql-parser deps to the ksql-execution, which I'm not sure about it. So I just converted the type to a string from the CreateSourceFactory which calls this class. I then converted back to CreateTable.Type on the DdlCommandExec.

I would have to add ksql-parser deps to the ksql-execution, which I'm not sure about it.

What is the issue? Do we have other ENUM types that we write into the command topic? If yes, we should follow the same strategy.

Will use a boolean value instead of a Enum. I had another idea with the types to support types like external, temporary, etc in the future, so for compatibility I wanted to do it now, but I don't think that's really necessary. If they're supported, then users could mix the types, so we'll end up adding more parameters. I'll switch with the boolean instead.

mjsax

Couple of question (mainly for my own education...)

mjsax · 2021-08-10T06:31:51Z

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

+  public enum Type {
+    SOURCE,
+    NORMAL
+  }


I guess the question is, what is exact purpose of this "type": read/write, materialized/non-materialized?

In the end, we also want to introduce a SOURCE STREAM, so putting it into the parent might make more sense?

mjsax · 2021-08-10T06:33:15Z

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

+    NORMAL
+  }
+
+  private final Type type;


If we add type as field here (and not in the parent class), we need to update equals() and hashcode() accordingly. (Note sure why there is only a equals() but no hashcode() method...)

mjsax · 2021-08-10T06:35:04Z

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

@@ -27,15 +27,22 @@

 @Immutable
 public class CreateTable extends CreateSource implements ExecutableDdlStatement {
+  public enum Type {
+    SOURCE,
+    NORMAL


NORMAL is not very descriptive...

mjsax · 2021-08-10T06:35:45Z

ksqldb-parser/src/main/java/io/confluent/ksql/parser/SqlFormatter.java

@@ -259,7 +259,11 @@ protected Void visitCreateStream(final CreateStream node, final Integer indent)

    @Override
    protected Void visitCreateTable(final CreateTable node, final Integer indent) {
-      formatCreate(node, "TABLE");
+      formatCreate(node,
+          node.getType() == CreateTable.Type.SOURCE


When is SqlFormatter used?

It's used by the QueryLogger and other injector classes that re-formats the string statement when changing something in the statement (i.e. injecting a schema fetched from Schema Registry). I don't think we should re-format string statements, but this is an old class used by some legacy code.

mjsax · 2021-08-10T06:37:23Z

ksqldb-parser/src/main/java/io/confluent/ksql/parser/AstBuilder.java

@@ -281,13 +281,18 @@ public Node visitCreateTable(final SqlBaseParser.CreateTableContext context) {

      final Map<String, Literal> properties = processTableProperties(context.tableProperties());

+      final CreateTable.Type type = context.SOURCE() != null


When would this be null ?

Also, why is the method called SOURCE() is all caps?

context is what ANTlr generates based on the SqlBase.g4. For instance:

CREATE (OR REPLACE)? (SOURCE)? TABLE (IF NOT EXISTS)? sourceName (tableElements)? (WITH tableProperties)? #createTable

The SOURCE() is all caps because that's how it is defined in the .g4 file. It is not null when the SOURCE keyword is present, null otherwise.

If it's not null, what does it return?

mjsax · 2021-08-10T06:57:45Z

ksqldb-parser/src/test/java/io/confluent/ksql/parser/KsqlParserTest.java

-        ksqlTopicItems
+        ksqlTopicItems,
+        false,
+        false


Should we add a test for source tables, too?

This object is not even used anywhere in the test. I'll remove all the unused code. I'll add a test to parse CREATE SOURCE TABLE too.

mjsax · 2021-08-10T06:59:16Z

ksqldb-parser/src/test/java/io/confluent/ksql/parser/tree/CreateTableTest.java

+            new CreateTable(SOME_NAME, SOME_ELEMENTS, false, true, OTHER_PROPS, NORMAL)
+        )
+        .addEqualityGroup(
+            new CreateTable(SOME_NAME, SOME_ELEMENTS, false, false, OTHER_PROPS, SOURCE)


OTHER_PROPS -> SOME_PROPS ?

mjsax · 2021-08-10T07:00:19Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/server/TemporaryEngine.java

-                topic
+                topic,
+                false,
+                false


What is TemporaryEngine and would we need to add support for source tables?

We'll add it in a follow-up PR. This TemporaryEngine is used by the ListSourceExecutorTest, which tests the DESCRIBE and SHOW commands. There's a JIRA to make sure DESCRIBE correctly handles source streams & tables.

mjsax · 2021-08-10T07:01:26Z

...rest-app/src/test/java/io/confluent/ksql/rest/server/execution/InsertValuesExecutorTest.java

-          topic
+          topic,
+          false,
+          false


Does this PR add a test for rejected INSERT INTO for source tables? Should it? Or do we have some other work item for it?

This PR does not add it. Will be in a follow-up.

mjsax · 2021-08-10T07:01:38Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

-            ksqlTopic
+            ksqlTopic,
+            false,
+            false


jzaralim

lgtm, with one comment

jzaralim · 2021-08-10T21:53:38Z

ksqldb-parser/src/main/java/io/confluent/ksql/parser/SqlFormatter.java

-      formatCreate(node, "TABLE");
+      formatCreate(node,
+          node.isSource()
+              ? "SOURCE TABLE"


We can keep this method as is, and update formatCreate to add the "SOURCE" keyword if the node is a source. Then we won't have to do the same thing for streams.

guozhangwang

Sorry for the late reviews. All LGTM, I'm just wondering if we want to keep the isSource as a boolean or keep it as a enum (of course for not it would only have two possible values, source, or derived).

guozhangwang · 2021-08-11T22:50:41Z

ksqldb-engine/src/main/java/io/confluent/ksql/ddl/commands/CreateSourceFactory.java

@@ -142,10 +143,12 @@ public CreateTableCommand createTableCommand(final KsqlStructuredDataOutputNode
        outputNode.getKsqlTopic().getKafkaTopicName(),
        Formats.from(outputNode.getKsqlTopic()),
        outputNode.getKsqlTopic().getKeyFormat().getWindowInfo(),
-        Optional.of(outputNode.getOrReplace())
+        Optional.of(outputNode.getOrReplace()),
+        Optional.of(false)


nit: how about define another isDerived here, rather than using false to indicate is not source?

The parameter name is tricky. I initially wanted to use a Enum for future purposes like External, Temp, Source, etc types, but I found that those other types can be mixed with source too, like external source, temp source, or something. So I ended up using a boolean. IsDerived makes sense too. I think any of isDerived or isSource could work.

spena requested review from mjsax, jzaralim and a team August 4, 2021 17:34

spena commented Aug 4, 2021

View reviewed changes

spena force-pushed the source_table_readonly_metadata branch from c8be139 to 404f3b3 Compare August 5, 2021 19:05

mjsax reviewed Aug 10, 2021

View reviewed changes

spena force-pushed the source_table_readonly_metadata branch from 404f3b3 to 99871d7 Compare August 10, 2021 20:56

jzaralim approved these changes Aug 10, 2021

View reviewed changes

spena force-pushed the source_table_readonly_metadata branch from 2e6d0bc to 195898f Compare August 11, 2021 13:46

ConfluentJenkins added 5 commits August 11, 2021 13:36

feat: add CREATE SOURCE TABLE syntax

47d2f70

refactor: add readOnly & materialized variables to KsqlTable

f260e1c

address PR feedback

8382447

Replace Type.SOURCE in CreateTable with a Boolean parameter instead

00c4d60

fix test issues

140f281

spena force-pushed the source_table_readonly_metadata branch from 195898f to 140f281 Compare August 11, 2021 18:57

spena merged commit 70565f2 into master Aug 11, 2021

spena deleted the source_table_readonly_metadata branch August 11, 2021 21:10

guozhangwang reviewed Aug 11, 2021

View reviewed changes

jzaralim mentioned this pull request Aug 17, 2021

feat: add CREATE SOURCE STREAM syntax and metadata info #8004

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add CREATE SOURCE TABLE syntax and metadata info #7945

feat: add CREATE SOURCE TABLE syntax and metadata info #7945

spena commented Aug 4, 2021

spena Aug 4, 2021

jzaralim Aug 9, 2021

mjsax Aug 10, 2021

spena Aug 4, 2021

mjsax Aug 10, 2021

spena Aug 10, 2021

mjsax left a comment

mjsax Aug 10, 2021

mjsax Aug 10, 2021

mjsax Aug 10, 2021

mjsax Aug 10, 2021

spena Aug 10, 2021

mjsax Aug 10, 2021

spena Aug 10, 2021

mjsax Aug 10, 2021

mjsax Aug 10, 2021

spena Aug 10, 2021

mjsax Aug 10, 2021

mjsax Aug 10, 2021

spena Aug 10, 2021

mjsax Aug 10, 2021

spena Aug 10, 2021

mjsax Aug 10, 2021

jzaralim left a comment

jzaralim Aug 10, 2021

spena Aug 11, 2021

guozhangwang left a comment

guozhangwang Aug 11, 2021

spena Aug 19, 2021

		@@ -281,13 +281,18 @@ public Node visitCreateTable(final SqlBaseParser.CreateTableContext context) {

		final Map<String, Literal> properties = processTableProperties(context.tableProperties());

		final CreateTable.Type type = context.SOURCE() != null

feat: add CREATE SOURCE TABLE syntax and metadata info #7945

feat: add CREATE SOURCE TABLE syntax and metadata info #7945

Conversation

spena commented Aug 4, 2021

Description

Testing done

Reviewer checklist

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

mjsax 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

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

jzaralim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guozhangwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment