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/> >> >
