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