daitch_mokotoff module

Please find attached an updated patch, with the following fixes:

* Replaced remaining malloc/free with palloc/pfree.
* Made "make check" pass.
* Updated notes on other implementations.

Best regards

Dag Lem

On 12/13/21 14:38, Dag Lem wrote:

Please find attached an updated patch, with the following fixes:

* Replaced remaining malloc/free with palloc/pfree.
* Made "make check" pass.
* Updated notes on other implementations.

Thanks, looks interesting. A couple generic comments, based on a quick
code review.

1) Can the extension be marked as trusted, just like fuzzystrmatch?

2) The docs really need an explanation of what the extension is for, not
just a link to fuzzystrmatch. Also, a couple examples would be helpful,
I guess - similarly to fuzzystrmatch. The last line in the docs is
annoyingly long.

3) What's daitch_mokotov_header.pl for? I mean, it generates the header,
but when do we need to run it?

4) It seems to require perl-open, which is a module we did not need
until now. Not sure how well supported it is, but maybe we can use a
more standard module?

5) Do we need to keep DM_MAIN? It seems to be meant for some kind of
testing, but our regression tests certainly don't need it (or the palloc
mockup). I suggest to get rid of it.

6) I really don't understand some of the comments in daitch_mokotov.sql,
like for example:

-- testEncodeBasic
-- Tests covered above are omitted.

Also, comments with names of Java methods seem pretty confusing. It'd be
better to actually explain what rules are the tests checking.

7) There are almost no comments in the .c file (ignoring the comment on
top). Short functions like initialize_node are probably fine without
one, but e.g. update_node would deserve one.

8) Some of the lines are pretty long (e.g. the update_node signature is
almost 170 chars). That should be wrapped. Maybe try running pgindent on
the code, that'll show which parts need better formatting (so as not to
get broken later).

9) I'm sure there's better way to get the number of valid chars than this:

for (i = 0, ilen = 0; (c = read_char(&str[i], &ilen)) && (c < 'A' ||
c > ']'); i += ilen)
{
}

Say, a while loop or something?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/13/21 09:26, Tomas Vondra wrote:

On 12/13/21 14:38, Dag Lem wrote:

Please find attached an updated patch, with the following fixes:

* Replaced remaining malloc/free with palloc/pfree.
* Made "make check" pass.
* Updated notes on other implementations.

Thanks, looks interesting. A couple generic comments, based on a quick
code review.

1) Can the extension be marked as trusted, just like fuzzystrmatch?

2) The docs really need an explanation of what the extension is for,
not just a link to fuzzystrmatch. Also, a couple examples would be
helpful, I guess - similarly to fuzzystrmatch. The last line in the
docs is annoyingly long.

It's not clear to me why we need a new module for this. Wouldn't it be
better just to add the new function to fuzzystrmatch?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 12/13/21 16:05, Andrew Dunstan wrote:

On 12/13/21 09:26, Tomas Vondra wrote:

On 12/13/21 14:38, Dag Lem wrote:

Please find attached an updated patch, with the following fixes:

* Replaced remaining malloc/free with palloc/pfree.
* Made "make check" pass.
* Updated notes on other implementations.

Thanks, looks interesting. A couple generic comments, based on a quick
code review.

1) Can the extension be marked as trusted, just like fuzzystrmatch?

2) The docs really need an explanation of what the extension is for,
not just a link to fuzzystrmatch. Also, a couple examples would be
helpful, I guess - similarly to fuzzystrmatch. The last line in the
docs is annoyingly long.

It's not clear to me why we need a new module for this. Wouldn't it be
better just to add the new function to fuzzystrmatch?

Yeah, that's a valid point. I think we're quite conservative about
adding more contrib modules, and adding a function to an existing one
works around a lot of that.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:

[...]

Thanks, looks interesting. A couple generic comments, based on a quick
code review.

Thank you for the constructive review!

1) Can the extension be marked as trusted, just like fuzzystrmatch?

I have now moved the daitch_mokotoff function into the fuzzystrmatch
module, as suggested by Andrew Dunstan.

2) The docs really need an explanation of what the extension is for,
not just a link to fuzzystrmatch. Also, a couple examples would be
helpful, I guess - similarly to fuzzystrmatch. The last line in the
docs is annoyingly long.

Please see the updated documentation for the fuzzystrmatch module.

3) What's daitch_mokotov_header.pl for? I mean, it generates the
header, but when do we need to run it?

It only has to be run if the soundex rules are changed. I have now
made the dependencies explicit in the fuzzystrmatch Makefile.

4) It seems to require perl-open, which is a module we did not need
until now. Not sure how well supported it is, but maybe we can use a
more standard module?

