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: drop WITH(KEY) syntax #5363

Merged
merged 21 commits into from
May 18, 2020

Conversation

big-andy-coates
Copy link
Contributor

Description

fixes: #3537

implements: See KLIP-25

This change removes the WITH(KEY) syntax which previously allowed users to specify a value column that could act as an alias for the key column. This allowed a more user friendly name to be used for the key column, at the expense of requiring a copy of the key data in the value.

With the new 'any key name' feature, the key columns themselves can be given appropriate names, removing the need for this aliasing functionality. See KLIP-25 for more details.

Reviewing notes:

Commits split to give:

  1. Prod code changes - which is mainly just removing the KeyField stuff - deleting code!
  2. Test code changes - a lot of tests to change. Needed to change TestDataProvider to not rely on key fields too, which meant changing the schema. Also MetastoreFixture changed to not used key fields.
  3. Test historical test plans - new test plans for any changed tests.
  4. Docs - updated docs @JimGalasyn, rather than 10s of suggested changes I need to apply (generally blindly), how do you feel about making any changes in a follow up PR that I can then review?

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

fixes: confluentinc#3537

implements: See [KLIP-25](https://github.com/confluentinc/ksql/blob/master/design-proposals/klip-25-removal-of-with-key-syntax.md)

This change removes the `WITH(KEY)` syntax which previously allowed users to specify a value column that could act as an alias for the key column. This allowed a more user friendly name to be used for the key column, at the expense of requiring a copy of the key data in the value.

With the new 'any key name' feature, the key columns themselves can be given appropriate names, removing the need for this aliasing functionality.  See [KLIP-25](https://github.com/confluentinc/ksql/blob/master/design-proposals/klip-25-removal-of-with-key-syntax.md) for more details.
@big-andy-coates big-andy-coates requested review from JimGalasyn and a team as code owners May 14, 2020 15:58
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

Awesome! Ignore my edits, I'll get them in a separate PR, as you suggested.

big-andy-coates and others added 11 commits May 14, 2020 20:28
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
big-andy-coates and others added 3 commits May 14, 2020 20:30
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Conflicting files
ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/integration/LagReportingAgentFunctionalTest.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.

So many red lines! LGTM, mostly a quick scan of the tests and prod code

Conflicting files
ksqldb-rest-app/src/test/java/io/confluent/ksql/api/integration/ApiIntegrationTest.java
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.

Primitive Keys: Remove WITH(KEY='z')
3 participants