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: expose support for array and struct keys #6722

Merged
merged 9 commits into from
Dec 7, 2020

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Dec 4, 2020

Description

This PR adds more test coverage for array and struct keys, and exposes the feature to the world (by removing the relevant feature flag) 🎉 (partial progress towards #824)

Four commits for ease of reviewing:

  • additional QTT test coverage
  • adds integration test coverage, via the HTTP2 API and Java client tests
  • removes the feature flag
  • adds QTT historic plans

Testing done

See above -- lots of new tests!

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

@vcrfxia vcrfxia requested a review from a team as a code owner December 4, 2020 21:03
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.

Thanks for all the added test coverage @vcrfxia!

}
for (final SimpleColumn column : columns) {
if (containsMapType(column.type())) {
throw new KsqlException("Map keys, including types that contain maps, are not supported "
Copy link
Contributor

Choose a reason for hiding this comment

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

(from a previous PR) we might want to link to #6621 in the exception of the message if anyone needs to "continue the conversation"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, must've missed the previous comment. Updated in this PR. I went with simply See https://github.com/confluentinc/ksql/issues/6621 for more. rather than language about "continuing the conversation" since the latter makes it sound like this is an ongoing discussion which it isn't (at least as of now).

Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry my comment wasn't clear! I meant (this is code from a previous PR) not (I left a comment on a previous PR). And definitely don't think we should include "continue the conversation" in the exception message! I meant it as that's what they'd be doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got'cha. Glad we're on the same page :)

@vcrfxia vcrfxia merged commit c7fc2b0 into confluentinc:master Dec 7, 2020
@vcrfxia vcrfxia deleted the complex-key-tests branch December 7, 2020 21:49
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