I believe Perl I/O layers have been part of Perl core for two decades
now :-)

5) Do we need to keep DM_MAIN? It seems to be meant for some kind of
testing, but our regression tests certainly don't need it (or the
palloc mockup). I suggest to get rid of it.

Done. BTW this was modeled after dmetaphone.c

6) I really don't understand some of the comments in
daitch_mokotov.sql, like for example:

-- testEncodeBasic
-- Tests covered above are omitted.

Also, comments with names of Java methods seem pretty confusing. It'd
be better to actually explain what rules are the tests checking.

The tests were copied from various web sites and implementations. I have
cut down on the number of tests and made the comments more to the point.

7) There are almost no comments in the .c file (ignoring the comment
on top). Short functions like initialize_node are probably fine
without one, but e.g. update_node would deserve one.

More comments are added to both the .h and the .c file.

8) Some of the lines are pretty long (e.g. the update_node signature
is almost 170 chars). That should be wrapped. Maybe try running
pgindent on the code, that'll show which parts need better formatting
(so as not to get broken later).

Fixed. I did run pgindent earlier, however it didn't catch those long
lines.

9) I'm sure there's better way to get the number of valid chars than this:

for (i = 0, ilen = 0; (c = read_char(&str[i], &ilen)) && (c < 'A' ||
c > ']'); i += ilen)
{
}

Say, a while loop or something?

The code gets to the next encodable character, skipping any other
characters. I have now added a comment which should hopefully make this
clearer, and broken up the for loop for readability.

Please find attached the revised patch.

Best regards

Dag Lem

Hello again,

It turns out that there actually exists an(other) implementation of
the Daitch-Mokotoff Soundex System which gets it right; the JOS
Soundex Calculator at https://www.jewishgen.org/jos/jossound.htm
Other implementations I have been able to find, like the one in Apache
Commons Coded used in e.g. Elasticsearch, are far from correct.

The source code for the JOS Soundex Calculator is not available, as
far as I can tell, however I have run the complete list of 98412 last
names at
https://raw.githubusercontent.com/philipperemy/name-dataset/master/names_dataset/v1/last_names.all.txt
through the calculator, in order to have a good basis for comparison.

This revealed a few shortcomings in my implementation. In particular I
had to go back to the drawing board in order to handle the dual nature
of "J" correctly. "J" can be either a vowel or a consonant in
Daitch-Mokotoff soundex, and this complicates encoding of the
*previous* character.

I have also done a more thorough review and refactoring of the code,
which should hopefully make things quite a bit more understandable to
others.

The changes are summarized as follows:

* Returns NULL for input without any encodable characters.
* Uses the same "unoffical" rules for "UE" as other implementations.
* Correctly considers "J" as either a vowel or a consonant.
* Corrected encoding for e.g. "HANNMANN".
* Code refactoring and comments for readability.
* Better examples in the documentation.

The implementation is now in correspondence with the JOS Soundex
Calculator for the 98412 last names mentioned above, with only the
following exceptions:

JOS: cedeño 430000 530000
PG: cedeño 436000 536000
JOS: sadab(khura) 437000
PG: sadab(khura) 437590

I hope this addition to the fuzzystrmatch extension module will prove
to be useful to others as well!

This is my very first code contribution to PostgreSQL, and I would be
grateful for any advice on how to proceed in order to get the patch
accepted.

Best regards

Dag Lem

On 2021-12-21 22:41:18 +0100, Dag Lem wrote:

This is my very first code contribution to PostgreSQL, and I would be
grateful for any advice on how to proceed in order to get the patch
accepted.

Currently the tests don't seem to pass on any platform:
https://cirrus-ci.com/task/5941863248035840?logs=test_world#L572
https://api.cirrus-ci.com/v1/artifact/task/5941863248035840/regress_diffs/contrib/fuzzystrmatch/regression.diffs

Greetings,

Andres Freund

On Mon, Jan 3, 2022 at 10:32 AM Andres Freund <andres@anarazel.de> wrote:

On 2021-12-21 22:41:18 +0100, Dag Lem wrote:

This is my very first code contribution to PostgreSQL, and I would be
grateful for any advice on how to proceed in order to get the patch
accepted.

Currently the tests don't seem to pass on any platform:
https://cirrus-ci.com/task/5941863248035840?logs=test_world#L572
https://api.cirrus-ci.com/v1/artifact/task/5941863248035840/regress_diffs/contrib/fuzzystrmatch/regression.diffs

Erm, it looks like something weird is happening somewhere in cfbot's
pipeline, because Dag's patch says:

+SELECT daitch_mokotoff('Straßburg');
+ daitch_mokotoff
+-----------------
+ 294795
+(1 row)

