Struct Data Type(Part-1): New struct type, DDL statements and Describe #1114

hjafarpour · 2018-04-03T22:00:55Z

The first part of struct (nested) data type support. This PR adds a new type, STRUCT, and adds support in DDL statements along with DESCRIBE statement.
Here is an example of how you can define a stream/table with struct field:

CREATE STREAM orders (ordertime bigint, orderid bigint, itemid varchar, orderunits double, arraycol array<double>, mapcol map<varchar, double>, address STRUCT < number bigint, street varchar, city varchar, state varchar, zipcode bigint>) WITH (kafka_topic='orders_with_struct_topic_json', key='orderid', value_format='json');

This is the first step to resolve several filed issues including #638 and #824.

…nsupported-Operations-From-The-Grammar

…nce-To-AST-Nodes

get rid of String encoded values. registered -> boolean replicaInfo -> list<int> instead of string encoding that info partitionCount -> gone since it is encoded in replicaInfo

…rrides (confluentinc#924) This includes one minor fix to the MetricsCollector to make it correctly robust to reuse across tests that are both aware of its initialization/cleanup and those that are not.

…#932)

* reduce checkstyle suppressions * remove commented out suppression

clusterId metadata was introduced in Kafka 0.10.1

…nce-To-AST-Nodes

rodesai

Took a first pass through. Looks good. Left some comments inline.

rodesai · 2018-05-09T18:06:46Z

ksql-common/src/main/java/io/confluent/ksql/util/SchemaUtil.java

+      return "MAP[" + getSchemaFieldType(field.schema().keySchema().fields().get(0)) + ","
+          + getSchemaFieldType(field.schema().valueSchema().fields().get(0)) + "]";
+    } else if (field.schema().type() == Schema.Type.STRUCT) {
+      StringBuilder stringBuilder = new StringBuilder("STRUCT <");


stringBuilder.append(
field.schema().fields().stream()
.map(getSchemaFieldType)
.collect(Collectors.joining(", ")));

rodesai · 2018-05-09T18:44:40Z

ksql-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4

@@ -263,6 +263,10 @@ primaryExpression
    | POSITION '(' valueExpression IN valueExpression ')'                            #position
    | '(' expression (',' expression)+ ')'                                           #rowConstructor
    | ROW '(' expression (',' expression)* ')'                                       #rowConstructor
+    | '(' expression (',' expression)+ ')'


This is the same as rowConstructor, which seems bad. Isn't this ambiguous? Kind of surprising antlr doesn't raise an error. I guess its working fine since we're not using row constructors anywhere?

We don't need ROW. Removed it!

rodesai · 2018-05-09T18:45:27Z

ksql-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4

@@ -263,6 +263,10 @@ primaryExpression
    | POSITION '(' valueExpression IN valueExpression ')'                            #position
    | '(' expression (',' expression)+ ')'                                           #rowConstructor
    | ROW '(' expression (',' expression)* ')'                                       #rowConstructor
+    | '(' expression (',' expression)+ ')'
+    #structConstructor
+    | STRUCT '(' expression (',' expression)* ')'


this is here for when we want to create structs on the fly? Is this what the final syntax is going to look like? Maybe leave it out until that's decided?

Yes, we can decide about this later. Removed it.

rodesai · 2018-05-09T22:31:07Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Map.java

+
+public class Map extends Type {
+
+  final Type keyType = new PrimitiveType(KsqlType.STRING);


Why is the key always STRING?

Key for map is always string.

Ah - I didn't know avro keys were restricted to strings too. Makes sense. We should raise an error if a user has a ddl statement with MAP<not-STRING,...> in it. Or change the grammar to just take a value parameter.

rodesai · 2018-05-09T23:12:16Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/DefaultTraversalVisitor.java

@@ -362,4 +372,12 @@ protected R visitInsertInto(InsertInto node, C context) {
    process(node.getQuery(), context);
    return null;
  }
+
+  protected R visitCreateStreamAsSelect(CreateStreamAsSelect node, C context) {


just curious - is this used anywhere? the analyzers seem to just operate on Query nodes.

This is a visitor that can be used by creating subclasses of it. So even though we may not use it now we may have future uses for it.

rodesai · 2018-05-09T23:26:39Z

ksql-common/src/test/java/io/confluent/ksql/util/SchemaUtilTest.java

+                                          + "\t arraycol ARRAY[DOUBLE], \n"
+                                          + "\t mapcol MAP[VARCHAR(STRING),DOUBLE], \n"
+                                          + "\t address STRUCT < \n"
+                                          + "\t NUMBER BIGINT, \n"


would be nice to add an indent here. I'm fine with punting this to another PR though.

Yes, for now I'll leave it as is. We may want to provide unified ux for both web based UI and CLI

rodesai · 2018-05-09T23:33:51Z

ksql-parser/src/test/java/io/confluent/ksql/parser/util/TypeUtilTest.java

+
+  @Test
+  public void shouldGetCorrectKsqlType() throws Exception {
+


we should add a case for INT64

rodesai · 2018-05-09T23:34:42Z

ksql-parser/src/test/java/io/confluent/ksql/parser/util/TypeUtilTest.java

+    assertThat(schema8.field("COL1").schema().type(), equalTo(Schema.Type.STRING));
+    assertThat(schema8.field("COL4").schema().type(), equalTo(Schema.Type.ARRAY));
+    assertThat(schema8.field("COL5").schema().type(), equalTo(Schema.Type.MAP));
+  }


We should add a nested struct case here.

rodesai · 2018-05-09T23:37:37Z

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

+    Statement statement = KSQL_PARSER.buildAst(queryStr, metaStore).get(0);
+    Assert.assertTrue("testCreateStream failed.", statement instanceof CreateStream);
+    CreateStream createStream = (CreateStream)statement;
+    Assert.assertTrue("testCreateStream failed.", createStream.getName().toString().equalsIgnoreCase("ORDERS"));


assertThat(createStream.getName().toString().toUpper(), equals("ORDERS"))
ditto below

rodesai · 2018-05-09T23:38:04Z

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

+    Assert.assertTrue("testCreateStream failed.", statement instanceof CreateStream);
+    CreateStream createStream = (CreateStream)statement;
+    Assert.assertTrue("testCreateStream failed.", createStream.getName().toString().equalsIgnoreCase("ORDERS"));
+    Assert.assertTrue("testCreateStream failed.", createStream.getElements().size() == 7);


assertThat(.., equals(..))
ditto below

rodesai

Looks good. One small comment inline.

rodesai · 2018-05-10T17:42:39Z

ksql-parser/src/test/java/io/confluent/ksql/parser/util/TypeUtilTest.java

+    assertThat(((Struct) type7).getItems().get(5).getRight().getKsqlType(), equalTo(Type.KsqlType
+                                                                                        .STRUCT));
+
+    Type type8 = TypeUtil.getKsqlType(Schema.INT64_SCHEMA);


We should move this up alongside the other primitive cases.

dguy

Thanks @hjafarpour, left some comments

dguy · 2018-05-11T16:42:29Z

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

@@ -1614,4 +1588,23 @@ public InvalidColumnReferenceException(String message, Throwable cause) {
      super(message, cause);
    }
  }
+
+  private static PrimitiveType getPrimitiveType(String typeName) {


maybe put this as a static method on PrimitiveType

dguy · 2018-05-11T16:46:13Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Array.java

+  }
+
+  @Override
+  public int hashCode() {


is this intentional? i.e., hashcode always 0 and equals always false?

No, updated the implementations.

dguy · 2018-05-11T16:47:46Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Map.java

+
+public class Map extends Type {
+
+  final Type keyType = new PrimitiveType(KsqlType.STRING);


private ?

dguy · 2018-05-11T16:48:22Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Map.java

+    return visitor.visitMap(this, context);
+  }
+
+  public Type getKeyType() {


Do we actually need keyType if it is always String?

It was more for clarifying it. I removed it.

dguy · 2018-05-11T16:48:39Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Map.java

+  }
+
+  @Override
+  public int hashCode() {


hashCode and equals again

dguy · 2018-05-11T16:49:33Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Node.java

+  }
+
+  public void setParent(Node parent) {
+    if (parent == null) {


this.parent = Optional.ofNullable(parent);

dguy · 2018-05-11T16:53:07Z

ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Type.java

+public abstract class Type extends Expression {
+
+
+  public enum KsqlType {


why do we have an enum for KsqlType and a class hierarchy extending Type? IMO it would be better if they enum was for PrimitiveTypes only and was an internal thing. At the moment we have too many things like Schema, Type, etc that have corresponding XXXXUtil classes rather than having objects and using polymorphism. We need to address this

dguy · 2018-05-11T16:53:42Z

ksql-parser/src/test/java/io/confluent/ksql/parser/rewrite/SetParentVisitorTest.java

+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class SetParentVisitorTest {


I'm not sure this test covers everything in SetParentVisitor

dguy · 2018-05-11T16:56:22Z

ksql-parser/src/test/java/io/confluent/ksql/parser/util/TypeUtilTest.java

+public class TypeUtilTest {
+
+  @Test
+  public void shouldGetCorrectKsqlType() throws Exception {


Multiple tests. Much better to have this broken down into individual tests than one big test

Also, doesn't throw exception

Broke it down to multiple tests.

dguy · 2018-05-11T16:57:06Z

ksql-parser/src/test/java/io/confluent/ksql/parser/util/TypeUtilTest.java

+  }
+
+  @Test
+  public void shouldGetCorrectSchema() throws Exception {


Ditto above

dguy · 2018-05-11T22:28:31Z

Something that @bluemonk3y mentioned and makes a lot of sense... It would be better for the future if we replaces GenericRow with Struct, i.e., why should GenericRow be special? It is really just Struct all the way down.

hjafarpour · 2018-05-14T21:26:05Z

@dguy yes, that's a good point and we can do it. I created a github issue to track it: #1304
We can implement it in a separate PR.

dguy · 2018-05-14T22:06:15Z

@hjafarpour it would make sense to do this before any further work struct based PRs are done. So if it isn't in this PR you probably want to do it in the next one

hjafarpour · 2018-05-14T22:19:02Z

@dguy yes, I can do it in a PR after switching to Connect data type. This PR is only for DDL statement and ksql type.

dguy

Thanks, LGTM

hjafarpour and others added 30 commits January 29, 2018 12:45

Cleaned up the grammar.

322f8f2

Merge remote-tracking branch 'upstream/master'

0911202

More clean up

f69c25c

Merge remote-tracking branch 'upstream/master' into KSQL-665-Remove-U…

3e6200b

…nsupported-Operations-From-The-Grammar

Added parent reference to AST nodes.

415aa49

Merge remote-tracking branch 'upstream/master'

0253884

Merge branch 'master' of https://github.com/confluentinc/ksql

3846f4c

Merge branch 'master' of https://github.com/confluentinc/ksql

7893e2c

Merge branch 'master' of https://github.com/confluentinc/ksql

f391466

Merge remote-tracking branch 'upstream/master'

b45efb0

Merge branch 'master' of https://github.com/confluentinc/ksql

4af44e3

Merge remote-tracking branch 'upstream/master' into Add-Parent-Refere…

38c5456

…nce-To-AST-Nodes

remove unused/old suppressions. reduce scope on some (confluentinc#915)

f84d561

Merge remote-tracking branch 'upstream/4.1.x'

e885e75

Merge branch 'master' of https://github.com/confluentinc/ksql

fa1a261

rename kafkaTopic->topic to match SourceInfo

0f68684

proper typing for KafkaTopicInfo

ef41a33

get rid of String encoded values. registered -> boolean replicaInfo -> list<int> instead of string encoding that info partitionCount -> gone since it is encoded in replicaInfo

Fix integration test annotations and remove maven-surefire-plugin ove…

e1821cf

…rrides (confluentinc#924) This includes one minor fix to the MetricsCollector to make it correctly robust to reuse across tests that are both aware of its initialization/cleanup and those that are not.

Merge remote-tracking branch 'origin/4.1.x'

511f5e3

KSQL-739 return kafka cluster id as part of server info (confluentinc…

7032379

…#932)

Merge remote-tracking branch 'origin/4.1.x'

8b58d47

reduce checkstyle suppressions (confluentinc#938)

1e96708

* reduce checkstyle suppressions * remove commented out suppression

remove AbbreviationAsWordInName suppression (confluentinc#937)

41eeb16

fix build (confluentinc#946)

2e1d04d

Merge remote-tracking branch 'origin/4.1.x'

c2e05f3

Merge branch '4.1.x'

02983f6

Merge remote-tracking branch 'origin/4.1.x'

e19bfb7

Merge remote-tracking branch 'upstream/4.1.x'

dabfab2

Merge remote-tracking branch 'origin/4.1.x'

46ce7e8

cluster id check subsumes broker compatibility (confluentinc#959)

d5493ba

clusterId metadata was introduced in Kafka 0.10.1

hjafarpour added 5 commits March 29, 2018 14:11

Merge remote-tracking branch 'upstream/master' into Add-Parent-Refere…

f4745ea

…nce-To-AST-Nodes

Minor fix.

c3bfd72

Merge remote-tracking branch 'upstream/master' into Add-Struct-Type

096d37e

Describe with struct type works!

e7399d6

Removed old doc files.

c106874

hjafarpour added the enhancement label Apr 3, 2018

hjafarpour self-assigned this Apr 3, 2018

hjafarpour requested a review from a team April 3, 2018 22:00

hjafarpour added 9 commits April 12, 2018 08:23

Merge remote-tracking branch 'upstream/master' into Add-Struct-Type

434bf5d

Merge remote-tracking branch 'upstream/master' into Add-Parent-Refere…

6fb6bcb

…nce-To-AST-Nodes

Merge branch 'Add-Parent-Reference-To-AST-Nodes' into Add-Struct-Type

5517b54

Merge remote-tracking branch 'upstream/master' into Add-Parent-Refere…

787a8a8

…nce-To-AST-Nodes

Applied review feedback.

30bcab0

Merge branch 'Add-Parent-Reference-To-AST-Nodes' into Add-Struct-Type

cb50d56

Minor change

573c28a

Merge remote-tracking branch 'upstream/master' into Add-Parent-Refere…

8a6e836

…nce-To-AST-Nodes

Merge branch 'Add-Parent-Reference-To-AST-Nodes' into Add-Struct-Type

83e06c0

rodesai reviewed May 9, 2018

View reviewed changes

Applied review feedback

4be7b47

rodesai approved these changes May 10, 2018

View reviewed changes

dguy reviewed May 11, 2018

View reviewed changes

hjafarpour added 3 commits May 14, 2018 11:30

Applied the feedback.

d625753

Merge remote-tracking branch 'upstream/master' into Add-Struct-Type

ab80df9

Fix findbug issue.

87dc0da

dguy approved these changes May 15, 2018

View reviewed changes

hjafarpour merged commit e5658dc into confluentinc:master May 15, 2018


		public class Map extends Type {

		final Type keyType = new PrimitiveType(KsqlType.STRING);


		@Test
		public void shouldGetCorrectKsqlType() throws Exception {

		public abstract class Type extends Expression {


		public enum KsqlType {

Struct Data Type(Part-1): New struct type, DDL statements and Describe #1114

Struct Data Type(Part-1): New struct type, DDL statements and Describe #1114

Conversation

hjafarpour commented Apr 3, 2018

rodesai 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

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dguy 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

dguy commented May 11, 2018

hjafarpour commented May 14, 2018

dguy commented May 14, 2018

hjafarpour commented May 14, 2018

dguy left a comment

Choose a reason for hiding this comment