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