... but it's failing like:

 SELECT daitch_mokotoff('Straßburg');
  daitch_mokotoff
 -----------------
- 294795
+ 297950
 (1 row)

It's possible that I broke cfbot when upgrading to Python 3 a few
months back (ie encoding snafu when using the "requests" module to
pull patches down from the archives). I'll try to fix this soon.

Thomas Munro <thomas.munro@gmail.com> writes:

Erm, it looks like something weird is happening somewhere in cfbot's
pipeline, because Dag's patch says:

+SELECT daitch_mokotoff('Straßburg');
+ daitch_mokotoff
+-----------------
+ 294795
+(1 row)

... so, that test case is guaranteed to fail in non-UTF8 encodings,
I suppose? I wonder what the LANG environment is in that cfbot
instance.

(We do have methods for dealing with non-ASCII test cases, but
I can't see that this patch is using any of them.)

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Thomas Munro <thomas.munro@gmail.com> writes:

Erm, it looks like something weird is happening somewhere in cfbot's
pipeline, because Dag's patch says:

+SELECT daitch_mokotoff('Straßburg');
+ daitch_mokotoff
+-----------------
+ 294795
+(1 row)

... so, that test case is guaranteed to fail in non-UTF8 encodings,
I suppose? I wonder what the LANG environment is in that cfbot
instance.

(We do have methods for dealing with non-ASCII test cases, but
I can't see that this patch is using any of them.)

regards, tom lane

I naively assumed that tests would be run in an UTF8 environment.

Running "ack -l '[\x80-\xff]'" in the contrib/ directory reveals that
two other modules are using UTF8 characters in tests - citext and
unaccent.

The citext tests seem to be commented out - "Multibyte sanity
tests. Uncomment to run."

Looking into the unaccent module, I don't quite understand how it will
work with various encodings, since it doesn't seem to decode its input -
will it fail if run under anything but ASCII or UTF8?

In any case, I see that unaccent.sql starts as follows:

CREATE EXTENSION unaccent;

-- must have a UTF8 database
SELECT getdatabaseencoding();

SET client_encoding TO 'UTF8';

Would doing the same thing in fuzzystrmatch.sql fix the problem with
failing tests? Should I prepare a new patch?

Best regards

Dag Lem

Dag Lem <dag@nimrod.no> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

(We do have methods for dealing with non-ASCII test cases, but
I can't see that this patch is using any of them.)

I naively assumed that tests would be run in an UTF8 environment.

Nope, not necessarily.

Our current best practice for this is to separate out encoding-dependent
test cases into their own test script, and guard the script with an
initial test on database encoding. You can see an example in
src/test/modules/test_regex/sql/test_regex_utf8.sql
and the two associated expected-files. It's a good idea to also cover
as much as you can with pure-ASCII test cases that will run regardless
of the prevailing encoding.

Running "ack -l '[\x80-\xff]'" in the contrib/ directory reveals that
two other modules are using UTF8 characters in tests - citext and
unaccent.

Yeah, neither of those have been upgraded to said best practice.
(If you feel like doing the legwork to improve that situation,
that'd be great.)

Looking into the unaccent module, I don't quite understand how it will
work with various encodings, since it doesn't seem to decode its input -
will it fail if run under anything but ASCII or UTF8?

Its Makefile seems to be forcing the test database to use UTF8.
I think this is a less-than-best-practice choice, because then
we have zero test coverage for other encodings; but it does
prevent test failures.

regards, tom lane

Hi,

On 2022-01-02 21:41:53 -0500, Tom Lane wrote:

... so, that test case is guaranteed to fail in non-UTF8 encodings,
I suppose? I wonder what the LANG environment is in that cfbot
instance.

LANG="en_US.UTF-8"

But it looks to me like the problem is in the commit cfbot creates, rather
than the test run itself:
https://github.com/postgresql-cfbot/postgresql/commit/d5b4ec87cfd65dc08d26e1b789bd254405c90a66#diff-388d4bb360a3b24c425e29a85899315dc02f9c1dd9b9bc9aaa828876bdfea50aR56

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Hi,

On 2022-01-02 21:41:53 -0500, Tom Lane wrote:

... so, that test case is guaranteed to fail in non-UTF8 encodings,
I suppose? I wonder what the LANG environment is in that cfbot
instance.

LANG="en_US.UTF-8"

But it looks to me like the problem is in the commit cfbot creates, rather
than the test run itself:
https://github.com/postgresql-cfbot/postgresql/commit/d5b4ec87cfd65dc08d26e1b789bd254405c90a66#diff-388d4bb360a3b24c425e29a85899315dc02f9c1dd9b9bc9aaa828876bdfea50aR56

Greetings,

Andres Freund

I have now separated out the UTF8-dependent tests, hopefully according
to the current best practice (based on src/test/modules/test_regex/ and
https://www.postgresql.org/docs/14/regress-variant.html).

However I guess this won't make any difference wrt. actually running the
tests, as long as there seems to be an encoding problem in the cfbot
pipeline.

Is there anything else I can do? Could perhaps fuzzystrmatch_utf8 simply
be commented out from the Makefile for the time being?

Best regards

Dag Lem

On Wed, Jan 5, 2022 at 2:49 AM Dag Lem <dag@nimrod.no> wrote:

However I guess this won't make any difference wrt. actually running the
tests, as long as there seems to be an encoding problem in the cfbot

Fixed -- I told it to pull down patches as binary, not text. Now it
makes commits that look healthier, and so far all the Unix systems
have survived CI:

https://github.com/postgresql-cfbot/postgresql/commit/79700efc61d15c2414b8450a786951fa9308c07f
http://cfbot.cputube.org/dag-lem.html

Thomas Munro <thomas.munro@gmail.com> writes:

On Wed, Jan 5, 2022 at 2:49 AM Dag Lem <dag@nimrod.no> wrote:

However I guess this won't make any difference wrt. actually running the
tests, as long as there seems to be an encoding problem in the cfbot

Fixed -- I told it to pull down patches as binary, not text. Now it
makes commits that look healthier, and so far all the Unix systems
have survived CI:

https://github.com/postgresql-cfbot/postgresql/commit/79700efc61d15c2414b8450a786951fa9308c07f
http://cfbot.cputube.org/dag-lem.html

Great!

Dag

Tom Lane <tgl@sss.pgh.pa.us> writes:

Dag Lem <dag@nimrod.no> writes:

Running "ack -l '[\x80-\xff]'" in the contrib/ directory reveals that
two other modules are using UTF8 characters in tests - citext and
unaccent.

Yeah, neither of those have been upgraded to said best practice.
(If you feel like doing the legwork to improve that situation,
that'd be great.)

Please find attached a patch to run the previously commented-out
UTF8-dependent tests for citext, according to best practice. For now I
don't dare to touch the unaccent module, which seems to be UTF8-only
anyway.

Best regards

Dag Lem

Dag Lem <dag@nimrod.no> writes:

Please find attached a patch to run the previously commented-out
UTF8-dependent tests for citext, according to best practice. For now I
don't dare to touch the unaccent module, which seems to be UTF8-only
anyway.

I tried this on a bunch of different locale settings and concluded that
we need to restrict the locale to avoid failures: it falls over with
locale C. With that, it passes on all UTF8 LANG settings on RHEL8
and FreeBSD 12, and all except am_ET.UTF-8 on current macOS. I'm not
sure what the deal is with am_ET, but macOS has a long and sad history
of wonky UTF8 locales, so I was actually expecting worse. If the
buildfarm shows more problems, we can restrict it further --- I won't
be too upset if we end up restricting to just Linux systems, like
collate.linux.utf8. Anyway, pushed to see what happens.

regards, tom lane

Dag Lem <dag@nimrod.no> writes:

Thomas Munro <thomas.munro@gmail.com> writes:

On Wed, Jan 5, 2022 at 2:49 AM Dag Lem <dag@nimrod.no> wrote:

However I guess this won't make any difference wrt. actually running the
tests, as long as there seems to be an encoding problem in the cfbot

Fixed -- I told it to pull down patches as binary, not text. Now it
makes commits that look healthier, and so far all the Unix systems
have survived CI:

https://github.com/postgresql-cfbot/postgresql/commit/79700efc61d15c2414b8450a786951fa9308c07f
http://cfbot.cputube.org/dag-lem.html

Great!

Dag

After this I did the mistake of including a patch for citext in this
thread, which is now picked up by cfbot instead of the Daitch-Mokotoff
patch.

Attaching the original patch again in order to hopefully fix my mistake.

Best regards

Dag Lem

Hi,

Just some minor adjustments to the patch:

* Removed call to locale-dependent toupper()
* Cleaned up input normalization

I have been asked to sign up to review a commitfest patch or patches -
unfortunately I've been ill with COVID-19 and it's not until now that
I feel well enough to have a look.

Julien: I'll have a look at https://commitfest.postgresql.org/36/3468/
as you suggested (https://commitfest.postgresql.org/36/3379/ seems to
have been reviewed now).

If there are other suggestions for a patch or patches to review for
someone new to PostgreSQL internals, I'd be grateful for that.

Best regards

Dag Lem