[CALCITE-5831] Add SOUNDEX function (enabled in Spark library) #3307

chucheng92 · 2023-07-10T11:29:31Z

What is the purpose of the change

Returns Soundex code of the string.

SELECT soundex('Miller');
M460

details pls see: https://issues.apache.org/jira/browse/CALCITE-5831
note: spark soundex not throw exception but return original string if the input string can't be mapped.

Brief change log

Add soundex function (enabled in Spark Library)

Verifying this change

SqlOperatorTests#testSoundex2Func()

tanclary

Looks pretty good I think. Is the behavior for the other dialects correct? I would imagine so but on the off chance it's not, we could avoid adding a whole extra operator. Otherwise just left a couple small comments.

tanclary · 2023-07-10T17:19:29Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java

@@ -227,6 +227,7 @@
 import static org.apache.calcite.sql.fun.SqlLibraryOperators.SINH;
 import static org.apache.calcite.sql.fun.SqlLibraryOperators.SORT_ARRAY;
 import static org.apache.calcite.sql.fun.SqlLibraryOperators.SOUNDEX;
+import static org.apache.calcite.sql.fun.SqlLibraryOperators.SOUNDEX2;


I feel like SOUNDEX2 implies it takes 2 arguments, maybe SOUNDEX_SPARK would be a better name? Similar to the SUBSTR or FLOOR/CEIL functions.

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

NobiGo · 2023-07-11T06:13:29Z

@chucheng92 Please make the commit info stay the same as the Jira summary.

chucheng92 · 2023-07-11T06:21:58Z

@chucheng92 Please make the commit info stay the same as the Jira summary.

@NobiGo thanks. I added a commit for letting commiters to review. should i merge to one commit and force-push it?

chucheng92 · 2023-07-11T06:24:46Z

Looks pretty good I think. Is the behavior for the other dialects correct? I would imagine so but on the off chance it's not, we could avoid adding a whole extra operator. Otherwise just left a couple small comments.

Maybe the behavior is still a bit different, such as calcite mysql library(it limits 4 chars) now

mysql> SELECT SOUNDEX('Quadratically');
         -> 'Q363'

And mysql:

mysql> SELECT SOUNDEX('Quadratically');
         -> 'Q36324'

But this behavior is also different from spark, so we may need to deal with each dialect separately later. Given that these issues are orthogonal, I prepare to create another issue to address it. WDYT?

chucheng92 · 2023-07-11T06:33:17Z

@tanclary thanks for reviewing. I have updated this pr. please take a look.

tanclary

I think this is looking good, I just left one comment about the operator table entry. If you wouldn't mind changing the commit message from Soundex->SOUNDEX whenever you squash your commits that'd be great. Thank you!

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

chucheng92 · 2023-07-12T08:45:58Z

@tanclary @olivrlee hi, I have addressed the problems and rebased calcite-main, PTAL. thanks.

tanclary

I think this looks great! Thanks for addressing all the comments. If no one has other suggestions I will merge later today.

chucheng92 · 2023-07-27T12:06:04Z

@tanclary Hi, tanner. Main Code freeze has been closed. Can you help to merge this?

tanclary · 2023-07-27T14:12:30Z

@tanclary Hi, tanner. Main Code freeze has been closed. Can you help to merge this?

I saw you opened an issue for making an addition to SqlBasicFunction and I believe you referenced this case. Should we resolve that so this case can use that logic?

chucheng92 · 2023-07-27T14:14:23Z

@tanclary Hi, tanner. Main Code freeze has been closed. Can you help to merge this?

I saw you opened an issue for making an addition to SqlBasicFunction and I believe you referenced this case. Should we resolve that so this case can use that logic?

ok, it makes sense, thanks.

chucheng92 · 2023-07-29T08:24:46Z

@tanclary hi, tanner. I have added a method of withReturnTypeInference and use it, just like Julian suggested in the issue to squash it in this PR instead of open a new PR. PTAL~ thanks a lot.

tanclary

I think this looks good and is just about ready. Would you mind writing a RelToSqlConverter test and ensure that it gets unparsed as SOUNDEX(..) and not SOUNDEX_SPARK(..)? Sorry for the delay in review I have been out the past few days.

chucheng92 · 2023-08-02T03:25:18Z

I think this looks good and is just about ready. Would you mind writing a RelToSqlConverter test and ensure that it gets unparsed as SOUNDEX(..) and not SOUNDEX_SPARK(..)? Sorry for the delay in review I have been out the past few days.

Hi, @tanclary I got your point. I have added it. PTAL.

