On Wed, Jul 9, 2014 at 1:49 AM, Peter Geoghegan <pg@heroku.com> wrote:
> 4) This is not nice, could it be possible to remove the stuff from varlena.c?
> +/* Expand each Levenshtein distance variant */ > +#include "levenshtein.c" > +#define LEVENSHTEIN_LESS_EQUAL > +#include "levenshtein.c" > +#undef LEVENSHTEIN_LESS_EQUAL > Part of the same comment: only varstr_leven_less_equal is used to > calculate the distance, should we really move varstr_leven to core? > This clearly needs to be reworked as not just a copy-paste of the > things in fuzzystrmatch. > The flag LEVENSHTEIN_LESS_EQUAL should be let within fuzzystrmatch I think.
So there'd be one variant within core and one within contrib/fuzzystrmatch? I don't think that's an improvement.
No. The main difference between varstr_leven_less_equal and varstr_leven is the use of the extra argument max_d in the former. My argument here is instead of blindly cut-pasting into core the code you are interested in to evaluate the string distances, is to refactor it to have a unique function, and to let the business with LEVENSHTEIN_LESS_EQUAL within contrib/fuzzystrmatch. This will require some reshuffling of the distance function, but by looking at this patch I am getting the feeling that this is necessary, and should even be split into a first patch for fuzzystrmatch that would facilitate its integration into core. Also why is rest_of_char_same within varlena.c?
> 5) Do we want hints on system columns as well?
I think it's obvious that the answer must be no. That's going to frequently result in suggestions of columns that users will complain aren't even there. If you know about the system columns, you can just get it right. They're supposed to be hidden for most purposes.
This may sound ridiculous, but I have already found myself mistyping ctid by tid and cid while working on patches and modules that played with page format, and needing a couple of minutes to understand what was going on (bad morning). I would have welcomed such hints in those cases. -- Michael
