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(static): initial syntax for static queries #3300

Merged
merged 12 commits into from
Sep 10, 2019

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Sep 4, 2019

Description

NOTE: this may not be the final syntax we go with, but it's good enough for now.

NOTE: Changed to have reduced scope to only the minimum required.

This PR contains the changes needed to add support for the new EMIT CHANGES clause. (Discussion on syntax in #3242).

The EMIT is used to indicate the query is continuous, as opposed to the proposed YIELD which will indicate its static. (Note YIELD is not included in this PR.)

The CHANGES is used to indicate the query outputs intermediate results, as opposed to the proposed FINAL which would output materialized results, or other extensions to express other ways of materializing. (Note: FINAL is not included in this PR).

Persistent queries, (CSAS, CTAS + INSERT INTO), default to EMIT CHANGES.
Bare queries, (SELECT * FROM X;), default to YIELD FINAL.

Presence of any of these is a no-op at the moment. A follow up PR will wire up functionality.

Some example queries this syntax supports, (note: this doesn't mean we're implementing them initially!):

-- bare static query, outputting final result.
SELECT * FROM X;

-- bare continuous query, outputting intermediate results:
SELECT * FROM X EMIT CHANGES;

-- persistent continuous query, outputting intermediate results:
-- Note: 'EMIT CHANGES' is optional/implicit until next major version bump:
CREATE TABLE X AS SELECT SUM(V) FROM Y GROUP BY Z [EMIT CHANGES];

Backwards compatibility:

With this change:

  • bare queries, e.g. SELECT * FROM X; are not backwards compatible. You'll need to add EMIT STREAM on the end to get the same behaviour. However, such queries are not persisted in the command topic, so this is not so much of an issue
  • persistent queries are backwards compatible for now. It's proposed we make EMIT CHANGES a requirement as part of our next major version bump.

Commit history:

I would strongly advise reviewing by-commit

  1. Add syntax to SqlBase.g4 and update AstBuilder.s
  2. Update SqlFormatter to support EMIT and WITH syntax.
  3. Merge from master
  4. Switches tests over to new style. Static or Final queries now fail.
  5. Switch use of WITH to use of YIELD.
  6. Reduce scope of syntax changes to just EMIT CHANGES.

Testing done

Added suitable tests to AstBuilderTest.

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

The change contains only the syntax changes to allow queries to have one of the following:
 * EMIT CHANGES - marking it as continuous, outputting intermediate results
 * EMIT FINAL   - marking it as continuous, outputting final results
 * WITH CHANGES - marking it as static, outputting intermediate results
 * WITH FINAL   - marking it as static, outputting final results

 Persistent queries, (CSAS, CTAS + INSERT INTO), default to `EMIT CHANGES`.
 Bare queries, (SELECT * FROM X;), default to `WITH FINAL`.

 Presence of any of these is a no-op at the moment.
@big-andy-coates big-andy-coates requested a review from a team as a code owner September 4, 2019 14:51
# Conflicts:
#	ksql-parser/src/main/java/io/confluent/ksql/parser/tree/Query.java
#	ksql-parser/src/test/java/io/confluent/ksql/parser/tree/QueryTest.java
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

First pass - I think it mostly looks good. Don't love WITH but I guess that's a comment for the KLIP. I'll take another pass after this sinks in.

@agavra agavra requested a review from a team September 5, 2019 00:54
@vinothchandar
Copy link
Contributor

NOTE: this may not be the final syntax we go with, but it's good enough for now.

High level question. We are going to wait on merging this until pending issues on the KLIP are resolved?

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented Sep 6, 2019

High level question. We are going to wait on merging this until pending issues on the KLIP are resolved?

Yes, we're going to merge - I've a tight deadline I need to meet. Change keywords later is a relatively small change, but I need some language constructs to start building test cases around.

However, on reflection, I've reduced the syntax changes to the bare minimum required.

@vinothchandar
Copy link
Contributor

vinothchandar commented Sep 6, 2019

Change keywords later is a relatively small change,

As long as we are open to changing the syntax with a follow-on, it may be ok.. We still have not also resolved the concerns around breaking/non-breaking changes that @apurvam raised on the KLIP.

@big-andy-coates
Copy link
Contributor Author

@vinothchandar

We still have not also resolved the concerns around breaking/non-breaking changes that @apurvam raised on the KLIP.

I'm not aware of anything outstanding...

# Conflicts:
#	ksql-engine/src/test/java/io/confluent/ksql/codegen/CodeGenRunnerTest.java
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @big-andy-coates

@agavra agavra requested a review from a team September 9, 2019 20:21
@big-andy-coates big-andy-coates merged commit 8917e48 into confluentinc:master Sep 10, 2019
@big-andy-coates big-andy-coates deleted the static_syntax branch September 10, 2019 08:21
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.

3 participants