tanclary · 2023-08-02T16:42:15Z

I think this looks good and is just about ready. Would you mind writing a RelToSqlConverter test and ensure that it gets unparsed as SOUNDEX(..) and not SOUNDEX_SPARK(..)? Sorry for the delay in review I have been out the past few days.

Hi, @tanclary I got your point. I have added it. PTAL.

Looks good. Squash your commits please and I will merge soon.

chucheng92 · 2023-08-03T01:09:46Z

I think this looks good and is just about ready. Would you mind writing a RelToSqlConverter test and ensure that it gets unparsed as SOUNDEX(..) and not SOUNDEX_SPARK(..)? Sorry for the delay in review I have been out the past few days.

Hi, @tanclary I got your point. I have added it. PTAL.

Looks good. Squash your commits please and I will merge soon.

Done. thanks again.

sonarqubecloud · 2023-08-03T01:25:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

100.0% Coverage
0.0% Duplication

chucheng92 force-pushed the CALCITE-5831 branch 2 times, most recently from 1c1aaa1 to ac8961a Compare July 10, 2023 12:23

tanclary reviewed Jul 10, 2023

View reviewed changes

chucheng92 force-pushed the CALCITE-5831 branch from 75492c4 to d431c4f Compare July 11, 2023 06:12

chucheng92 force-pushed the CALCITE-5831 branch from d431c4f to 2ac8dc4 Compare July 11, 2023 06:23

tanclary reviewed Jul 11, 2023

View reviewed changes

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java Show resolved Hide resolved

olivrlee reviewed Jul 11, 2023

View reviewed changes

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java Show resolved Hide resolved

chucheng92 force-pushed the CALCITE-5831 branch from 2ac8dc4 to 146472a Compare July 12, 2023 02:22

chucheng92 requested a review from tanclary July 12, 2023 02:28

chucheng92 force-pushed the CALCITE-5831 branch 3 times, most recently from 6c98a97 to b680dc3 Compare July 12, 2023 06:32

chucheng92 force-pushed the CALCITE-5831 branch from b680dc3 to 008c055 Compare July 12, 2023 15:03

chucheng92 changed the title ~~[CALCITE-5831] Add Soundex function (enabled in Spark library)~~ [CALCITE-5831] Add SOUNDEX function (enabled in Spark library) Jul 12, 2023

tanclary approved these changes Jul 12, 2023

View reviewed changes

tanclary added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 12, 2023

chucheng92 force-pushed the CALCITE-5831 branch from 008c055 to 0dcbad4 Compare July 29, 2023 08:19

chucheng92 requested a review from tanclary July 31, 2023 13:12

tanclary reviewed Aug 1, 2023

View reviewed changes

[CALCITE-5831] Add SOUNDEX function (enabled in Spark library)

3a8bf52

chucheng92 force-pushed the CALCITE-5831 branch from a8511cf to 3a8bf52 Compare August 3, 2023 01:06

chucheng92 requested a review from tanclary August 3, 2023 14:28

tanclary merged commit 76ba489 into apache:main Aug 4, 2023

[CALCITE-5831] Add SOUNDEX function (enabled in Spark library) #3307

[CALCITE-5831] Add SOUNDEX function (enabled in Spark library) #3307

Uh oh!

Conversation

chucheng92 commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Uh oh!

tanclary left a comment

Choose a reason for hiding this comment

Uh oh!

tanclary Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NobiGo commented Jul 11, 2023

Uh oh!

chucheng92 commented Jul 11, 2023

Uh oh!

chucheng92 commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chucheng92 commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanclary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chucheng92 commented Jul 12, 2023

Uh oh!

tanclary left a comment

Choose a reason for hiding this comment

Uh oh!

chucheng92 commented Jul 27, 2023

Uh oh!

tanclary commented Jul 27, 2023

Uh oh!

chucheng92 commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chucheng92 commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanclary left a comment

Choose a reason for hiding this comment

Uh oh!

chucheng92 commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanclary commented Aug 2, 2023

Uh oh!

chucheng92 commented Aug 3, 2023

Uh oh!

sonarqubecloud bot commented Aug 3, 2023

Uh oh!

Uh oh!

chucheng92 commented Jul 10, 2023 •

edited

Loading

chucheng92 commented Jul 11, 2023 •

edited

Loading

chucheng92 commented Jul 11, 2023 •

edited

Loading

chucheng92 commented Jul 27, 2023 •

edited

Loading

chucheng92 commented Jul 29, 2023 •

edited

Loading

chucheng92 commented Aug 2, 2023 •

edited

Loading