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

chore: remove Connect schema from Format interface #4637

Merged

Conversation

big-andy-coates
Copy link
Contributor

Description

As this interface is moving towards being a pluggable public interface.

Now, the Format interface deals with converting SchemaRegistry ParsedSchemas into the list of SimpleColumn ksql should use. Each SimpleColumn defines the name and type of the column.

Conversion between connect and ksql types is now handled by the ConnectFormat base class. This base class currently converts all field names to uppercase. However, it is now possible for a format to return any-case names and those names will be respected.

Testing done

usual

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

As this interface is moving towards being a pluggable public interface.

Now, the `Format` interface deals with converting SchemaRegistry `ParsedSchema`s into the list of `SimpleColumn` ksql should use. Each `SimpleColumn` defines the name and type of the column.

Conversion between connect and ksql types is now handled by the `ConnectFormat` base class.  This base class currently converts all field names to uppercase. However, it is now possible for a format to return any-case names and those names will be respected.
@big-andy-coates big-andy-coates requested a review from a team as a code owner February 26, 2020 13:22
@agavra agavra self-assigned this Feb 26, 2020
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! I like this change quite a bit :) reduces the surface area of what's exposed if we ever make this a public interface.

I'm not sure I understand why we've added ensureNamed though, probably something basic I'm missing.

Conflicting files
ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestCaseBuilderUtil.java
ksql-functional-tests/src/main/java/io/confluent/ksql/test/utils/SerdeUtil.java
ksql-serde/src/main/java/io/confluent/ksql/serde/json/JsonFormat.java
@big-andy-coates big-andy-coates merged commit 766ffe1 into confluentinc:master Feb 27, 2020
@big-andy-coates big-andy-coates deleted the no_connect_in_format branch February 27, 2020 15:52
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