docs: klip-33 key format

[big-andy-coates]

KLIP adding KEY_FORMAT and other things to the language.

[agavra]

LGTM!

[agavra]

+1

[blueedgenick]

Please don't eol the datagen tool!
It's far too useful for anyone who wants to test or demo something quickly. And before someone says "you can just run an embedded datagen connector though so what's the difference?" :-), allow me to opine that this requires a steeper learning curve and advanced troubleshooting skills to get working right, which I think should be avoided where possible. Just an opinion of course ;)
I'd also say that the connector should be refactored (as i recall suggesting loudly when it was originally forked off from here) so that it embeds some re-usable portion of the datagen tool (which likely requires a small refactor of datagen itself too, to facilitate, to be fair) rather than being a copy/paste that now proceeds on its own life journey and inevitable divergence.

[big-andy-coates]

@blueedgenick are you putting a case forward that DataGen should be enhanced to support Avro / Json keys as part of this work, i.e. it should be in-scaope?

[vcrfxia]

LGTM! Thanks @big-andy-coates

[big-andy-coates]

@agavra @vcrfxia I've added you back in for a review as there have been some changes, and I've found some more edge cases that need more thought.

See outstanding questions in the description at the top of this page, and also would be great if you could look at and think about the implementation section where it talks about deciding which side to repartition if key formats don't match.

[agavra]

tl;dr I think the KLIP is good whichever way you decide to go on the outstanding issues. I have some thoughts below if you need some tie-breaker on what you're intuition tells you.

---

* DataGen: Do we end-of-life our DataGen in favour of the datagen connector or update DataGen to support other key formats?

IMO, deprecating DataGen is scope creep that we should avoid if possible. If a user really wants to generate keys in other formats, I'm happy just leaving it publishing only KAFKA keys and then asking them to manually CREATE STREAM bar WITH(key_format='AVRO') AS SELECT * FROM foo . This is probably a product decision though, but I don't think we should block the KLIP on it.

* Cost based optimiser: Do we price the cost of repartitioning tables slightly less than streams? Based on the BIG assumption that streams, on average, see higher throughput than tables.

🤷 (responding to this and impl section on choosing key) Whichever decision we make will upset some people and be sub-optimal for their use cases. I think it might make sense to default to repartition the table, and then do what @mjsax suggested to repartition the stream if it's getting re-partitioned anyway. I wouldn't over-index on this because it's likely that organizations use the same data format throughout, so I suspect that this scenario will be somewhat rare.

* How to handle key-less streams? If the default key format is one that supports the schema registry, should we / can we register an empty schema? If not, how can we differentiate a missing schema, i.e. an error, from a key-less stream?

🤔 that's an interesting scenario. I think it makes sense to force keyless streams to be KAFKA format (or, we could create an alias NULL if we want to make it easier to read) and throw an error if they specify a different key format and no key columns with something like key format 'AVRO' does not support empty keys, either use key_format='NULL' or specify a (PRIMARY) KEY column

[big-andy-coates]

I wouldn't over-index on this because it's likely that organizations use the same data format throughout, so I suspect that this scenario will be somewhat rare.

Good point. We could just NOT support it...

For keyless streams, I've added a proposal to the KLIP:https://github.com/confluentinc/ksql/blob/4ca0788edb5e72537890bdb1c89628e039680320/design-proposals/klip-33-key-format.md#schema-inference. Just thinking out loud really...

The NULL key_format is probably an easier solution though! ;)

[vcrfxia]

IMO, deprecating DataGen is scope creep that we should avoid if possible. If a user really wants to generate keys in other formats, I'm happy just leaving it publishing only KAFKA keys and then asking them to manually CREATE STREAM bar WITH(key_format='AVRO') AS SELECT * FROM foo .

+1 to this. I don't think we should invest the effort to enhance datagen to support multiple key formats (and types) unless we receive user requests to do so. I'm not aware of user requests to enhance ksql-datagen to support non-string keys even though ksqlDB supports those (in Kafka format), so I'm not sure why support for different key formats would be different.

I think it makes sense to force keyless streams to be KAFKA format (or, we could create an alias NULL if we want to make it easier to read) and throw an error if they specify a different key format and no key columns with something like key format 'AVRO' does not support empty keys, either use key_format='NULL' or specify a (PRIMARY) KEY column.

I'm not a fan of forcing keyless streams to have key format KAFKA, feels rather arbitrary. I like the idea of either using key_format='NULL' or introducing syntax such as NULL KEY (where NULL is the type and no column name is provided). Naively I prefer the latter so the declaration occurs in the same place columns are typically defined, but I'm no SQL guru so perhaps I've proposed something blasphemous.

Then again, maybe key_format='NULL' is preferred since it extends more naturally to supporting empty value columns, assuming that's something we'd like to do in the future, since the NULL KEY option would require NULL VALUE to parallel it, which would be new syntax (since we don't have VALUE today).

[PeterLindner]

@big-andy-coates I remember reading a discussion that different versions of the same Avro schema are byte incompatible. May be worth adding compatibility implications of schema evolution to this Klip (especially for joins)

[MichaelDrogalis]

Do we really need to introduce ksql.persistence.default.format.key? I have two concerns:

1. If I understand right, unless someone configures their server with this property, relaunching queries that worked in a previous version will no longer work. I get why we need to have something like this, but could we not just make KEY_FORMAT default to KAFKA and deprecate it being optional a few releases down the road? We're dinging people with a lot of breaking changes in the last few releases—this one seems avoidable.

2. Similar to KLIP-34, the value of these types of server configs isn't apparent. I'm not sure anyone to date has asked for something like that.

[MichaelDrogalis]

@big-andy-coates Ah, I misread—my fault. I still think the server config is overkill, but I don't feel that strongly about that.

[PeterLindner]

@big-andy-coates not relevant for this Klip, but would it make sense to create a new schema registry compatibility level that does not allow evolution? Otherwise another application external to ksqldb could try to evolve the key schema and break things unitentionally

[big-andy-coates]

@big-andy-coates not relevant for this Klip, but would it make sense to create a new schema registry compatibility level that does not allow evolution? Otherwise another application external to ksqldb could try to evolve the key schema and break things unitentionally

@PeterLindner, yeah, it may be possible to have a compatibility level that just says "don't allow evolution", which would be useful for key schemas. confluentinc/schema-registry#1610

[big-andy-coates]

@MichaelDrogalis @derekjn @colinhicks engineers have now approved this. Can I get product approval please?

[JimGalasyn]

LGTM!

[agavra]

Skimmed what seemed new, still LGTM

[colinhicks]

LGTM. Thanks for the thorough and thoughtful discussion here, @big-andy-coates.

I left a few nits as suggestions for readability below.

[vcrfxia]

Latest updates LGTM!

[vcrfxia]

Add note clarifying that users can always repartition topics themselves before the join, in order to have full control over which sources are repartitioned (and that choosing the same sources ksqlDB would repartition and performing the repartitions upfront is equivalent from a resource-usage standpoint)? Or if not here, at least in the docs section so we don't forget to add the note later.