It seems that we should only warn the user about changes to the field type,
not define a compatibility check, or throw an exception.
*Just like this: *
*log.warn("proto.read.ProtobufSchema.uint64Field field type for uint64, was
changed into a uint32");*I will update this in the PIP issue Alternatives and discuss both designs with other people. Kiryl Valkovich <kiryl_valkovich@teal.tools> 于2023年2月20日周一 18:14写道： > 1. Sure, I didn’t mean that it's only about the required fields. > 2. I found the page you are referring to. > https://protobuf.dev/programming-guides/proto3/#updating > > QUOTE START > If a number is parsed from the wire which doesn’t fit in the corresponding > type, you will get the same effect as if you had cast the number to that > type in C++ (for example, if a 64-bit number is read as an int32, it will > be truncated to 32 bits). > QUOTE END > > It’s discussable. Maybe I’m just missing something. > > I personally wouldn’t rely on such compatibility guarantees in a real > application. > If my check amount (> int32 lol) would be truncated because someone > changed the field type in a schema, I would be quite upset. > > From: SiNan Liu <liusinan1...@gmail.com> > Date: Monday, February 20, 2023 at 2:30 AM > To: dev@pulsar.apache.org <dev@pulsar.apache.org> > Subject: Re: [DISCUSS] PIP-246: Improved PROTOBUF_NATIVE schema > compatibility checks without using avro-protobuf > Thank you for your suggestions and questions. > 1. Your first question. It's not just a matter of the required field. There > should be many differences between proto3 and proto2. I will later test the > current code for compatibility checks in proto3, and also look at > compatibility between different protobuf versions. > 2. On your second question. This is the compatibility rule for field type > changes I found on the official website. You mean that this rule should not > pass the compatibility check. Or should an exception be thrown whenever the > field type changes? > > > Kiryl Valkovich <kiryl_valkovich@teal.tools> 于 2023年2月20日周一 上午12:31写道： > > > First, thank you for looking into it! > > > > There is a thing caught my eye: > > > > > The writtenSchema cannot add required fields, … > > > > As far as I know, the required fields were removed in Protocol Buffers > v3. > > > > I see proto3 usage in existing schema registry tests: > > > > > https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-broker/src/test/proto/ProtobufSchemaTest.proto#L19 > > > > > > > https://github.com/apache/pulsar/blob/1ea4ad814f5f30b8c371db2a86626cd568ace553/pulsar-sql/presto-pulsar/src/test/java/org/apache/pulsar/sql/presto/decoder/protobufnative/TestMsg.proto#L19 > > > > I see proto2 usage in the proposed changes: > > > > > > > https://github.com/apache/pulsar/pull/19566/files#diff-f2f7463a3df6dc6366f3ee3a416707e655f0d5b8d2ae623a3f05a209c1d6ec88R19 > > > > > > > https://github.com/apache/pulsar/pull/19566/files#diff-f2f7463a3df6dc6366f3ee3a416707e655f0d5b8d2ae623a3f05a209c1d6ec88R19 > > > > Also, I’m not sure about this: > > > > > int32, uint32, int64, uint64, and bool are all compatible – this means > > you can change a field from one of these types to another without > breaking > > forwards- or backwards-compatibility. > > > > It leads to JS/PHP-like implicit conversions if I understand it right. > > > > From: SiNan Liu <liusinan1...@gmail.com> > > Date: Sunday, February 19, 2023 at 1:59 PM > > To: dev@pulsar.apache.org <dev@pulsar.apache.org> > > Subject: [DISCUSS] PIP-246: Improved PROTOBUF_NATIVE schema compatibility > > checks without using avro-protobuf > > Hi all, > > > > I made a PIP to discuss: https://github.com/apache/pulsar/issues/19565. > > > > We can talk about the current design here. Especially for the field type > > change check rules, please give your valuable advice. > > > > Thanks, > > Sinan > > >
