[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) #3732

caicancai · 2024-03-13T03:34:11Z

https://issues.apache.org/jira/browse/CALCITE-6325

caicancai · 2024-03-13T03:36:39Z

I also fixed some problems with bigquery's log function. For example, one parameter of the log function defaults to ln, and the same is true for mysql and spark.

caicancai · 2024-03-13T14:25:04Z

@tanclary If you have time, can you help review the code? As you can see, I have not implemented the ln and log10 functions, I'm sorry about that.
Thank you

tanclary · 2024-03-13T22:47:30Z

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

@@ -211,6 +211,7 @@
 import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOG2;
 import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOGICAL_AND;
 import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOGICAL_OR;
+import static org.apache.calcite.sql.fun.SqlLibraryOperators.LOG_MS;


We really should look into some other mechanism for operators implemented differently across different dialects. I feel that just modifying the name is confusing and not very sustainable (what if 2 dialects support 1 version and 2 support another? what do you name it then?) I know it's not in the scope of this change but just putting it out there.

I think we could rethink how we use the @LibraryAnnotations because it seems like we have a straightforward way of referencing what dialects a given operator supports.

@tanclary I very much agree with your point of view, I am also thinking about this issue, maybe we should take the first step here with the log function. Do you think I should send an email to discuss it?

I have opened an email discussion. If there is a good way, I am willing to try it.

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

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

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

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

tanclary · 2024-03-13T22:59:21Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -6332,6 +6346,66 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
    f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.SPARK), consumer);
  }

+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-6325">[CALCITE-6325]
+   * Add LOG function (enabled in MYSQL, Spark library)</a>. */


nit: MySQL*, and can you make that consistent across this PR (commit message, jira, etc.)

I'm not sure what it means, sorry

Please write it as "MySQL", that's how they style it.

Add LOG function as MySql style or Add LOG function (enabled in MYSQL*) ?

Please write it as "MySQL"

Sorry, I was sick during this period and was delayed for some time.

Co-authored-by: Tanner Clary <[email protected]>

caicancai · 2024-03-14T15:38:37Z

I added clary as co-author, thank you for his review and suggestions

caicancai · 2024-03-17T09:22:45Z

I tried to add LOG_Postgres(SqlFunctions.class, "log", long.class, long.class) to BuiltInMethod, trying to use method.name() to distinguish different databases, but the method of BuiltInMethod corresponds to a Sqlfunctions method, in the form of a map Save, it seems that it cannot be superimposed,

tanclary

@caicancai did you forget to push your changes? All the comments are marked as resolved but it looks identical to how it was last week. Re-ping me when it's ready!

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

caicancai · 2024-03-18T15:55:17Z

@caicancai did you forget to push your changes? All the comments are marked as resolved but it looks identical to how it was last week. Re-ping me when it's ready!

@tanclary Really, I submitted two commits, can you see them?
Is this a GitHub bug?

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

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

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

site/_docs/reference.md

tanclary · 2024-03-18T16:11:35Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -6332,6 +6346,66 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
    f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.SPARK), consumer);
  }

+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-6325">[CALCITE-6325]
+   * Add LOG function (enabled in MYSQL, Spark library)</a>. */


Please write it as "MySQL", that's how they style it.

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

caicancai · 2024-04-01T01:04:02Z

@tanclary PTAL

tanclary · 2024-04-01T02:55:01Z

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

@@ -2205,6 +2205,14 @@ private static RelDataType deriveTypeMapFromEntries(SqlOperatorBinding opBinding
          OperandTypes.NUMERIC_OPTIONAL_NUMERIC,
          SqlFunctionCategory.NUMERIC);

+  /** The "LOG(numeric1, numeric2)" function. Returns the base numeric1 logarithm of numeric. */
+  @LibraryOperator(libraries = {MYSQL, SPARK})


the PR title just has MySQL.

@tanclary How should I reasonably describe this PR title?
Add LOG function (enabled in MYSQL, Spark library) or Add LOG function (enabled in MYSQL* library).

Before I was Add LOG function (enabled in MYSQL, Spark library), I thought you asked me to change it to Add LOG function (enabled in MYSQL library), it seems I didn’t understand what you meant, sorry

I use Mysql_Style to describe it. Do you think it is acceptable?

No. Please look at other similar function implementations to see how they handle multiple libraries.

Other functions do not specifically adapt their log functions to the mysql and spark dialects like the log function.

@tanclary I found similar cases [CALCITE-5642] Add SHA256, SHA512 functions (enabled in BigQuery and PostgreSQL libraries), so I should change back to Add LOG function (enabled in MYSQL, Spark library)

"Style" doesn't add much. The issue is you can't name a functionLOG_MYSQL if that function is also used for Spark. Does that make sense?

LOG_MYSQL_SPARK?

What I wrote at the beginning was LOG_MYSQL_SPARK. It seems that I have always misunderstood what you meant. Sorry.

At present, we have some differences regarding some specifications, and the logic of the code itself is fine. If I still have problems after I modify it, I hope you can tell me how it should be modified because I really can't understand what you mean. Thank you very much. I will be very grateful. (please submit a commit and @caicancai ).

I'm sorry for my behavior of ping you multiple times, I have to admit it was not a polite way

@tanclary PTAL, thank you

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

Co-authored-by: Tanner Clary <[email protected]>

sonarcloud · 2024-04-01T16:11:48Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

caicancai · 2024-04-15T13:46:28Z

@tanclary PATL, i think this pr can merge 1.37. Thank you

tanclary

