public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Mikhail Gribkov <[email protected]>
Cc: Arne Roland <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Permute underscore separated components of columns before fuzzy matching
Date: Fri, 17 Nov 2023 16:23:04 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAMEv5_u4mBp7qfhitvXDBQWzgeuYfkYek5kU13=O7iB1VgKXKQ@mail.gmail.com>
References: <[email protected]>
<CAMEv5_ujj7GnOWBMkq_pKtGZ4+t3N9O46ec3F2NytwqMNE_WTA@mail.gmail.com>
<FRYP281MB3146A8731F35A092B1358AE5FE30A@FRYP281MB3146.DEUP281.PROD.OUTLOOK.COM>
<CAMEv5_u4mBp7qfhitvXDBQWzgeuYfkYek5kU13=O7iB1VgKXKQ@mail.gmail.com>
Mikhail Gribkov <[email protected]> writes:
> Honestly I'm not entirely sure fixing only two switched words is worth the
> effort, but the declared goal is clearly achieved.
> I think the patch is good to go, although you need to fix code formatting.
I took a brief look at this. I concur that we shouldn't need to be
hugely concerned about the speed of this code path. However, we *do*
need to be concerned about its maintainability, and I think the patch
falls down badly there: it adds a chunk of very opaque and essentially
undocumented code, that people will need to reverse-engineer anytime
they are studying this function. That could be alleviated perhaps
with more work on comments, but I have to wonder whether it's worth
carrying this logic at all. It's a rather strange behavior to add,
and I wonder if many users will want it.
One thing that struck me is that no care is being taken for adjacent
underscores (that is, "foo__bar" and similar cases). It seems
unlikely that treating the zero-length substring between the
underscores as a word to permute is helpful; moreover, it adds
an edge case that the string-moving logic could easily get wrong.
I wonder if the code should treat any number of consecutive
underscores as a single separator. (Somewhat related: I think it
will behave oddly when the first or last character is '_', since the
outer loop ignores those positions.)
> And it would be much more convenient to work with your patch if every next
> version file will have a unique name (maybe something like "_v2", "_v3"
> etc. suffixes)
Please. It's very confusing when there are multiple identically-named
patches in a thread.
regards, tom lane
view thread (7+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Permute underscore separated components of columns before fuzzy matching
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox