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(ksql-connect): introduce syntax for CREATE CONNECTOR (syntax only) #3139

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Jul 26, 2019

Description

As part 2 of a series of PRs related to connect integration, this one adds syntax for CREATE (SOURCE | SINK) CONNECTOR. This does not wire in anything, these statements will parse but nothing more.

Testing done

Unit testing

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

@agavra agavra requested a review from a team as a code owner July 26, 2019 20:39
@@ -291,6 +292,7 @@ qualifiedName
identifier
: IDENTIFIER #unquotedIdentifier
| QUOTED_IDENTIFIER #quotedIdentifierAlternative
| STRING #stringIdentifier
Copy link
Contributor Author

@agavra agavra Jul 26, 2019

Choose a reason for hiding this comment

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

ANSI standard is to have " for identifiers and ' for constants - most DBs don't really follow this (Postgres does, but MySQL and SQL-Server don't). Since we use identifier in tableProperties it's either we allow ' or we deal with needing to do "foo"='bar' which I think is incredibly confusing, or we duplicate a bunch of code to allow ' in table properties, but not in other identifiers...

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add STRING here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the other syntax only allow for " for identifiers, not ' - I just reused STRING because it allows for '.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline here. The problem is that identifiers and literals need to be differentiated in the select statements (e.g. SELECT 'foo' and SELECT "foo" are different, one refers to literals the other refers to a column)

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.

@agavra agavra merged commit e823659 into confluentinc:master Jul 30, 2019
@agavra agavra deleted the connect_syntax branch July 30, 2019 17:00
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