Hi all, Any other suggestions/objections/dubs?
On Fri, May 10, 2024 at 5:10 PM Chris Egerton <fearthecel...@gmail.com> wrote: > 👍 Done > > On Fri, May 10, 2024, 10:55 Mario Fiore Vitale <mvit...@redhat.com> wrote: > > > Thanks a lot! I have just a minor comment, should we also update the > title > > to be more generic since now it's also related to other SMTs? > > > > On Fri, May 10, 2024 at 4:44 PM Chris Egerton <chr...@aiven.io.invalid> > > wrote: > > > > > I've finished updating the KIP; @Mario, please let me know what you > > think. > > > > > > On Fri, May 10, 2024 at 10:26 AM Chris Egerton <chr...@aiven.io> > wrote: > > > > > > > I can do it :) > > > > > > > > On Fri, May 10, 2024 at 10:02 AM Mario Fiore Vitale < > > mvit...@redhat.com> > > > > wrote: > > > > > > > >> Yes, I agree. Unfortunately due to the issue of the creation of a > new > > > >> account for the WIKI, I asked Mickael Maison to create the KIP for > me. > > > >> > > > >> I'll ask him to update the KIP. Do you have other alternatives? > > > >> > > > >> Thanks, > > > >> Mario. > > > >> > > > >> On Fri, May 10, 2024 at 3:40 PM Chris Egerton > <chr...@aiven.io.invalid > > > > > > >> wrote: > > > >> > > > >> > Yes, I think we should just do one KIP for all the SMTs. You don't > > > have > > > >> to > > > >> > implement everything all at once or by yourself, but I don't see > why > > > we > > > >> > should require one or more follow-up KIPs to apply the exact same > > > >> changes > > > >> > to the SMTs we missed the first time. > > > >> > > > > >> > On Fri, May 10, 2024 at 5:20 AM Mario Fiore Vitale < > > > mvit...@redhat.com> > > > >> > wrote: > > > >> > > > > >> > > Hi Chris, > > > >> > > > > > >> > > Thanks for the survey. Do you think I need to update the KIP to > > put > > > >> all > > > >> > of > > > >> > > these? > > > >> > > > > > >> > > On Thu, May 9, 2024 at 4:14 PM Chris Egerton > > > <chr...@aiven.io.invalid > > > >> > > > > >> > > wrote: > > > >> > > > > > >> > > > After doing a brief survey of the SMTs that ship with Connect, > > it > > > >> seems > > > >> > > > like these would also benefit: > > > >> > > > > > > >> > > > - HeaderFrom, which populates record headers with subfields of > > > >> > > keys/values > > > >> > > > [1] > > > >> > > > - Cast, which performs type transformation on subfields of > > > >> keys/values > > > >> > > [2] > > > >> > > > - SetSchemaMetadata, which (when the record key/value is a > > struct) > > > >> > copies > > > >> > > > fields from the input struct to the output struct (which uses > a > > > >> > different > > > >> > > > schema) [3] > > > >> > > > - TimestampConverter, which does similar input/output field > > > copying > > > >> to > > > >> > > > SetSchemaMetadata [4] > > > >> > > > - ReplaceField, which does similar input/output field copying > to > > > >> > > > SetSchemaMetadata and TimestampConverter > > > >> > > > > > > >> > > > [1] - > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f4fdaa702a2e718bdb44b9c5fec254f32a33f0d8/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/HeaderFrom.java#L143 > > > >> > > > [2] - > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f4fdaa702a2e718bdb44b9c5fec254f32a33f0d8/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/Cast.java#L178 > > > >> > > > [3] - > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f4fdaa702a2e718bdb44b9c5fec254f32a33f0d8/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/SetSchemaMetadata.java#L174 > > > >> > > > [4] - > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/2a5efe4a334611fc7c15f76b322482bad0942db0/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/TimestampConverter.java#L420 > > > >> > > > [5] - > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f4fdaa702a2e718bdb44b9c5fec254f32a33f0d8/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/ReplaceField.java#L183 > > > >> > > > > > > >> > > > On Thu, May 9, 2024 at 3:28 AM Mario Fiore Vitale < > > > >> mvit...@redhat.com> > > > >> > > > wrote: > > > >> > > > > > > >> > > > > Hi Chris, > > > >> > > > > > > > >> > > > > > Wouldn't ValueToKey [1] be applicable as well, for > example? > > > >> > > > > Yes, also that one can be affected. > > > >> > > > > > > > >> > > > > On Wed, May 8, 2024 at 5:59 PM Chris Egerton > > > >> <chr...@aiven.io.invalid > > > >> > > > > > >> > > > > wrote: > > > >> > > > > > > > >> > > > > > Wait, just one more thing--are there any other SMTs that > > could > > > >> > > benefit > > > >> > > > > from > > > >> > > > > > this? Wouldn't ValueToKey [1] be applicable as well, for > > > >> example? > > > >> > > > > > > > > >> > > > > > [1] - > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/2a5efe4a334611fc7c15f76b322482bad0942db0/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/ValueToKey.java#L106 > > > >> > > > > > > > > >> > > > > > On Wed, May 8, 2024 at 11:46 AM Chris Egerton < > > > chr...@aiven.io> > > > >> > > wrote: > > > >> > > > > > > > > >> > > > > > > Hi Mario, > > > >> > > > > > > > > > >> > > > > > > I think we could have something like `copy` and > > > >> > > `copyWithoutDefaults` > > > >> > > > > to > > > >> > > > > > > get around that, but now that you bring up > compatibility, > > I > > > >> think > > > >> > > > it's > > > >> > > > > > best > > > >> > > > > > > to hold off on this. I'm forced to recall that anything > we > > > >> add to > > > >> > > the > > > >> > > > > > > Connect API may be used by plugin developers who write > for > > > the > > > >> > > > bleeding > > > >> > > > > > > edge of the Connect runtime, but deployed by users who > are > > > >> > running > > > >> > > on > > > >> > > > > > > (possibly much) older versions. In that scenario, any > use > > of > > > >> new > > > >> > > > Struct > > > >> > > > > > > methods would cause issues at runtime caused by > > > compatibility > > > >> > > clashes > > > >> > > > > > > between the newer API that the plugin was written for, > and > > > the > > > >> > > older > > > >> > > > > API > > > >> > > > > > > that's provided by the runtime it's running on. > > > >> > > > > > > > > > >> > > > > > > Anyway, thanks for humoring me. The KIP looks good to me > > 👍 > > > >> > > > > > > > > > >> > > > > > > Cheers, > > > >> > > > > > > > > > >> > > > > > > Chris > > > >> > > > > > > > > > >> > > > > > > On Wed, May 8, 2024 at 10:50 AM Mario Fiore Vitale < > > > >> > > > mvit...@redhat.com > > > >> > > > > > > > > >> > > > > > > wrote: > > > >> > > > > > > > > > >> > > > > > >> Hi Chris, > > > >> > > > > > >> > > > >> > > > > > >> Thanks for reviewing this. > > > >> > > > > > >> > > > >> > > > > > >> > It seems like the pattern of "copy the contents of > this > > > >> Struct > > > >> > > > into > > > >> > > > > > >> another > > > >> > > > > > >> one for the purpose of mutation" could be fairly common > > in > > > >> user > > > >> > > code > > > >> > > > > > bases > > > >> > > > > > >> in addition to the core Connect SMTs. Do you think > > there's > > > a > > > >> way > > > >> > > to > > > >> > > > > > >> simplify this with, e.g., a Struct.putAll(Struct > > > >> destination) or > > > >> > > > > > >> Struct.copy(Schema destinationSchema) method? > > > >> > > > > > >> > > > >> > > > > > >> The only concern that I see is backward compatibility. > > > >> Suppose > > > >> > > that > > > >> > > > > you > > > >> > > > > > >> are > > > >> > > > > > >> not using the JsonConvert but another convert that > does't > > > >> > support > > > >> > > > the > > > >> > > > > > >> 'replace.null.with.default', when you use the current > > > >> > > 'InsertField' > > > >> > > > > smt > > > >> > > > > > >> the null values will be replace by default values. If > we > > > >> replace > > > >> > > the > > > >> > > > > > >> "copy" > > > >> > > > > > >> logic with a method in the Struct we remove this > > behavior. > > > >> > > > > > >> > > > >> > > > > > >> Isn't it? > > > >> > > > > > >> > > > >> > > > > > >> Mario. > > > >> > > > > > >> > > > >> > > > > > >> On Wed, May 8, 2024 at 2:14 PM Chris Egerton > > > >> > > > <chr...@aiven.io.invalid > > > >> > > > > > > > > >> > > > > > >> wrote: > > > >> > > > > > >> > > > >> > > > > > >> > Hi Mario, > > > >> > > > > > >> > > > > >> > > > > > >> > Thanks for the KIP! Looks good overall. One quick > > > >> thought--it > > > >> > > > wasn't > > > >> > > > > > >> > immediately obvious to me why we had to touch on > > > >> InsertField > > > >> > for > > > >> > > > > this. > > > >> > > > > > >> It > > > >> > > > > > >> > looks like the reason is that we use Struct::get [1] > to > > > >> > create a > > > >> > > > > clone > > > >> > > > > > >> of > > > >> > > > > > >> > the input value [2], instead of > > Struct::getWithoutDefault > > > >> [3]. > > > >> > > > > > >> > > > > >> > > > > > >> > It seems like the pattern of "copy the contents of > this > > > >> Struct > > > >> > > > into > > > >> > > > > > >> another > > > >> > > > > > >> > one for the purpose of mutation" could be fairly > common > > > in > > > >> > user > > > >> > > > code > > > >> > > > > > >> bases > > > >> > > > > > >> > in addition to the core Connect SMTs. Do you think > > > there's > > > >> a > > > >> > way > > > >> > > > to > > > >> > > > > > >> > simplify this with, e.g., a Struct.putAll(Struct > > > >> destination) > > > >> > or > > > >> > > > > > >> > Struct.copy(Schema destinationSchema) method? > > > >> > > > > > >> > > > > >> > > > > > >> > [1] - > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f74f596bc7d35fcea97edcd83244e5d6aee308d2/connect/api/src/main/java/org/apache/kafka/connect/data/Struct.java#L78-L91 > > > >> > > > > > >> > [2] - > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f74f596bc7d35fcea97edcd83244e5d6aee308d2/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/InsertField.java#L179-L183 > > > >> > > > > > >> > [3] - > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://github.com/apache/kafka/blob/f74f596bc7d35fcea97edcd83244e5d6aee308d2/connect/api/src/main/java/org/apache/kafka/connect/data/Struct.java#L93-L101 > > > >> > > > > > >> > > > > >> > > > > > >> > Cheers, > > > >> > > > > > >> > > > > >> > > > > > >> > Chris > > > >> > > > > > >> > > > > >> > > > > > >> > On Wed, May 8, 2024 at 3:40 AM Mario Fiore Vitale < > > > >> > > > > mvit...@redhat.com > > > >> > > > > > > > > > >> > > > > > >> > wrote: > > > >> > > > > > >> > > > > >> > > > > > >> > > Hi All, > > > >> > > > > > >> > > > > > >> > > > > > >> > > I have created (through Mickael Maison's account > > since > > > >> there > > > >> > > was > > > >> > > > > an > > > >> > > > > > >> issue > > > >> > > > > > >> > > creating a new one for me) KIP-1040[1] to improve > > > >> handling > > > >> > of > > > >> > > > > > nullable > > > >> > > > > > >> > > values in InsertField/ExtractField transformations, > > > this > > > >> is > > > >> > > > > required > > > >> > > > > > >> > after > > > >> > > > > > >> > > the introduction of KIP-581[2] that introduced the > > > >> > > configuration > > > >> > > > > > >> property > > > >> > > > > > >> > > *replace.null.with.default* to *JsonConverter* to > > > choose > > > >> > > whether > > > >> > > > > to > > > >> > > > > > >> > replace > > > >> > > > > > >> > > fields that have a default value and that are null > to > > > the > > > >> > > > default > > > >> > > > > > >> value. > > > >> > > > > > >> > > > > > >> > > > > > >> > > [1] > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=303794677 > > > >> > > > > > >> > > [2] > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-581%3A+Value+of+optional+null+field+which+has+default+value > > > >> > > > > > >> > > > > > >> > > > > > >> > > Feedback and suggestions are welcome. > > > >> > > > > > >> > > > > > >> > > > > > >> > > Regards, > > > >> > > > > > >> > > Mario. > > > >> > > > > > >> > > -- > > > >> > > > > > >> > > > > > >> > > > > > >> > > Mario Fiore Vitale > > > >> > > > > > >> > > > > > >> > > > > > >> > > Senior Software Engineer > > > >> > > > > > >> > > > > > >> > > > > > >> > > Red Hat <https://www.redhat.com/> > > > >> > > > > > >> > > <https://www.redhat.com/> > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> -- > > > >> > > > > > >> > > > >> > > > > > >> Mario Fiore Vitale > > > >> > > > > > >> > > > >> > > > > > >> Senior Software Engineer > > > >> > > > > > >> > > > >> > > > > > >> Red Hat <https://www.redhat.com/> > > > >> > > > > > >> <https://www.redhat.com/> > > > >> > > > > > >> > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > -- > > > >> > > > > > > > >> > > > > Mario Fiore Vitale > > > >> > > > > > > > >> > > > > Senior Software Engineer > > > >> > > > > > > > >> > > > > Red Hat <https://www.redhat.com/> > > > >> > > > > <https://www.redhat.com/> > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > -- > > > >> > > > > > >> > > Mario Fiore Vitale > > > >> > > > > > >> > > Senior Software Engineer > > > >> > > > > > >> > > Red Hat <https://www.redhat.com/> > > > >> > > <https://www.redhat.com/> > > > >> > > > > > >> > > > > >> > > > >> > > > >> -- > > > >> > > > >> Mario Fiore Vitale > > > >> > > > >> Senior Software Engineer > > > >> > > > >> Red Hat <https://www.redhat.com/> > > > >> <https://www.redhat.com/> > > > >> > > > > > > > > > > > > > -- > > > > Mario Fiore Vitale > > > > Senior Software Engineer > > > > Red Hat <https://www.redhat.com/> > > <https://www.redhat.com/> > > > -- Mario Fiore Vitale Senior Software Engineer Red Hat <https://www.redhat.com/> <https://www.redhat.com/>