The problem with this PR is I don't know what I'm reviewing. I know you're adding functions but it's not clear why you need an operator separate from BigQuery. Right now, your JIRA case is completely blank so no reviewer, viewer, or user has any context on what you're doing.

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

caicancai force-pushed the 6325 branch from 7b7cf4a to d49c994 Compare March 13, 2024 03:42

[CALCITE-6325] Add LOG function (enabled in Mysql, Spark library)

4487985

caicancai force-pushed the 6325 branch from d49c994 to 4487985 Compare March 13, 2024 14:51

tanclary requested changes Mar 13, 2024

View reviewed changes

caicancai force-pushed the 6325 branch from 3c2af07 to a7125fe Compare March 14, 2024 15:11

[CALCITE-6325] Add LOG function (enabled in Mysql, Spark library)

719dc85

Co-authored-by: Tanner Clary <[email protected]>

caicancai force-pushed the 6325 branch from a7125fe to 719dc85 Compare March 14, 2024 15:19

caicancai requested a review from tanclary March 14, 2024 15:21

caicancai marked this pull request as draft March 15, 2024 09:24

[CALCITE-6325] Optimize LogImplementor

4b946b9

caicancai marked this pull request as ready for review March 15, 2024 10:18

tanclary requested changes Mar 18, 2024

View reviewed changes

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

tanclary requested changes Mar 18, 2024

View reviewed changes

caicancai changed the title ~~[CALCITE-6325] Add LOG function (enabled in Mysql, Spark library)~~ [CALCITE-6325] Add LOG function (enabled in Mysql library) Mar 19, 2024

[CALCITE-6325] Add LOG function (enabled in Mysql library)

adcbe2e

caicancai force-pushed the 6325 branch from 1d93f42 to ee2ed68 Compare March 25, 2024 12:26

caicancai requested a review from tanclary March 25, 2024 12:29

tanclary requested changes Mar 25, 2024

View reviewed changes

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java Outdated Show resolved Hide resolved

[CALCITE-6325] Modify to MYSQL unified style

22a8767

caicancai force-pushed the 6325 branch from 7b1cc74 to 22a8767 Compare March 27, 2024 13:43

caicancai requested a review from tanclary March 27, 2024 13:44

tanclary requested changes Apr 1, 2024

View reviewed changes

caicancai changed the title ~~[CALCITE-6325] Add LOG function (enabled in Mysql library)~~ [CALCITE-6325] Add LOG function (enabled in Mysql* library) Apr 1, 2024

caicancai changed the title ~~[CALCITE-6325] Add LOG function (enabled in Mysql* library)~~ [CALCITE-6325] Add LOG function (enabled in Mysql Style library) Apr 1, 2024

[CALCITE-6325] Add LOG function (enabled in Mysql Style library)

3a5f076

Co-authored-by: Tanner Clary <[email protected]>

caicancai requested a review from tanclary April 1, 2024 08:33

caicancai changed the title ~~[CALCITE-6325] Add LOG function (enabled in Mysql Style library)~~ [CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) Apr 1, 2024

[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library)

a21896b

Co-authored-by: Tanner Clary <[email protected]>

tanclary requested changes Apr 15, 2024

View reviewed changes

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

caicancai closed this Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) #3732

[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) #3732

caicancai commented Mar 13, 2024 •

edited

Loading

caicancai commented Mar 13, 2024

caicancai commented Mar 13, 2024 •

edited

Loading

tanclary Mar 13, 2024

tanclary Mar 13, 2024

caicancai Mar 14, 2024

caicancai Mar 14, 2024

tanclary Mar 13, 2024

caicancai Mar 14, 2024

tanclary Mar 18, 2024

caicancai Mar 19, 2024

tanclary Mar 19, 2024

caicancai Mar 25, 2024

caicancai commented Mar 14, 2024

caicancai commented Mar 17, 2024

tanclary left a comment

caicancai commented Mar 18, 2024 •

edited

Loading

tanclary Mar 18, 2024

caicancai commented Apr 1, 2024

tanclary Apr 1, 2024

caicancai Apr 1, 2024 •

edited

Loading

caicancai Apr 1, 2024

tanclary Apr 1, 2024

caicancai Apr 1, 2024

caicancai Apr 1, 2024 •

edited

Loading

tanclary Apr 1, 2024

caicancai Apr 1, 2024 •

edited

Loading

caicancai Apr 1, 2024 •

edited

Loading

caicancai Apr 4, 2024

sonarcloud bot commented Apr 1, 2024

caicancai commented Apr 15, 2024 •

edited

Loading

tanclary left a comment

[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) #3732

[CALCITE-6325] Add LOG function (enabled in Mysql and Spark library) #3732

Conversation

caicancai commented Mar 13, 2024 • edited Loading

caicancai commented Mar 13, 2024

caicancai commented Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caicancai commented Mar 14, 2024

caicancai commented Mar 17, 2024

tanclary left a comment

Choose a reason for hiding this comment

caicancai commented Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

caicancai commented Apr 1, 2024

Choose a reason for hiding this comment

caicancai Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caicancai Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caicancai Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

caicancai Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 1, 2024

Quality Gate passed

caicancai commented Apr 15, 2024 • edited Loading

tanclary left a comment

Choose a reason for hiding this comment

caicancai commented Mar 13, 2024 •

edited

Loading

caicancai commented Mar 13, 2024 •

edited

Loading

caicancai commented Mar 18, 2024 •

edited

Loading

caicancai Apr 1, 2024 •

edited

Loading

caicancai Apr 1, 2024 •

edited

Loading

caicancai Apr 1, 2024 •

edited

Loading

caicancai Apr 1, 2024 •

edited

Loading

caicancai commented Apr 15, 2024 •

edited

Loading