fix: enable upsert protobuf combination #17624
Conversation
Signed-off-by: tabVersion <[email protected]>
It was not by mistake. We never implemented parsing kafka key as protobuf until we decided not to parse and always |
|
Is it still draft? Any e2e test to make sure it works? |
IIRC, we the prev behavior is fetching schema from both key and value, comparing them, and inferring the pk (aligning with upsert avro). After migrating to the additional columns, the upsert protobuf is disabled. |
I am working on e2e test. |
Before the migration, |
There was a problem hiding this comment.
Note that Flink also don't support Upsert Protobuf. https://nightlies.apache.org/flink/flink-docs-release-1.19/docs/connectors/table/formats/overview/
So this might be non-trivial
Just my guess: https://docs.confluent.io/platform/7.6/control-center/topics/schema.html#c3-schemas-best-practices-key-value-pairs
|
Signed-off-by: tabVersion <[email protected]>
Agree, but the risk is not on our side. Maybe we can comment NOT RECOMMENDED on the doc. |
| // For all Upsert formats, we only accept one and only key column as primary key. | ||
| // Additional KEY columns must be set in this case and must be primary key. | ||
| (Format::Upsert, encode @ Encode::Json | encode @ Encode::Avro) => { | ||
| ( | ||
| Format::Upsert, | ||
| encode @ Encode::Json | encode @ Encode::Avro | encode @ Encode::Protobuf, |
There was a problem hiding this comment.
Given the comment above says all Upsert formats, shall we match (Format::Upsert, _) ?
There was a problem hiding this comment.
It makes sense for all encodes other than Encode::Native but we may proceed with caution. Let's add more encodes here as requested.
Signed-off-by: tabVersion <[email protected]>
tabVersion
added
the
need-cherry-pick-release-1.10
Signed-off-by: tabVersion <[email protected]>
Signed-off-by: tabVersion <[email protected]>
Signed-off-by: tabVersion <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
as title, we disabled the combination by mistake. Now re-enabling it
related doc pr https://github.com/risingwavelabs/risingwave-docs/pull/2354
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
allow create table with
format upsert encode protobufbut do note it as NOT RECOMMENDED.according to docs.confluent.io/platform/7.6/control-center/topics/schema.html
Users may experience the upsert in wrong order.
For example, upsert_1 and upsert_2 occurs accordingly but goes into different partitions due to the partition key issue. upsert_2 may be ingested earlier than upsert_1 so the result in RisingWave can be wrong.