[CALCITE-6313] Add POWER function for PostgreSQL #3762

normanj-bitquill · 2024-04-16T18:15:38Z

The existing power function is moved to all non PostgreSQL libraries
The new power function is only for PostgreSQL
The new function returns a decimal if any argument is a decimal

normanj-bitquill · 2024-04-16T18:18:28Z

In order to separate the POWER function for PostgreSQL and non-PostgreSQL dialects, the POWER function was moved to the SqlLibraryOperators class. This downgrades the POWER function from a standard function to a function from a library. This is problematic for things like SQRT which is a standard function and gets rewritten to a POWER expression.

Is there a more clean way to separate the POWER function so that the PostgreSQL implementation is able to return Decimal values in some cases?

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

core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java

mihaibudiu · 2024-04-17T00:39:33Z

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

@@ -559,15 +558,6 @@ static SqlOperatorTable operatorTableFor(SqlLibrary library) {
  }

  @Test void testArithmeticOperatorsFails() {
-    expr("^power(2,'abc')^")


why delete these tests? What happens to them? They look like good tests.

The POWER function is no longer available in SqlStdOperatorTable. The implementation to use depends on which library is active since PostgreSQL can use a different return type.

This test fails as it was since POWER is not in StdOperatorTable. Is there a way to specify the library to use for the test?

Yes, see the operatorTableFor() function.

mihaibudiu · 2024-04-17T00:39:50Z

core/src/test/resources/sql/misc.iq

@@ -2301,17 +2301,6 @@ where false;

 !ok

-# [CALCITE-2447] POWER, ATAN2 functions fail with NoSuchMethodException


why delete this test?

The POWER function is no longer available in SqlStdOperatorTable. Is there any way of specifying a library to use here?

I don't think there is, but the question is whether there should be a standard power function.

I found where the ChainedSqlOperatorTable is created and was able to give the library operator table precedence over the standard operator table.

mihaibudiu · 2024-04-17T00:40:31Z

testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java

@@ -2006,7 +2006,7 @@ void checkPeriodPredicate(Checker checker) {
    expr("1-2+3*4/5/6-7")
        .ok("(((1 - 2) + (((3 * 4) / 5) / 6)) - 7)");
    expr("power(2,3)")
-        .ok("POWER(2, 3)");
+        .ok("`POWER`(2, 3)");


these quotes look suspicious

This will end up going through here:
https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L393

and only look in SqlStdOperatorTable for the function.

Then it's related to my previous question: should there be a variant of this function in the standard operator table?
If not, I guess this is fine.

Is it possible to have a power function in the standard operator table and still have an override for PostgreSQL that is able to return a decimal in some cases?

I don't know the answer to this question. But I know that Calcite can load multiple libraries, and I believe a function will be resolved by trying the libraries in order. I also am assuming that there is nothing special about the standard operator table. So if you load the postgres operator table first, perhaps the right POWER will be used?

A quick experiment should be able answer this question.

If this works, we should probably document that the right way to implement a dialect is to load the dialect specific operator table first.

I found where the ChainedSqlOperatorTable is created and was able to give the library operator table precedence over the standard operator table.

With this change, no changes are needed to existing tests.

mihaibudiu · 2024-04-24T18:14:05Z

In general, until the changes are approved I would recommend adding new commits instead of force-pushing. It makes it easier for reviewers to see what's new. Then you can squash when the PR is approved.

mihaibudiu · 2024-04-24T18:16:04Z

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

@@ -117,8 +117,7 @@ private SqlOperatorTable create(ImmutableSet<SqlLibrary> librarySet) {
    SqlOperatorTable operatorTable = SqlOperatorTables.of(list);
    if (standard) {
      operatorTable =
-          SqlOperatorTables.chain(SqlStdOperatorTable.instance(),
-              operatorTable);
+          SqlOperatorTables.chain(operatorTable, SqlStdOperatorTable.instance());


This may cause problems for other people. Ideally you want to do this just for your tests.

Understood. I'll take a look.

This has been fixed. The only changes needed now are in the unit test itself. I setup the operator table there such that PostgreSQL is first.

mihaibudiu

The second commit should have a descriptive name, e.g., "only update loading library order for tests." But that is moot now, since I think this is ready for merging, so the next step is to squash the commits. We can't merge this until 1.37 is released, though.

* The existing power function is moved to all non PostgreSQL libraries * The new power function is only for PostgreSQL * The new function returns a decimal if any argument is a decimal

sonarcloud · 2024-04-26T15:27:33Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

mihaibudiu requested changes Apr 17, 2024

View reviewed changes

normanj-bitquill force-pushed the calcite-6313 branch 5 times, most recently from 857f16d to d2cdb89 Compare April 24, 2024 18:09

mihaibudiu reviewed Apr 24, 2024

View reviewed changes

mihaibudiu approved these changes Apr 24, 2024

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Apr 24, 2024

normanj-bitquill force-pushed the calcite-6313 branch from bd25b70 to a0fff43 Compare April 25, 2024 17:44

[CALCITE-6313] Add POWER function for PostgreSQL

db336c2

* The existing power function is moved to all non PostgreSQL libraries * The new power function is only for PostgreSQL * The new function returns a decimal if any argument is a decimal

normanj-bitquill force-pushed the calcite-6313 branch from a0fff43 to db336c2 Compare April 26, 2024 15:16

mihaibudiu merged commit 5ab4397 into apache:main May 14, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6313] Add POWER function for PostgreSQL #3762

[CALCITE-6313] Add POWER function for PostgreSQL #3762

normanj-bitquill commented Apr 16, 2024

normanj-bitquill commented Apr 16, 2024 •

edited

mihaibudiu Apr 17, 2024

normanj-bitquill Apr 18, 2024

mihaibudiu Apr 18, 2024

normanj-bitquill Apr 19, 2024

mihaibudiu Apr 17, 2024

normanj-bitquill Apr 19, 2024

mihaibudiu Apr 19, 2024

normanj-bitquill Apr 24, 2024

mihaibudiu Apr 17, 2024

normanj-bitquill Apr 19, 2024

mihaibudiu Apr 19, 2024

normanj-bitquill Apr 22, 2024

mihaibudiu Apr 22, 2024

normanj-bitquill Apr 24, 2024

mihaibudiu commented Apr 24, 2024

mihaibudiu Apr 24, 2024

normanj-bitquill Apr 24, 2024

normanj-bitquill Apr 24, 2024

mihaibudiu left a comment

sonarcloud bot commented Apr 26, 2024

		@@ -2301,17 +2301,6 @@ where false;

		!ok

		# [CALCITE-2447] POWER, ATAN2 functions fail with NoSuchMethodException

[CALCITE-6313] Add POWER function for PostgreSQL #3762

[CALCITE-6313] Add POWER function for PostgreSQL #3762

Conversation

normanj-bitquill commented Apr 16, 2024

normanj-bitquill commented Apr 16, 2024 • edited

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

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

mihaibudiu commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 26, 2024

Quality Gate passed

normanj-bitquill commented Apr 16, 2024 •

edited