Skip to content
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

feat: add basic support for key syntax #3034

Merged
merged 7 commits into from
Jul 14, 2019

Conversation

big-andy-coates
Copy link
Contributor

Description

Adds ability to explicitly include key fields in C* statements so long as:

  • the key field is named ROWKEY
  • it of type STRING.

i.e. you can now do:

CREATE STREAM FOO AS (ROWKEY STRING KEY, V0 INT) ...;

Which is semantically the same:

CREATE STREAM FOO AS (V0 INT) ...;

Testing done

mvn test

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 #")

Adds ability to explicitly include key fields in C* statements so long as:
- the key field is named `ROWKEY`
- it of type `STRING`.

i.e. you can now do:

`CREATE STREAM FOO AS (ROWKEY STRING KEY, ...) ...;`
@big-andy-coates big-andy-coates requested a review from a team as a code owner July 1, 2019 15:33
Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @big-andy-coates, left a few comments.
Are you planning to support CT statement for topics with WINDOWED keys?

@big-andy-coates big-andy-coates requested a review from a team July 2, 2019 12:37
@big-andy-coates
Copy link
Contributor Author

@hjafarpour

Are you planning to support CT statement for topics with WINDOWED keys?

We currently do through the use of the WINDOW_TYPE with clause property. However, as I understand it, any use of this will currently eventually run out of disk space and crash, due to a bug in KS. I'm not certain, but I think the same may be true of any KSQL topology that uses a windowed table as a source.

@mjsax knows more about the details. They're planning to fix this with KIP-300: https://cwiki.apache.org/confluence/display/KAFKA/KIP-300%3A+Add+Windowed+KTable+API+in+StreamsBuilder

If you're wondering how the syntax will work in KSQL for windowed keys, then for this initial piece I wasn't planning on changing how they currently work. So the user will be able to use CREATE TABLE x (ROWKEY STRING KEY, ...) to create a table. Whether the key is windowed or not will continue to be controlled by the WINDOW_TYPE property.

Conflicting files
ksql-engine/src/test/java/io/confluent/ksql/ddl/commands/CommandFactoriesTest.java
ksql-engine/src/test/java/io/confluent/ksql/ddl/commands/CreateSourceCommandTest.java
ksql-engine/src/test/java/io/confluent/ksql/ddl/commands/CreateStreamCommandTest.java
ksql-engine/src/test/java/io/confluent/ksql/ddl/commands/CreateTableCommandTest.java
ksql-engine/src/test/java/io/confluent/ksql/schema/ksql/inference/DefaultSchemaInjectorTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/KsqlParserTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/SchemaParserTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/SqlFormatterTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/rewrite/StatementRewriterTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/tree/CreateStreamTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/tree/CreateTableTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/tree/TableElementTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/tree/TableElementsTest.java
ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/StandaloneExecutorTest.java
ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java
Conflicting files
ksql-engine/src/test/java/io/confluent/ksql/ddl/commands/CommandFactoriesTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/tree/CreateStreamTest.java
ksql-parser/src/test/java/io/confluent/ksql/parser/tree/CreateTableTest.java
Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending green build!

@big-andy-coates big-andy-coates requested a review from a team July 12, 2019 19:29
@big-andy-coates
Copy link
Contributor Author

Merging with one reviewer, as it's been two weeks since this PR was raised.

@big-andy-coates big-andy-coates merged commit ca6478a into confluentinc:master Jul 14, 2019
@big-andy-coates big-andy-coates deleted the key_syntax branch July 14, 2019 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants