You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Jorge Esteban Quilcate Otoya <qu...@gmail.com> on 2022/05/15 20:50:12 UTC

Re: [DISCUSS] KIP-821: Connect Transforms support for nested structures

Thank you all for your feedback, and sorry for the long wait for a reply.

I would like to explore the idea of JSONPath-inspired/subset notation a bit
further:

It will need to be a much-reduced version of JSONPath:
- No full support for JsonPath therefore an additional dependency.
- All paths must start with '$'
- No functions support or other operators allowed.
- JsonPath dotted and square-bracket notations can be supported to avoid
escaping dots or other characters: `$a.b.c` and `$['a.b']['c']` - or we
could only support the second one as it's more complete.
- Add support for arrays with `[<integer>]`, e.g. `$a.[1].b`
- Add support for multiple-value paths using array access `$a.*.b` or deep
scan `$a..b`.
    - Some SMTs that will benefit from this: `MaskField`, `Cast`,
`ReplaceField`.
- We could introduce a `path[s]` config under the current configurations to
apply this feature so no compatibility issues are introduced: e.g.
`field.path`, `fields.paths`, `exclude.path`.

With these,  100, 101, 102, and 103 will be effectively solved.

The main challenge that I see at the moment is that by being JSON
path-like, there may be some edge cases that I can't foresee at the moment,
that could make this hard to implement, test, and maintain in the long run.

I'll appreciate your feedback on how this JSONPath-based alternative
compares to the dotted notation initially proposed, and if it matches your
feedback.
I have drafted a copy of the KIP to change the examples to JSONPath
approach and validate some differences:
https://cwiki.apache.org/confluence/x/9BihD

About 104. I agreed with Chris. We can handle this as part of a new KIP.

Thanks,
Jorge.

On Sun, 24 Apr 2022 at 17:27, Chris Egerton <fe...@gmail.com> wrote:

