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