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