> Hi Joshua,
>
> I have a few reservations about using JsonPath notation here.
>
> 1. There's likely to be a substantial performance penalty for converting
> between the Kafka Connect format and something that a JsonPath library
> would understand.
>
> 2. The complexity of the feature will be significantly higher. It will be
> harder to test, document, and implement. There will be many more edge cases
> to consider and support, and we'll be on the hook to handle any bugs or
> inconsistencies that arise, either as a result of our use of the JsonPath
> library we choose, or as a result of bugs in that library.
>
> 3. It's not clear that JsonPath is superior or even suitable for some of
> the SMTs proposed here. What would be the advantages of JsonPath with the
> InsertField or HoistField SMTs?
>
> I also don't think that adding dot notation is unfriendly to users; many
> have proposed this type of syntax in the past, and it's frequently used in
> informal discussions to refer to nested fields. If the proposed syntax was
> not already intuitive then a case against deciding on our own might be more
> convincing, but as things stand, simple dot notation is likely going to be
> easier for users to understand than JsonPath syntax.
>
> Cheers,
>
> Chris
>
> On Fri, Apr 22, 2022 at 9:31 AM Joshua Grisham <gr...@gmail.com>
> wrote:
>
> > Hello all!
> >
> > Sorry that I come a bit later to the party here, but I am the one who
> wrote
> > KIP-683 [1] for recursive support (just simply looping through all child
> > non-primitive structures for the same matching name(s)) which is a
> slightly
> > different way to try and solve a similar requirement -- unfortunately at
> > the time the dev community was not quite as active and then I also got
> busy
> > with work and just life in general so wasn't able to follow up or push
> it.
> >
> > I do think it is a very good idea to have some kind of path-like
> expression
> > to be able to specifically address a nested field, as I can see that the
> > simple "recursive" case could potentially result in unwanted or
> unexpected
> > behavior, plus there is the potential to introduce a bit of a performance
> > hit to always loop through everything in cases where the schemas/values
> > might be quite large.
> >
> > One thing I wanted to ask: instead of creating a new "path parser"
> > including some bespoke or "borrowed" syntax, why not just use something
> > that already exists? Specifically here I am thinking about JsonPath (
> > https://github.com/json-path/JsonPath)
> >
> > There is already quite nice support in JsonPath for handling special
> > characters in field names, for handling different non-primitive types
> > (arrays etc), for handling multiple levels of nesting, etc etc.  Would it
> > be possible to instead to re-think this and maybe have some kind of
> > JsonPath-based Schema selector / updater and/or JsonPath-based Value
> > selector / updater? Conceptually this feels like it makes sense to me, as
> > from the top of my head it would be quite a natural fit to map a Json
> data
> > structure to the Connect API data structure (and you could potentially
> even
> > try to leverage the existing Json-to-Connect serializer/deserializer to
> > help out with this even in a more "out of the box"-feeling kind of way).
> >
> > Maybe also as Tom mentioned, this part (in my example, this
> JsonPath-based
> > "thing") could even be a generic API that could be used by any SMT,
> > including used in custom ones built by the community.  Then I think to
> use
> > a completely separate config property somehow related to "path" (as Tom
> > also mentioned) would also make a lot of sense here as well. This way, if
> > you select based on "path" then this JsonPath-based API would be used,
> > otherwise it could use something similar to the existing get-field based
> > approach (which I guess could also be refactored into some kind of
> utility
> > / API as well if it made sense?)
> >
> > And with that in mind, if this was the kind of direction to go, then a
> > "recursive" capability like I pitched in KIP-683 would also become
> > unnecessary because you could easily write a JsonPath expression like
> > "$..someRecuriveField" and it would do the same thing (on top of anything
> > else you would want to do that is already supported by JsonPath). Then we
> > could also kill that older KIP and do a bit of clean-up :)
> >
> > [1] -
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-683%3A+Add+recursive+support+to+Connect+Cast+and+ReplaceField+transforms%2C+and+support+for+casting+complex+types+to+either+a+native+or+JSON+string
> >
> > Just some extra food for thought. All-in-all I think this is a super
> great
> > initiative!
> >
> > Best,
> >
> > Joshua
> >
> >
> > Den fre 22 apr. 2022 kl 14:50 skrev Chris Egerton <
> fearthecellos@gmail.com
> > >:
> >
> > > Hi Tom,
> > >
> > > Thanks for taking a look at this, and for your thoughtful comments.
> I'll
> > > leave it up to Jorge to address most of your comments but I wanted to
> > share
> > > a couple quick thoughts I had regarding 103 and 104.
> > >
> > > 103. Like you, I was envisioning a possible syntax for array access
> that
> > > used classic C-style brackets; e.g., `arr[index]`. However, I wonder if
> > we
> > > could keep things simple and use the same syntax that we're proposing
> for
> > > nested field access? In other words, instead of `arr[index]`, you'd
> write
> > > `arr.index`. It'd save us and users the headache of reserving
> characters
> > > now that would need to be escaped even if their unescaped brethren
> aren't
> > > used for anything, and also avoid the question of what exactly we
> should
> > do
> > > when we see a config that uses reserved characters that aren't yet
> > > supported (throwing an exception seems pretty unfriendly for new
> users).
> > >
> > > 104. This would probably be useful, but it would come with some nasty
> > > compatibility questions that would need to be addressed if we'd want
> SMTs
> > > that leverage this new API to be viable for older versions of Connect.
> If
> > > we package and distribute this feature as a library (either via an
> > entirely
> > > new artifact, or as part of the existing connect-transforms or
> > connect-api
> > > artifacts), then we'd have to either sidestep the existing plugin
> > isolation
> > > logic [1] that basically makes it impossible for Connect plugins to
> ship
> > > their own versions of Connect artifacts, or issue a big warning to
> people
> > > that any SMT that uses this API won't work with any older versions of
> > > Connect. There's also some other features we might want to add in an
> > > SMT-utils library such as the existing, internal, utils that Connect
> uses
> > > right now [2]. It may be worth exploring this in a separate KIP of its
> > own.
> > >
> > > [1] -
> > >
> > >
> >
> https://github.com/apache/kafka/blob/d480c4aa6e513e36050d8e067931de2270525d18/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java#L46-L143
> > >
> > > [2] -
> > >
> > >
> >
> https://github.com/apache/kafka/tree/d480c4aa6e513e36050d8e067931de2270525d18/connect/transforms/src/main/java/org/apache/kafka/connect/transforms/util
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Fri, Apr 22, 2022 at 6:55 AM Tom Bentley <tb...@redhat.com>
> wrote:
> > >
> > > > Hi Jorge,
> > > >
> > > > Thanks for the KIP, especially for the examples which are
> super-clear.
> > > >
> > > > 100. The name `field.style` isn't so clear for something like
> > > ReplaceField:
> > > > it's not so obvious that field.style applies to `include` and
> > `exclude`.
> > > >
> > > > 101. The permitted values for `field.style` don't seem terribly
> > intuitive
> > > > (to me anyway): the meaning of `plain` isn't very guessable. Why not
> > > > `top-level` or `root` instead? Also `nested` could be misconstrued to
> > > mean
> > > > nested-but-not-top-level, so perhaps `recursive` or `cascading` might
> > be
> > > > better?
> > > >
> > > > 102. I'm torn on whether making the interpretation of existing
> configs
> > > like
> > > > `field` be dependent on `field.style` is a good idea. I can see that
> > it's
> > > > the simplest thing to do, but it just feels a bit odd that sometimes
> > the
> > > > `field` would actually be a path and have different escaping rules.
> An
> > > > alternative would be to come up with a parallel set of config names
> > (e.g.
> > > > as well as "field" an SMT might support "path") which were defined to
> > > > always take paths, thus avoiding the changeable interpretation of the
> > > > existing configs. The SMT's #configure() would need to throw in the
> > case
> > > > that both configs were given. I can see that that would be more work
> in
> > > > implementation, but it feels cleaner.
> > > >
> > > > 103. I think in order to allow for supporting arrays in a later KIP
> > > (which
> > > > certainly seems like it could be useful), we'd want to specify the
> > syntax
> > > > now, even if it wasn't implemented under this KIP. That's because I
> > don't
> > > > think you can't exclude the possibility that characters such as `[`
> and
> > > `]`
> > > > appear in field names. So you'd have a compatibility problem if
> people
> > > > started using the features of this KIP to access such fields, only
> for
> > > > those characters to change their meaning under a later KIP.
> > > >
> > > > 104. I also wonder whether making paths into a public Java API, for
> use
> > > by
> > > > 3rd party SMTs, would be valuable.
> > > >
> > > > Thanks again,
> > > >
> > > > Tom
> > > >
> > > >
> > > >
> > > > On Wed, 20 Apr 2022 at 17:53, Chris Egerton <fearthecellos@gmail.com
> >
> > > > wrote:
> > > >
> > > > > 💯 Thanks Jorge, LGTM!
> > > > >
> > > > > On Wed, Apr 20, 2022, 12:40 Jorge Esteban Quilcate Otoya <
> > > > > quilcate.jorge@gmail.com> wrote:
> > > > >
> > > > > > Thank you, Chris! Not possible without your feedback.
> > > > > >
> > > > > > On Tue, 19 Apr 2022 at 23:04, Chris Egerton <
> > fearthecellos@gmail.com
> > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Jorge,
> > > > > > >
> > > > > > > Thank you for sticking through this. I have one small remark
> and
> > > one
> > > > > > small
> > > > > > > clarification; assuming you agree with me on them then I'm
> ready
> > to
> > > > > vote
> > > > > > on
> > > > > > > the KIP.
> > > > > > >
> > > > > > > 1. InsertField: The "field.on.missing.parent" and
> > > > > > "field.on.existing.field"
> > > > > > > docs both mention a permitted value of "ingore"; this should be
> > > > > "ignore",
> > > > > > > right?
> > > > > > >
> > > > > >
> > > > > > Of course, one more typo :)
> > > > > >
> > > > > >
> > > > > > > 2. InsertField: The examples are still missing the
> "field.style"
> > > > > property
> > > > > > > from the configurations. They should all include the property
> > > > > > > "transforms.smt1.field.style": "nested", correct?
> > > > > > >
> > > > > >
> > > > > > Yes, it is there. I think I know what you mean now, seems that
> > > > Confluence
> > > > > > is putting everything in one line when it's in separate lines in
> > the
> > > > > > editor.
> > > > > > Hopefully, it's fixed now.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks again for working through this, and congratulations on a
> > > > > > > well-written KIP!
> > > > > > >
> > > > > > > Cheers,
> > > > > > >
> > > > > > > Chris
> > > > > > >
> > > > > > > On Tue, Apr 19, 2022 at 2:06 PM Jorge Esteban Quilcate Otoya <
> > > > > > > quilcate.jorge@gmail.com> wrote:
> > > > > > >
> > > > > > > > Thank you, Chris! I apply these improvements to the KIP, let
> me
> > > > know
> > > > > > how
> > > > > > > it
> > > > > > > > looks now.
> > > > > > > >
> > > > > > > > On Mon, 11 Apr 2022 at 23:43, Chris Egerton <
> > > > fearthecellos@gmail.com
> > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi Jorge,
> > > > > > > > >
> > > > > > > > > Wow, those examples are great! A few more remarks, but I
> > think
> > > > > we're
> > > > > > > > > getting close:
> > > > > > > > >
> > > > > > > > > 1. The examples differ across SMTs with the name of the
> > > > > > > newly-introduced
> > > > > > > > > style property; some of them use "field.style", and some
> use
> > > > > > > > > "fields.style". I think for consistency's sake we should
> > stick
> > > > with
> > > > > > > just
> > > > > > > > > "field.style"; otherwise it could be painful for users to
> > have
> > > to
> > > > > > > > remember
> > > > > > > > > which to use.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Great catch. Agree, I fixed the config names to
> `field.style`.
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 2. Some of the examples are off:
> > > > > > > > > - TimestampConverter: the input in the second example
> ("when
> > > > field
> > > > > > > names
> > > > > > > > > include dots") doesn't contain a field with a dotted name
> > > > > > > > > - ValueToKey: the config in the third example ("when field
> > > names
> > > > > > > include
> > > > > > > > > dots") should probably use "parent..child.k2" as the
> > > > > > > > > "transforms.smt1.fields" property
> > > > > > > > >
> > > > > > > >
> > > > > > > > Fixed. Thanks!
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 3. RE changes to InsertField:
> > > > > > > > > - The InsertField SMT should also come with the new
> > > "field.style"
> > > > > > > > property
> > > > > > > > > in order to preserve backwards compatibility, right? I
> don't
> > > see
> > > > it
> > > > > > > > > included in the example configs for that one, just want to
> > make
> > > > > sure
> > > > > > > > > - I don't know of any cases where we use snake_case for
> > > property
> > > > > > names
> > > > > > > in
> > > > > > > > > Kafka; we should probably use "on.missing.parent" and
> > > > > > > "on.existing.field"
> > > > > > > > > as the new property names for InsertField.
> > > > > > > > > - Why is the "on_existing_field" (or "on.existing.field")
> > > > property
> > > > > > only
> > > > > > > > > applied when the field style is nested? Couldn't it be
> useful
> > > for
> > > > > > > > > non-nested fields as well?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Great points! I have applied these suggestions to the KIP.
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Cheers,
> > > > > > > > >
> > > > > > > > > Chris
> > > > > > > > >
> > > > > > > > > On Sat, Apr 9, 2022 at 12:40 PM Jorge Esteban Quilcate
> Otoya
> > <
> > > > > > > > > quilcate.jorge@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > > Again, great feedback Chris. Much appreciated.
> > > > > > > > > > Added my comments below:
> > > > > > > > > >
> > > > > > > > > > On Tue, 5 Apr 2022 at 20:22, Chris Egerton <
> > > > > > fearthecellos@gmail.com>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi Jorge,
> > > > > > > > > > >
> > > > > > > > > > > Looking good! I have a few comments left but all but
> one
> > or
> > > > two
> > > > > > are
> > > > > > > > > > minor.
> > > > > > > > > > >
> > > > > > > > > > > 1. The motivation section states "This KIP is aimed to
> > > > include
> > > > > > > > support
> > > > > > > > > > for
> > > > > > > > > > > nested structures on the existing SMTs... and to
> include
> > > the
> > > > > > > > > abstractions
> > > > > > > > > > > to reuse this in future SMTs". A good implementation of
> > > this
> > > > > KIP
> > > > > > > will
> > > > > > > > > > > definitely isolate reusable logic into a separate
> > > abstraction
> > > > > > that
> > > > > > > > can
> > > > > > > > > be
> > > > > > > > > > > easily pulled in to the SMTs we want to add nested
> field
> > > > > support
> > > > > > > to,
> > > > > > > > > but
> > > > > > > > > > > unless we plan on making this kind of abstraction
> > publicly
> > > > > > > available
> > > > > > > > as
> > > > > > > > > > > some kind of utility method or class that external SMT
> > > > > developers
> > > > > > > can
> > > > > > > > > > > leverage, we can probably leave this part out as it's
> > more
> > > of
> > > > > an
> > > > > > > > > > > implementation detail.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Make sense, will leave this out of the KIP.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 2. The Cast example is a little misleading, isn't it?
> It
> > > > > > > demonstrates
> > > > > > > > > the
> > > > > > > > > > > escape syntax for fields with dot literals in their
> > names,
> > > > but
> > > > > it
> > > > > > > > > doesn't
> > > > > > > > > > > demonstrate a way to actually use the Cast (or any
> other)
> > > SMT
> > > > > to
> > > > > > > > > access a
> > > > > > > > > > > nested field in a record, which is the whole point of
> the
> > > > KIP.
> > > > > I
> > > > > > > like
> > > > > > > > > the
> > > > > > > > > > > example of escape syntax but we should probably also
> add
> > > one
> > > > > for
> > > > > > > > nested
> > > > > > > > > > > field access.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Agree. I have added examples to each SMT to be more clear
> > > about
> > > > > how
> > > > > > > it
> > > > > > > > > > affects each function
> > > > > > > > > > .
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 3. With the InsertField SMT, I'm wondering what the
> > > specific
> > > > > > > behavior
> > > > > > > > > > will
> > > > > > > > > > > be when some or all of the "middle layer" nested fields
> > are
> > > > > > > missing.
> > > > > > > > > For
> > > > > > > > > > > example, if I have a record with a value of { "k1":
> "v1 }
> > > > and I
> > > > > > > apply
> > > > > > > > > > > InsertField with topic.field = n1.n2.n3.topic, what
> will
> > > > > happen?
> > > > > > > Will
> > > > > > > > > the
> > > > > > > > > > > updated value become { "k1": "v1", "n1": { "n2": {
> "n3":
> > > > > "topic"
> > > > > > > }}},
> > > > > > > > > > will
> > > > > > > > > > > an exception be thrown, or something else? This seems
> > > > analogous
> > > > > > to
> > > > > > > > the
> > > > > > > > > > > command line mkdir command, which (at least on some
> > > operating
> > > > > > > > systems)
> > > > > > > > > > > fails by default if you try to create a new nested
> > > directory
> > > > > > where
> > > > > > > > > > anything
> > > > > > > > > > > but the last element in the path doesn't exist, but can
> > be
> > > > > > invoked
> > > > > > > > > with a
> > > > > > > > > > > flag that instructs it to go ahead and create all
> levels
> > of
> > > > > > nested
> > > > > > > > > > > directory instead. I'm leaning on the side of "just
> > create
> > > > > > > > everything"
> > > > > > > > > > but
> > > > > > > > > > > would be interested in your thoughts, and either way,
> we
> > > > should
> > > > > > > > > probably
> > > > > > > > > > > make sure the intended behavior is well-defined before
> > > > voting.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > This is an interesting case, thanks for catching this!
> > > > > > > > > > The default behavior I see is to create parents if they
> are
> > > > > missing
> > > > > > > and
> > > > > > > > > > overwrite fields
> > > > > > > > > > if they already exist.
> > > > > > > > > > I'm planning to include the following two flags if there
> > is a
> > > > > need
> > > > > > to
> > > > > > > > > > overwrite this behavior:
> > > > > > > > > > - `on_missing_parent` = (CREATE|IGNORE), default=CREATE
> > > > > > > > > > - `on_existing_field` = (OVERWRITE|IGNORE),
> > default=OVERWRITE
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 4. Similarly, what will the behavior be if any of the
> > field
> > > > > > > elements
> > > > > > > > > > > specified with InsertField already exist in the record
> > > value?
> > > > > > Will
> > > > > > > we
> > > > > > > > > > just
> > > > > > > > > > > overwrite them? What's the behavior of InsertField
> today
> > > > under
> > > > > > > > similar
> > > > > > > > > > > circumstances?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > The current behavior is to overwrite the value.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Cheers,
> > > > > > > > > > >
> > > > > > > > > > > Chris
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Mar 31, 2022 at 4:15 PM Jorge Esteban Quilcate
> > > Otoya
> > > > <
> > > > > > > > > > > quilcate.jorge@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Thanks, Chris! Much appreciated all the feedback
> here.
> > > > > > > > > > > >
> > > > > > > > > > > > 1. You nailed it setting the design goal here: "it
> > > > shouldn't
> > > > > be
> > > > > > > > > > > impossible
> > > > > > > > > > > > to use this new feature for any field name, no matter
> > how
> > > > > > > > convoluted.
> > > > > > > > > > > It's
> > > > > > > > > > > > fine if edge cases introduce difficulty (such as
> > > > > less-readable
> > > > > > > > > > > > configurations), but it's not fine if they can't be
> > > > addressed
> > > > > > at
> > > > > > > > > all."
> > > > > > > > > > > > Back to the previous proposals (using only dots as
> > > > > separators)
> > > > > > we
> > > > > > > > > have
> > > > > > > > > > 2
> > > > > > > > > > > > alternatives:
> > > > > > > > > > > > 1. escaping with backslashes
> > > > > > > > > > > > 2. escaping with dots itself
> > > > > > > > > > > >
> > > > > > > > > > > > I'll lean for alternative 2, as you proposed before.
> > > Feels
> > > > to
> > > > > > me
> > > > > > > > that
> > > > > > > > > > > > backslashes have more potential to lead to confusion
> in
> > > > JSON
> > > > > > > > configs,
> > > > > > > > > > and
> > > > > > > > > > > > CSV seems like a good precedent to use the same
> > character
> > > > to
> > > > > > > escape
> > > > > > > > > > > itself.
> > > > > > > > > > > > KIP is updated to reflect this.
> > > > > > > > > > > >
> > > > > > > > > > > > 2. Thanks! I'll add an example, and stick with the
> > > current
> > > > > > > approach
> > > > > > > > > > > > defining the style per individual transform
> > > configuration.
> > > > > > > > > > > >
> > > > > > > > > > > > 3. Yes, thanks! KIP updated.
> > > > > > > > > > > >
> > > > > > > > > > > > 4. Of course. KIP updated.
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, 28 Mar 2022 at 21:59, Chris Egerton <
> > > > > > > > fearthecellos@gmail.com
> > > > > > > > > >
> > > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > Hi Jorge,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for addressing my comments; the KIP looks
> > > > up-to-date
> > > > > > and
> > > > > > > > > > pretty
> > > > > > > > > > > > > readable now, and the rejected alternatives section
> > > does
> > > > a
> > > > > > > great
> > > > > > > > > job
> > > > > > > > > > of
> > > > > > > > > > > > > outlining the discussion so far and providing
> context
> > > for
> > > > > > > anyone
> > > > > > > > > else
> > > > > > > > > > > who
> > > > > > > > > > > > > might want to join in.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 1. Thoughts on choice of delimiter:
> > > > > > > > > > > > > - I like the optimization for simple cases, but I
> > think
> > > > the
> > > > > > new
> > > > > > > > > > > proposal
> > > > > > > > > > > > is
> > > > > > > > > > > > > a little too restrictive. What if there's a field
> > whose
> > > > > name
> > > > > > > > > contains
> > > > > > > > > > > all
> > > > > > > > > > > > > of the permitted options (currently just ".", ",",
> > and
> > > > > "/")?
> > > > > > > > > > > > > - If we expand the set of permitted delimiters to
> > allow
> > > > for
> > > > > > any
> > > > > > > > > > > > > single-character string, configuration complexity
> > will
> > > > > > increase
> > > > > > > > and
> > > > > > > > > > > > > readability may decrease
> > > > > > > > > > > > > - Also worth pointing out that there is some
> > convention
> > > > for
> > > > > > > > > doubling
> > > > > > > > > > a
> > > > > > > > > > > > > delimiter character as an escape mechanism with
> > formats
> > > > > like
> > > > > > > CSV
> > > > > > > > > [1]
> > > > > > > > > > > > > - Overall I think we may be approaching the
> > saturation
> > > > > point
> > > > > > > for
> > > > > > > > > > > > productive
> > > > > > > > > > > > > discussion on delimiter syntax so I don't want to
> > spend
> > > > too
> > > > > > > much
> > > > > > > > > more
> > > > > > > > > > > of
> > > > > > > > > > > > > your time on it. I think the one point I'd like to
> > > leave
> > > > > for
> > > > > > > now
> > > > > > > > is
> > > > > > > > > > > that
> > > > > > > > > > > > it
> > > > > > > > > > > > > shouldn't be impossible to use this new feature for
> > any
> > > > > field
> > > > > > > > name,
> > > > > > > > > > no
> > > > > > > > > > > > > matter how convoluted. It's fine if edge cases
> > > introduce
> > > > > > > > difficulty
> > > > > > > > > > > (such
> > > > > > > > > > > > > as less-readable configurations), but it's not fine
> > if
> > > > they
> > > > > > > can't
> > > > > > > > > be
> > > > > > > > > > > > > addressed at all.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 2.
> > > > > > > > > > > > > The configuration style where you define
> > > > > > > "transforms.field.style"
> > > > > > > > > in
> > > > > > > > > > > the
> > > > > > > > > > > > > connector config, and then this applies to all SMTs
> > for
> > > > the
> > > > > > > > > > connector,
> > > > > > > > > > > is
> > > > > > > > > > > > > very interesting. However, it doesn't follow
> > convention
> > > > for
> > > > > > > > > existing
> > > > > > > > > > > > SMTs.
> > > > > > > > > > > > > Right now, if you want to configure an SMT, you
> > define
> > > > its
> > > > > > name
> > > > > > > > in
> > > > > > > > > > the
> > > > > > > > > > > > > connector config (for example, "transforms":
> "smt1"),
> > > and
> > > > > > then
> > > > > > > > > define
> > > > > > > > > > > all
> > > > > > > > > > > > > of the properties for that SMT in the connector
> > config
> > > > > using
> > > > > > a
> > > > > > > > > > > > namespacing
> > > > > > > > > > > > > mechanism specific to that SMT (for example,
> > > > > > > > > "transforms.smt1.prop1":
> > > > > > > > > > > > > "val1"). That SMT then sees only the properties
> > defined
> > > > in
> > > > > > that
> > > > > > > > > > > > namespace,
> > > > > > > > > > > > > with the prefix stripped (for example, "prop1":
> > "val1")
> > > > in
> > > > > > its
> > > > > > > > > > > configure
> > > > > > > > > > > > > [2] [3] method.
> > > > > > > > > > > > > If we want to continue to follow this convention,
> > then
> > > > > > instead
> > > > > > > of
> > > > > > > > > > > > > specifying "transforms.field.style" in a connector
> > > > config,
> > > > > we
> > > > > > > > would
> > > > > > > > > > > > expect
> > > > > > > > > > > > > users to configure "transforms.<name>.field.style",
> > for
> > > > > each
> > > > > > > SMT
> > > > > > > > > that
> > > > > > > > > > > > they
> > > > > > > > > > > > > want to configure a field style for. This would
> > require
> > > > > more
> > > > > > > work
> > > > > > > > > on
> > > > > > > > > > > the
> > > > > > > > > > > > > part of the user, but would be simpler to reason
> > about
> > > > and
> > > > > > > easier
> > > > > > > > > to
> > > > > > > > > > > > > implement.
> > > > > > > > > > > > > If we want to explore an alternative where users
> can
> > > > > specify
> > > > > > > > global
> > > > > > > > > > > > > properties that apply to all transforms in a
> > connector
> > > > > > config,
> > > > > > > > then
> > > > > > > > > > the
> > > > > > > > > > > > > semantics for this need to be defined in the KIP.
> > This
> > > > > would
> > > > > > > have
> > > > > > > > > to
> > > > > > > > > > > > > include whether this will apply only for the
> special
> > > case
> > > > > of
> > > > > > > the
> > > > > > > > > > > > > "field.style" and possibly "field.separator"
> > properties
> > > > or
> > > > > if
> > > > > > > it
> > > > > > > > > > would
> > > > > > > > > > > be
> > > > > > > > > > > > > available more generally for other properties,
> > whether
> > > it
> > > > > > will
> > > > > > > > > apply
> > > > > > > > > > > only
> > > > > > > > > > > > > for the SMTs outlined in the KIP or if the
> > > "field.style"
> > > > > and
> > > > > > > > > possibly
> > > > > > > > > > > > > "field.separator" properties would also be passed
> > into
> > > > > custom
> > > > > > > > SMTs
> > > > > > > > > so
> > > > > > > > > > > > that
> > > > > > > > > > > > > they could choose to act on them if applicable, how
> > > edge
> > > > > > cases
> > > > > > > > like
> > > > > > > > > > > > having
> > > > > > > > > > > > > an SMT named "field" in your connector config would
> > be
> > > > > > handled,
> > > > > > > > > etc.
> > > > > > > > > > > > > Either way, it might help to have an example in the
> > KIP
> > > > > > > outlining
> > > > > > > > > how
> > > > > > > > > > > one
> > > > > > > > > > > > > of the to-be-augmented SMTs can be configured with
> > this
> > > > new
> > > > > > > > feature
> > > > > > > > > > > and a
> > > > > > > > > > > > > before/after of how a record value would be
> > transformed
> > > > > with
> > > > > > > that
> > > > > > > > > > > > > configuration.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 3. The docstring for the "transforms.field.style"
> > > > property
> > > > > > > > mentions
> > > > > > > > > > > that
> > > > > > > > > > > > > the permitted values are "plain" and "nested", but
> > then
> > > > > > > describes
> > > > > > > > > > > > behavior
> > > > > > > > > > > > > with a value of "root". Should that be "plain"
> > instead?
> > > > > > > > > > > > >
> > > > > > > > > > > > > 4. The docstring for the
> "transforms.field.separator"
> > > > > > property
> > > > > > > > > > > > exclusively
> > > > > > > > > > > > > mentions structs, but the feature is intended to
> work
> > > > with
> > > > > > maps
> > > > > > > > as
> > > > > > > > > > > well.
> > > > > > > > > > > > > Can we update it to reflect this?
> > > > > > > > > > > > >
> > > > > > > > > > > > > References:
> > > > > > > > > > > > >
> > > > > > > > > > > > > [1] - https://stackoverflow.com/a/17808731
> > > > > > > > > > > > > [2] -
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/kafka/blob/7243facb8d69a7252e6b9556b5eaee13e41bab7f/connect/api/src/main/java/org/apache/kafka/connect/transforms/Transformation.java#L30
> > > > > > > > > > > > > [3] -
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/kafka/blob/7243facb8d69a7252e6b9556b5eaee13e41bab7f/clients/src/main/java/org/apache/kafka/common/Configurable.java#L26-L29
> > > > > > > > > > > > >
> > > > > > > > > > > > > Cheers,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Chris
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Mar 28, 2022 at 1:32 PM Jorge Esteban
> > Quilcate
> > > > > Otoya
> > > > > > <
> > > > > > > > > > > > > quilcate.jorge@gmail.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks, Chris!
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 1. I'd argue "this..field.child" could be harder
> to
> > > > grasp
> > > > > > > than
> > > > > > > > > > > > > > "this.field/child" + separator: "/".
> > > > > > > > > > > > > > Even though this represents additional
> information,
> > > it
> > > > > > > follows
> > > > > > > > a
> > > > > > > > > > > > similar
> > > > > > > > > > > > > > approach as the "Flatten#delimeter"
> configuration.
> > > > > > > > > > > > > > I want to give the separator approach another
> try,
> > > so I
> > > > > > have
> > > > > > > > > > updated
> > > > > > > > > > > > the
> > > > > > > > > > > > > > KIP with the separator proposal, sticking to
> only 2
> > > > > > > > alternatives
> > > > > > > > > > that
> > > > > > > > > > > > > > should hopefully cover most scenarios.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 2. Agree. KIP has been updated with this
> > improvement.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 3. You're right. I have updated this section
> > > > accordingly.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 4. Good catch! I've replaced it with
> `DropHeaders`.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Looking forward to your feedback.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > Jorge.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Wed, 9 Mar 2022 at 21:33, Chris Egerton <
> > > > > > > > > > fearthecellos@gmail.com>
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi Jorge,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Looking good! Got a few more thoughts.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 1. Sorry to revisit this, but I think we may
> want
> > > to
> > > > > > adopt
> > > > > > > a
> > > > > > > > > > > slightly
> > > > > > > > > > > > > > > different escape syntax style. Backslashes are
> > > great,
> > > > > but
> > > > > > > > since
> > > > > > > > > > > > they're
> > > > > > > > > > > > > > > already used by JSON, using them as an escape
> > > > sequence
> > > > > in
> > > > > > > > field
> > > > > > > > > > > > > notation
> > > > > > > > > > > > > > > would also lead to some pretty ugly connector
> > > > configs.
> > > > > > > Anyone
> > > > > > > > > > who's
> > > > > > > > > > > > had
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > write regular expressions with backslashes in
> > Java
> > > is
> > > > > > > > probably
> > > > > > > > > > > > already
> > > > > > > > > > > > > > > familiar with this:
> > > > > > > > > "this\\\\.is\\\\.not\\\\.very\\\\.readable".
> > > > > > > > > > > What
> > > > > > > > > > > > > do
> > > > > > > > > > > > > > > you think about using the dot character to
> escape
> > > > > itself?
> > > > > > > In
> > > > > > > > > > other
> > > > > > > > > > > > > words,
> > > > > > > > > > > > > > > to access a single field named "this.field",
> > > instead
> > > > of
> > > > > > > using
> > > > > > > > > the
> > > > > > > > > > > > > syntax
> > > > > > > > > > > > > > > "this\.field" (which in JSON would have to be
> > > > expressed
> > > > > > as
> > > > > > > > > > > > > > "this\\.field"),
> > > > > > > > > > > > > > > we could use "this..field", and for a single
> > field
> > > > > named
> > > > > > > > > > > > "this\field",
> > > > > > > > > > > > > > > instead of using the syntax "this\\field" (or,
> in
> > > > JSON,
> > > > > > > > > > > > > "this\\\\field"),
> > > > > > > > > > > > > > > we could use "this\field" (or, in JSON,
> > > > "this\\field").
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 2. Could you flesh out the details on the new
> > > > > > "field.style"
> > > > > > > > > > > property,
> > > > > > > > > > > > > > > including the type, default value, importance,
> > and
> > > a
> > > > > > > > > preliminary
> > > > > > > > > > > > > > docstring?
> > > > > > > > > > > > > > > See
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-618%3A+Exactly-Once+Support+for+Source+Connectors#KIP618:ExactlyOnceSupportforSourceConnectors-Newproperties
> > > > > > > > > > > > > > > for an example.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 3. Is the "Compatibility, Deprecation, and
> > > Migration
> > > > > > Plan"
> > > > > > > > > > section
> > > > > > > > > > > > > still
> > > > > > > > > > > > > > > accurate after the latest update? Seems like
> it's
> > > > still
> > > > > > > > written
> > > > > > > > > > > with
> > > > > > > > > > > > > the
> > > > > > > > > > > > > > > assumption that nested field syntax will be
> > > hardcoded
> > > > > or
> > > > > > > > > opt-in,
> > > > > > > > > > > > which
> > > > > > > > > > > > > > IIUC
> > > > > > > > > > > > > > > isn't the case anymore.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 4. Nit: The "These SMTs do not require nested
> > > > structure
> > > > > > > > > support"
> > > > > > > > > > > > > section
> > > > > > > > > > > > > > > mentions a "Drop" SMT. I think this may be
> > > referring
> > > > to
> > > > > > the
> > > > > > > > > > > Confluent
> > > > > > > > > > > > > > Drop
> > > > > > > > > > > > > > > SMT, which isn't a part of Apache Kafka. Should
> > we
> > > > drop
> > > > > > > (heh)
> > > > > > > > > > that
> > > > > > > > > > > > SMT
> > > > > > > > > > > > > > from
> > > > > > > > > > > > > > > the list? Or perhaps just replace it with
> > > > > "DropHeaders",
> > > > > > > > which
> > > > > > > > > is
> > > > > > > > > > > > > > currently
> > > > > > > > > > > > > > > missing from the list and shouldn't require any
> > > > > > > nested-field
> > > > > > > > > > > related
> > > > > > > > > > > > > > > updates?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Cheers,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Chris
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Mon, Feb 28, 2022 at 2:12 PM Jorge Esteban
> > > > Quilcate
> > > > > > > Otoya
> > > > > > > > <
> > > > > > > > > > > > > > > quilcate.jorge@gmail.com> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Thank you, Chris! and sorry for the delayed
> > > > response.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Please, find my comments below:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Mon, 14 Feb 2022 at 17:34, Chris Egerton
> > > > > > > > > > > > > > <chrise@confluent.io.invalid
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Hi Jorge,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Thanks for the KIP! I'd love to see support
> > for
> > > > > > nested
> > > > > > > > > fields
> > > > > > > > > > > > added
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > > out-of-the-box SMTs provided with Connect.
> > Here
> > > > are
> > > > > > my
> > > > > > > > > > initial
> > > > > > > > > > > > > > > thoughts:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 1. I agree that there's a case to be made
> for
> > > > > > expanding
> > > > > > > > > > > > HoistField
> > > > > > > > > > > > > > > with a
> > > > > > > > > > > > > > > > > new config property for identifying a
> nested,
> > > > > > > > to-be-hoisted
> > > > > > > > > > > > field,
> > > > > > > > > > > > > > but
> > > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > > example in the KIP doesn't really
> demonstrate
> > > why
> > > > > > this
> > > > > > > > > would
> > > > > > > > > > be
> > > > > > > > > > > > > > > > valuable. I
> > > > > > > > > > > > > > > > > think it'd be helpful to expand the example
> > to
> > > > add
> > > > > > > other
> > > > > > > > > > fields
> > > > > > > > > > > > in
> > > > > > > > > > > > > > > order
> > > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > show how adding nested field support
> enables
> > > > users
> > > > > to
> > > > > > > > > hoist a
> > > > > > > > > > > > > nested
> > > > > > > > > > > > > > > > field
> > > > > > > > > > > > > > > > > without dropping other fields from the
> value.
> > > > Maybe
> > > > > > > > > something
> > > > > > > > > > > > like
> > > > > > > > > > > > > > > this:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >     source = nested.val
> > > > > > > > > > > > > > > > >     field = line
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >     value (before):
> > > > > > > > > > > > > > > > >         {
> > > > > > > > > > > > > > > > >             "nested": {
> > > > > > > > > > > > > > > > >                 "val": 42,
> > > > > > > > > > > > > > > > >                 "other val": 96
> > > > > > > > > > > > > > > > >             }
> > > > > > > > > > > > > > > > >         }
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >     value (after):
> > > > > > > > > > > > > > > > >         {
> > > > > > > > > > > > > > > > >             "nested": {
> > > > > > > > > > > > > > > > >                 "line": {
> > > > > > > > > > > > > > > > >                     "val": 42,
> > > > > > > > > > > > > > > > >                 }
> > > > > > > > > > > > > > > > >                 "other val": 96
> > > > > > > > > > > > > > > > >             }
> > > > > > > > > > > > > > > > >         }
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 2. Nit: I think "source" is a little
> strange
> > > for
> > > > > the
> > > > > > > new
> > > > > > > > > > > > HoistField
> > > > > > > > > > > > > > > > > property name. Maybe "hoisted" or
> > > "hoisted.field"
> > > > > > would
> > > > > > > > be
> > > > > > > > > > more
> > > > > > > > > > > > > > > > > descriptive?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > About 1. and 2.:
> > > > > > > > > > > > > > > > Agree. The example for this SMT is updated
> and
> > > have
> > > > > > added
> > > > > > > > the
> > > > > > > > > > > > > `hoisted`
> > > > > > > > > > > > > > > > configuration.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 3. Is there a reasonable use case for
> > expanding
> > > > > > Flatten
> > > > > > > > to
> > > > > > > > > be
> > > > > > > > > > > > able
> > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > flatten specific fields? My understanding
> is
> > > that
> > > > > > it's
> > > > > > > > > mostly
> > > > > > > > > > > > > useful
> > > > > > > > > > > > > > > for
> > > > > > > > > > > > > > > > > writing to systems like databases that
> don't
> > > > > support
> > > > > > > > nested
> > > > > > > > > > > > values
> > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > > require everything to be a flat list of
> > > key-value
> > > > > > > pairs.
> > > > > > > > > > Being
> > > > > > > > > > > > able
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > flatten a nested field wouldn't provide any
> > > > > advantage
> > > > > > > for
> > > > > > > > > > that
> > > > > > > > > > > > use
> > > > > > > > > > > > > > > case.
> > > > > > > > > > > > > > > > > Are there other cases where it would?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 4. I don't think we should unconditionally
> > > change
> > > > > the
> > > > > > > > > default
> > > > > > > > > > > > > > delimiter
> > > > > > > > > > > > > > > > for
> > > > > > > > > > > > > > > > > Flatten. It's a backwards-incompatible,
> > > breaking
> > > > > > change
> > > > > > > > > that
> > > > > > > > > > > > could
> > > > > > > > > > > > > > > cause
> > > > > > > > > > > > > > > > > headaches for users. It might be reasonable
> > to
> > > > > change
> > > > > > > the
> > > > > > > > > > > default
> > > > > > > > > > > > > > value
> > > > > > > > > > > > > > > > > dynamically based on whether the user has
> > > > > specified a
> > > > > > > > value
> > > > > > > > > > for
> > > > > > > > > > > > the
> > > > > > > > > > > > > > > > "field"
> > > > > > > > > > > > > > > > > property, but considering the motivation
> for
> > > > > changing
> > > > > > > the
> > > > > > > > > > > default
> > > > > > > > > > > > > is
> > > > > > > > > > > > > > > that
> > > > > > > > > > > > > > > > > it creates conflicts with the
> > to-be-introduced
> > > > > nested
> > > > > > > > field
> > > > > > > > > > > > syntax
> > > > > > > > > > > > > > > (which
> > > > > > > > > > > > > > > > > could arise with downstream SMTs regardless
> > of
> > > > > > whether
> > > > > > > > the
> > > > > > > > > > user
> > > > > > > > > > > > has
> > > > > > > > > > > > > > > > > explicitly configured Flatten with the
> > "field"
> > > > > > > > property), I
> > > > > > > > > > > don't
> > > > > > > > > > > > > > know
> > > > > > > > > > > > > > > > that
> > > > > > > > > > > > > > > > > this would be too useful either. I have
> some
> > > > > thoughts
> > > > > > > > below
> > > > > > > > > > on
> > > > > > > > > > > > how
> > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > handle possible conflicts between names
> with
> > > dots
> > > > > in
> > > > > > > > their
> > > > > > > > > > > names
> > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > dotted
> > > > > > > > > > > > > > > > > syntax for nested field references that
> > should
> > > > > > > hopefully
> > > > > > > > > make
> > > > > > > > > > > > > either
> > > > > > > > > > > > > > > > change
> > > > > > > > > > > > > > > > > unnecessary.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Fair enough. With the support for nested
> fields
> > > in
> > > > > > other
> > > > > > > > > SMTs,
> > > > > > > > > > > > > Flatten
> > > > > > > > > > > > > > > > could stay as it is.
> > > > > > > > > > > > > > > > This removes the need for (4) changing
> Flatten
> > > > config
> > > > > > as
> > > > > > > > > well.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 5. I think it's fine to expand ExtractField
> > to
> > > > > > support
> > > > > > > > > nested
> > > > > > > > > > > > > > notation,
> > > > > > > > > > > > > > > > but
> > > > > > > > > > > > > > > > > it might be worth noting in the rejected
> > > > > alternatives
> > > > > > > > > section
> > > > > > > > > > > > that
> > > > > > > > > > > > > > this
> > > > > > > > > > > > > > > > > isn't strictly necessary since you can
> > replace
> > > > any
> > > > > > > single
> > > > > > > > > > > > > invocation
> > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > > that SMT that uses nested field notation
> with
> > > > > > multiple
> > > > > > > > > > > > invocations
> > > > > > > > > > > > > of
> > > > > > > > > > > > > > > it
> > > > > > > > > > > > > > > > > that use non-nested notation.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Agree. Adding it.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 6. Nit: "RegerRouter" should be
> "RegexRouter"
> > > in
> > > > > the
> > > > > > > list
> > > > > > > > > of
> > > > > > > > > > > SMTs
> > > > > > > > > > > > > > that
> > > > > > > > > > > > > > > do
> > > > > > > > > > > > > > > > > not require nested structure support.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Ack. Fixing it.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 7. It may be rare for dots in field names
> to
> > > > occur
> > > > > in
> > > > > > > the
> > > > > > > > > > wild
> > > > > > > > > > > > > > > (although
> > > > > > > > > > > > > > > > I
> > > > > > > > > > > > > > > > > wouldn't be so certain of this), but unless
> > we
> > > > want
> > > > > > to
> > > > > > > > > > inflict
> > > > > > > > > > > > > > > headaches
> > > > > > > > > > > > > > > > on
> > > > > > > > > > > > > > > > > users of Flatten, I think we're going to
> have
> > > to
> > > > > > think
> > > > > > > > > about
> > > > > > > > > > > > > > conflicts
> > > > > > > > > > > > > > > > > between dotted notation and non-nested
> fields
> > > > whose
> > > > > > > names
> > > > > > > > > > > contain
> > > > > > > > > > > > > > > dots. I
> > > > > > > > > > > > > > > > > don't think this is actually such a bad
> > thing,
> > > > > > though.
> > > > > > > I
> > > > > > > > > > agree
> > > > > > > > > > > > that
> > > > > > > > > > > > > > > > dotted
> > > > > > > > > > > > > > > > > notation is intuitive and pretty
> commonplace
> > > (in
> > > > > > tools
> > > > > > > > like
> > > > > > > > > > jq,
> > > > > > > > > > > > for
> > > > > > > > > > > > > > > > > example), so I'd like it if we could stick
> to
> > > it.
> > > > > > What
> > > > > > > > > about
> > > > > > > > > > > > > > > introducing
> > > > > > > > > > > > > > > > > escape syntax, using a backslash? That way,
> > > users
> > > > > > could
> > > > > > > > > > > > > disambiguate
> > > > > > > > > > > > > > > > > between "this.field" (which would refer to
> > the
> > > > > nested
> > > > > > > > field
> > > > > > > > > > > > "field"
> > > > > > > > > > > > > > > under
> > > > > > > > > > > > > > > > > the top-level "this" field), and
> > "this\.field"
> > > > > (which
> > > > > > > > would
> > > > > > > > > > > refer
> > > > > > > > > > > > > to
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > > field named "this.field"). Like with most
> > > > languages
> > > > > > > that
> > > > > > > > > use
> > > > > > > > > > > the
> > > > > > > > > > > > > > > > backslash
> > > > > > > > > > > > > > > > > for escape sequences, it could also be used
> > to
> > > > > escape
> > > > > > > > > itself,
> > > > > > > > > > > in
> > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > event
> > > > > > > > > > > > > > > > > that a field name contains a backslash. I
> > think
> > > > > this
> > > > > > is
> > > > > > > > > more
> > > > > > > > > > > > > > intuitive
> > > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > > simpler than, e.g., adding a new config
> > > property
> > > > to
> > > > > > > > toggle
> > > > > > > > > > the
> > > > > > > > > > > > > > > delimiter
> > > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > be used when parsing nested field
> references.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I like this approach. Adding to the KIP.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 8. I don't think we can unconditionally
> turn
> > > this
> > > > > > > feature
> > > > > > > > > on.
> > > > > > > > > > > The
> > > > > > > > > > > > > > risk
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > > breaking existing pipelines (especially
> ones
> > > that
> > > > > > > > involve,
> > > > > > > > > > for
> > > > > > > > > > > > > > > example, a
> > > > > > > > > > > > > > > > > combination of the Flatten and Cast SMTs)
> is
> > > > pretty
> > > > > > > > high. I
> > > > > > > > > > > think
> > > > > > > > > > > > > > this
> > > > > > > > > > > > > > > > > should be an opt-in feature, at least until
> > the
> > > > > next
> > > > > > > > major
> > > > > > > > > > > > release.
> > > > > > > > > > > > > > One
> > > > > > > > > > > > > > > > way
> > > > > > > > > > > > > > > > > we could accomplish this is by introducing
> a
> > > new
> > > > > > > > > > "field.style"
> > > > > > > > > > > > > (name
> > > > > > > > > > > > > > > > > obviously subject to change) property with
> > > values
> > > > > of
> > > > > > > > > "plain"
> > > > > > > > > > > > > > (default)
> > > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > > "nested". If set to "plain" then the
> current
> > > > > > non-nested
> > > > > > > > > > > behavior
> > > > > > > > > > > > is
> > > > > > > > > > > > > > > used,
> > > > > > > > > > > > > > > > > and if set to "nested", then the proposed
> > > nested
> > > > > > > behavior
> > > > > > > > > is.
> > > > > > > > > > > We
> > > > > > > > > > > > > can
> > > > > > > > > > > > > > > > > consider updating the default value to
> > "nested"
> > > > in
> > > > > a
> > > > > > > > future
> > > > > > > > > > > major
> > > > > > > > > > > > > > > release
> > > > > > > > > > > > > > > > > (or even codify that plan in this KIP, if
> > > there's
> > > > > > > enough
> > > > > > > > > > > support
> > > > > > > > > > > > > for
> > > > > > > > > > > > > > > it).
> > > > > > > > > > > > > > > > > This would also leave the door open for
> > adding
> > > > > > > recursive
> > > > > > > > > > > support
> > > > > > > > > > > > to
> > > > > > > > > > > > > > > SMTs
> > > > > > > > > > > > > > > > in
> > > > > > > > > > > > > > > > > the future by adding a permitted value of
> > > > > > "recursive".
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 9. One of the linked tickets in the
> > > "Motivation"
> > > > > > > section,
> > > > > > > > > > > > > > > > >
> > > > https://issues.apache.org/jira/browse/KAFKA-10640,
> > > > > > has
> > > > > > > > an
> > > > > > > > > > open
> > > > > > > > > > > > PR
> > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > KIP
> > > > > > > > > > > > > > > > > that propose adding recursive support to
> some
> > > > SMTs.
> > > > > > > Have
> > > > > > > > > you
> > > > > > > > > > > > > > considered
> > > > > > > > > > > > > > > > > this type of functionality for your KIP? Or
> > is
> > > > your
> > > > > > aim
> > > > > > > > to
> > > > > > > > > > > stick
> > > > > > > > > > > > > > solely
> > > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > nested field support?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I like the `field.style` configuration flag
> > > > approach.
> > > > > > > > > > > > > > > > Thanks for pointing out the `recursive`
> > approach.
> > > > > Will
> > > > > > > add
> > > > > > > > > > > `nested`
> > > > > > > > > > > > > at
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > moment, let's check the demand for
> `recursive`
> > to
> > > > > > > consider
> > > > > > > > it
> > > > > > > > > > as
> > > > > > > > > > > > part
> > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > > this or another KIP.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I have added the following on the KIP:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > > Future KIPs could extend this support for:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > - Recursive notation: name a field and apply
> it
> > > to
> > > > > all
> > > > > > > > fields
> > > > > > > > > > > > across
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > schema matching that name.
> > > > > > > > > > > > > > > > - Access to arrays: Adding `[]` notation to
> > > > represent
> > > > > > > > access
> > > > > > > > > to
> > > > > > > > > > > > > arrays
> > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > > applying SMTs to fields within an array.
> > > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Cheers,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Chris
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On Tue, Feb 8, 2022 at 1:23 PM Jorge
> Esteban
> > > > > Quilcate
> > > > > > > > > Otoya <
> > > > > > > > > > > > > > > > > quilcate.jorge@gmail.com> wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Hi Dev team,
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I'd like to start a new discussion thread
> > on
> > > > > Kafka
> > > > > > > > > Connect
> > > > > > > > > > > > > KIP-821:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-821%3A+Connect+Transforms+support+for+nested+structures
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > This KIP is aimed to include support for
> > > nested
> > > > > > > > > structures
> > > > > > > > > > on
> > > > > > > > > > > > the
> > > > > > > > > > > > > > > > > existing
> > > > > > > > > > > > > > > > > > SMTs — where this make sense.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Looking forward to your feedback.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Regards,
> > > > > > > > > > > > > > > > > > Jorge.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>