builtins: add fuzzystrmatch metaphone() built-in #110950

charlespnh · 2023-09-20T05:33:22Z

informs: #56820

Release note (sql change): The metaphone() builtin function was added.

cockroach-teamcity · 2023-09-20T05:33:26Z

All committers have signed the CLA.

blathers-crl · 2023-09-20T05:33:26Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

@fqazi (commented on builtins: support fuzzystrmatch builtins #56820)
@jordanlewis (author of builtins: support fuzzystrmatch builtins #56820)
@otan (commented on builtins: support fuzzystrmatch builtins #56820)
@rafiss (commented on builtins: support fuzzystrmatch builtins #56820)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-09-20T05:33:32Z

This change is

blathers-crl · 2023-09-27T07:05:35Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

rafiss

Thanks for your contribution! can you please add tests for this in pkg/sql/logictest/testdata/logic_test/fuzzystrmatch ?

blathers-crl · 2023-11-23T01:46:26Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-23T04:54:39Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-23T05:59:21Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-23T06:59:15Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-23T22:08:10Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-25T16:18:11Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-25T16:26:42Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-25T16:31:58Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2023-11-25T17:54:18Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

charlespnh · 2023-11-26T16:19:36Z

@rafiss Sorry for the delay... I intend to add tests but the built-in is a bit tricky to implement with all the rules. The PR was a draft since it was still a work in progress, but commits have been squashed and it's ready for review now.
It seems like Postgres doesn't have many tests for metaphone. Please let me know if there are any other meaningful tests I should have added

Both metaphone(...)'s behaviour and interface are intended to match with Postgres. With all these if/else-if rules, the implementation focuses on readability as much as possible as well.

yuzefovich · 2023-12-26T17:00:38Z

Friendly ping @rafiss

rafiss

Thanks for your continued work on this @charlespnh!

I found a few more test cases from looking at other open-source implementations of metaphone. There are a few where this PR differs:

metaphone('Campbel', 10) -> expected "KMPBL" got "MBL"
metaphone('Cammmppppbbbeeelll', 10) -> expected "KMPBL" got "M"
metaphone('What', 10) -> expected "WT" got "HT"
metaphone('astronomical', 10) -> expected "ASTRNMKL" got "ASTRNML"
metaphone('district', 10) -> expected "TSTRKT" got "TSTR"
metaphone('hockey', 10) -> expected "HK" got "H"
metaphone('capital', 10) -> expected "KPTL" got "PTL"
metaphone('lightning', 10) -> expected "LTNNK" got "LFTK"
metaphone('light', 10) -> expected "LT" got "LFT"

I have added these test cases to your PR, but I will leave it to you to fix the remaining bugs, if you're still interested in working on this. Consider using other implementations as a reference (and cite them in comments if you do so). see https://github.com/go-dedup/metaphoneand and https://github.com/vividvilla/metaphone

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @jordanlewis, and @michae2)

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @charlespnh, @fqazi, @jordanlewis, and @michae2)

pkg/util/fuzzystrmatch/metaphone_test.go line 90 at r5 (raw file):

		{
			Source:   "What",
			Expected: "HT",

postgres returns WT for this test case

pkg/util/fuzzystrmatch/metaphone_test.go line 142 at r5 (raw file):

		{
			Source:   "lightning",
			Expected: "LFTNNK",

postgres returns LTNNK for this case

pkg/util/fuzzystrmatch/metaphone_test.go line 146 at r5 (raw file):

		{
			Source:   "light",
			Expected: "LFT",

postgres returns LT for this case

charlespnh

Thanks for helping me out! I'm using PostgreSQL 12.17 and this is what I get though:

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @jordanlewis, and @michae2)

rafiss

thanks for clarifying. i must have been checking against a different implementation; i confirmed that i see the same postgres behavior now.

just had one more nit

pkg/util/fuzzystrmatch/metaphone.go

Release note (sql change): metaphone() builtin function was added.

rafiss

thanks for your work on this!

bors r+

craig · 2024-01-10T04:45:04Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Sep 20, 2023

blathers-crl bot requested review from fqazi, jordanlewis, otan and rafiss September 20, 2023 05:33

otan removed their request for review September 21, 2023 23:13

charlespnh changed the title ~~builtins: add fuzzystrmatch metaphone built-in function~~ builtins: add fuzzystrmatch metaphone() built-in Sep 27, 2023

charlespnh force-pushed the chrls_metaphone branch from 867c854 to 0885b00 Compare September 27, 2023 07:05

jordanlewis requested a review from a team September 28, 2023 13:54

rharding6373 requested review from a team and michae2 and removed request for a team September 28, 2023 14:05

rafiss requested changes Oct 2, 2023

View reviewed changes

rafiss self-requested a review November 24, 2023 05:05

charlespnh force-pushed the chrls_metaphone branch from f99f873 to a875e32 Compare November 25, 2023 16:17

charlespnh force-pushed the chrls_metaphone branch from 90b7ce4 to f99f873 Compare November 25, 2023 16:31

charlespnh force-pushed the chrls_metaphone branch from f99f873 to cd410b0 Compare November 25, 2023 17:54

charlespnh marked this pull request as ready for review November 26, 2023 15:57

charlespnh requested a review from a team as a code owner November 26, 2023 15:57

charlespnh force-pushed the chrls_metaphone branch from cd410b0 to fc3fde5 Compare December 15, 2023 03:12

rafiss force-pushed the chrls_metaphone branch from fc3fde5 to d125556 Compare December 28, 2023 17:54

rafiss requested changes Dec 28, 2023

View reviewed changes

charlespnh force-pushed the chrls_metaphone branch from d125556 to 3b37eae Compare January 8, 2024 19:52

rafiss requested changes Jan 8, 2024

View reviewed changes

charlespnh commented Jan 8, 2024

View reviewed changes

charlespnh force-pushed the chrls_metaphone branch from 3b37eae to 17c5380 Compare January 8, 2024 21:24

rafiss requested changes Jan 8, 2024

View reviewed changes

pkg/util/fuzzystrmatch/metaphone.go Outdated Show resolved Hide resolved

builtins: add fuzzystrmatch metaphone()

13fb2c2

Release note (sql change): metaphone() builtin function was added.

charlespnh force-pushed the chrls_metaphone branch from 17c5380 to 13fb2c2 Compare January 8, 2024 21:43

rafiss approved these changes Jan 10, 2024

View reviewed changes

craig bot merged commit dab086d into cockroachdb:master Jan 10, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builtins: add fuzzystrmatch metaphone() built-in #110950

builtins: add fuzzystrmatch metaphone() built-in #110950

charlespnh commented Sep 20, 2023 •

edited by rafiss

cockroach-teamcity commented Sep 20, 2023 •

edited

blathers-crl bot commented Sep 20, 2023

cockroach-teamcity commented Sep 20, 2023

blathers-crl bot commented Sep 27, 2023

rafiss left a comment

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 25, 2023

blathers-crl bot commented Nov 25, 2023

blathers-crl bot commented Nov 25, 2023

blathers-crl bot commented Nov 25, 2023

charlespnh commented Nov 26, 2023 •

edited

yuzefovich commented Dec 26, 2023

rafiss left a comment •

edited

rafiss left a comment

charlespnh left a comment

rafiss left a comment

rafiss left a comment

craig bot commented Jan 10, 2024

builtins: add fuzzystrmatch metaphone() built-in #110950

builtins: add fuzzystrmatch metaphone() built-in #110950

Conversation

charlespnh commented Sep 20, 2023 • edited by rafiss

cockroach-teamcity commented Sep 20, 2023 • edited

blathers-crl bot commented Sep 20, 2023

cockroach-teamcity commented Sep 20, 2023

blathers-crl bot commented Sep 27, 2023

rafiss left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 23, 2023

blathers-crl bot commented Nov 25, 2023

blathers-crl bot commented Nov 25, 2023

blathers-crl bot commented Nov 25, 2023

blathers-crl bot commented Nov 25, 2023

charlespnh commented Nov 26, 2023 • edited

yuzefovich commented Dec 26, 2023

rafiss left a comment • edited

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

charlespnh left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Jan 10, 2024

charlespnh commented Sep 20, 2023 •

edited by rafiss

cockroach-teamcity commented Sep 20, 2023 •

edited

charlespnh commented Nov 26, 2023 •

edited

rafiss left a comment •

edited