-
Notifications
You must be signed in to change notification settings - Fork 1k
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: support PROTOBUF keys #6692
Conversation
"ksql.key.format.enabled": true | ||
}, | ||
"statements": [ | ||
"CREATE STREAM INPUT (F1 BOOLEAN KEY, foo INT) WITH (kafka_topic='input_topic', format='PROTOBUF');", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this works with declaring the key simply as BOOLEAN
rather than STRUCT<BOOLEAN>
? I thought keys were always forced to be unwrapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are forced unwrapped if the format supports unwrapping, otherwise it uses what the format supports:
private static Optional<SerdeFeature> getKeyWrapping(
final boolean singleKey,
final Format keyFormat
) {
// Until ksqlDB supports WRAP_SINGLE_KEYS in the WITH clause, we explicitly set
// UNWRAP_SINGLE_KEYS for any key format that supports both wrapping and unwrapping to avoid
// ambiguity later:
if (singleKey
&& keyFormat.supportsFeature(SerdeFeature.UNWRAP_SINGLES)
&& keyFormat.supportsFeature(SerdeFeature.WRAP_SINGLES)
) {
return Optional.of(SerdeFeature.UNWRAP_SINGLES);
}
return Optional.empty();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting. We should add tests for converting between protobuf and other key formats, and also joins between protobuf and other key formats. The rest of this PR LGTM, though! (Feel free to add in a separate PR if that's preferable.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I'll do this in a separate PR. I think the likelihood of an org using two formats is pretty low so it's OK if we release this with some rough edges around that (but definitely good to just have it working).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though see #6692 (comment) for comment on additional test coverage. Thanks @agavra !
Description
Supports protobuf keys (still under the config flag)
Testing done
updated
protobuf.json
to have tests for protobuf keysReviewer checklist