Re: Doing better at HINTing an appropriate column within errorMissingColumn()

On Mon, Dec 22, 2014 at 7:34 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Dec 22, 2014 at 5:50 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Looking over the latest patch, I think we could simplify the code so
>> that you don't need multiple FuzzyAttrMatchState objects. Instead of
>> creating a separate one for each RTE and then merging them, just have
>> one. When you find an inexact-RTE name match, set a field inside the
>> FuzzyAttrMatchState -- maybe with a name like rte_penalty -- to the
>> Levenshtein distance between the RTEs. Then call scanRTEForColumn()
>> and pass down the same state object. Now let
>> updateFuzzyAttrMatchState() work out what it needs to do based on the
>> observed inter-column distance and the currently-in-force RTE penalty.
>
> I'm afraid I don't follow. I think doing things that way makes things
> less clear. Merging is useful because it allows us to consider that an
> exact match might exist, which this searchRangeTableForCol() is
> already tasked with today. We now look for the best match
> exhaustively, or magically return immediately in the event of an exact
> match, without caring about the alias correctness or distance.
>
> Having a separate object makes this pattern apparent from the top
> level, within searchRangeTableForCol(). I feel that's better.
> updateFuzzyAttrMatchState() is the wrong place to put that, because
> that task rightfully belongs in searchRangeTableForCol(), where the
> high level diagnostic-report-generating control flow lives.
>
> To put it another way, creating a separate object obfuscates
> scanRTEForColumn(), since it's the only client of
> updateFuzzyAttrMatchState(). scanRTEForColumn() is a very important
> function, and right now I am only making it slightly less clear by
> tasking it with caring about distance of names on top of strict binary
> equality of attribute names. I don't want to push it any further.

I don't buy this. What you're essentially doing is using the
FuzzyAttrMatchState object in two ways that are not entirely
compatible with each other - updateFuzzyAttrMatchState doesn't set the
RTE fields, so searchRangeTableForCol has to do it. So there's an
unspoken contract that in some parts of the code, you can rely on
those fields being set, and in others, you can't. That pretty much
defeats the whole point of making the state its own object, AFAICS.
Furthermore, you end up with two copies of the state-combining logic,
one in FuzzyAttrMatchState and a second in searchRangeTableForCol.
That's ugly and unnecessary.

I decided to rework this patch myself today; my version is attached.
I believe that this version is significantly easier to understand than
yours, both as to the code and the comments. I put quite a bit of
work into both. I also suspect it's more efficient, because it avoids
computing the Levenshtein distances for column names when we already
know that those column names can't possibly be sufficiently-good
matches for us to care about the details; and when it does compute the
Levenshtein distance it keeps the max-distance threshold as low as
possible. That may not really matter much, but it can't hurt. More
importantly, essentially all of the fuzzy-matching logic is now
isolated in FuzzyAttrMatchState(); the volume of change in
scanRTEForColumn is the same as in your version, but the volume of
change in searchRangeTableForCol is quite a bit less, so the patch is
smaller overall.

I'm prepared to commit this version if nobody finds a problem with it.
It passes the additional regression tests you wrote.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company