[SQL] Add is_inf, is_nan, log for floats #1344

abhizer · 2024-01-26T13:49:48Z

[SQL] Add import float8 tests from Postgres
bugfix: Implement ln for double

Is this a user-visible change (yes/no): yes

Fixes: #1328
Fixes: #1326
Fixes: #1351
Fixes: #1348
Fixes: #1363

abhizer · 2024-01-26T13:51:17Z

...er/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java

+                    case "log10":
+                    case "ln":
+                    {
+                        // Cast to Double if the type isn't Decimal
+                        DBSPExpression arg = ops.get(0);
+                        if (!arg.type.is(DBSPTypeDecimal.class)) {
+                            ops.set(0, arg.cast(new DBSPTypeDouble(node, arg.getType().mayBeNull)));
+                        }
+                        return this.compilePolymorphicFunction(call, node, type, ops, 1);
+                    }


abhizer · 2024-01-26T13:51:58Z

...er/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java

+                    case "log":
+                    {
+                        // Turn the arguments into Double
+                        for (int i = 0; i < ops.size(); i++) {
+                            DBSPExpression arg = ops.get(i);
+                            ops.set(i, arg.cast(new DBSPTypeDouble(node, arg.getType().mayBeNull)));
+                        }
+                        return this.compilePolymorphicFunction(call, node, type, ops, 1, 2);
+                    }


We only support log for double as rust_decimal doesn't have a native decimal support for it.

mihaibudiu · 2024-01-26T18:44:49Z

docs/sql/float.md

  <tr>
    <td><code>LOG10(value)</code></td>
    <td>The logarithm base 10 of value. Produces a runtime error for values less than or equal to zero.</td>
  </tr>
+  <tr>
+    <td><code>IS_INF(value)</code></td>
+    <td>Returns true if the value is infinite.</td>


there are multiple infinities and NaNs in FP, but maybe we don't have to say that.

...er/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java

...er/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/functions/FunctionsTest.java

...er/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java

mihaibudiu · 2024-01-26T18:50:17Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+/*
+                -- power -> ensure both parameters have the same type
+                -- truncate
+              SELECT f.f1, trunc(f.f1) AS trunc_f1 --> not defined for double


trunc should work for double. it it doesn't, please file an issue

mihaibudiu · 2024-01-26T18:51:10Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+       NaN |                  NaN |                 NaN
+(22 rows)
+
+-- ignored some of the tests after the update query


if you have an update query you can probably create a separate table with just the updated versions of the data

Let's move these tests to a second file which initializes the table with the updated contents.

mihaibudiu · 2024-01-26T18:52:06Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+(5 rows)
+*/
+    // calcite returns: 1008618.49
+    @Test @Ignore("fails for calcite optimized version")


I am not sure this is a bug really in Calcite. We should investigate. Please file an issue for us.

Please add the issue to the ignore comment so we can fix it later

sql-to-dbsp-compiler/lib/sqllib/src/lib.rs

abhizer · 2024-01-30T20:25:53Z

Fixes: #1351
Fixes: #1348

mihaibudiu

This looks very good.

mihaibudiu · 2024-01-30T21:45:57Z

docs/sql/float.md

+  </tr>
+  <tr>
+    <td><code>LOG(value, [, value2])</code></td>
+    <td>The logarithm of value to base value2, or base e if value2 is not present. Returns `-inf` for value 0. Produces a runtime error for negative numbers.</td>


for value2=0.
negative values for either value or value2
Let's call "value2" "base" instead

mihaibudiu · 2024-01-30T21:46:32Z

docs/sql/float.md

+  </tr>
+  <tr>
+    <td><code>IS_NAN(value)</code></td>
+    <td>Returns true if the value is NaN. Note that two NaN values aren't equal.</td>


they may be equal.
But there are multiple kinds of NaN, which are not equal to each other.

mihaibudiu · 2024-01-30T21:46:54Z

docs/sql/float.md

+  </tr>
+  <tr>
+    <td><code>IS_INF(value)</code></td>
+    <td>Returns true if the value is infinite. Note that two infinite values aren't necessarily equal.</td>


there is + and - infinity.
I think there is one of each, though, and equality works for them.

Oh.
I thought like in maths, some infinities can be larger than others. But that doesn't seem to be true here.
I'll update it.

mihaibudiu · 2024-01-30T21:58:35Z

sql-to-dbsp-compiler/lib/sqllib/src/lib.rs

 pub fn sqrt_decimal(left: Decimal) -> F64 {
    F64::from(left.sqrt().unwrap().to_f64().unwrap())
 }

 some_polymorphic_function1!(sqrt, decimal, Decimal, F64);

 pub fn sqrt_d(left: F64) -> F64 {
-    F64::from(left.into_inner().sqrt())
+    let left = left.into_inner();
+    if left < 0.0 || left.is_nan() {


usually is_nan produces is_nan as a result. Is this check necessary?

Yeah, it doesn't seem necessary. Removed.

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

mihaibudiu · 2024-01-30T22:00:16Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+ * Tests manually adapted from
+ * https://github.com/postgres/postgres/blob/master/src/test/regress/expected/float8.out
+ */
+@SuppressWarnings("JavadocLinkAsPlainText")


If you make the comment /* instead of /** you can remove this warning.

mihaibudiu · 2024-01-30T22:01:56Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+    }
+
+/*
+                -- check edge cases for exp


what is wrong with these tests?

We don't support those functions yet.

mihaibudiu · 2024-01-30T22:02:38Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+       NaN |                  NaN |                 NaN
+(22 rows)
+
+-- ignored some of the tests after the update query


Let's move these tests to a second file which initializes the table with the updated contents.

mihaibudiu · 2024-01-30T22:03:10Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+(5 rows)
+*/
+    // calcite returns: 1008618.49
+    @Test @Ignore("fails for calcite optimized version")


Please add the issue to the ignore comment so we can fix it later

mihaibudiu · 2024-01-30T22:04:08Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+    }
+
+    // Calcite optimized version returns 0
+    @Test @Ignore("https://github.com/feldera/feldera/issues/1348")


Can we fix this test?
I think we agreed that log in base 0 does return 0.

Yes, this is fixed.

abhizer · 2024-01-31T15:48:22Z

Fixes: #1363

* [SQL] Add import float8 tests from Postgres * [SQL] bugfix: Implement ln for double * [SQL] add more methods for polymorphic function power() * [SQL] Define is_nan, is_inf for both real & double * [SQL] Implement truncate, round, sqrt for double Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

mihaibudiu · 2024-01-31T19:26:45Z

...L-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/postgres/PostgresFloat8Tests.java

+    }
+
+/*
+                -- check edge cases for exp --> we don't support exp yet


I have added EXP to the list of functions to implement #1265

abhizer requested a review from mihaibudiu January 26, 2024 13:49

abhizer commented Jan 26, 2024

View reviewed changes

mihaibudiu reviewed Jan 26, 2024

View reviewed changes

This was referenced Jan 26, 2024

[SQL] ICE: Casting invalid value to REAL causes compiler bug #1345

Closed

[SQL] Calcite returns a rounded off value for power() on doubles #1347

Open

[SQL] Calcite optimizes log(POSITIVE_NUMBER, 0) down to 0, when it should be an error #1348

Closed

abhizer force-pushed the fp_log_and_tests branch from ee7cb52 to c2e6bb3 Compare January 30, 2024 20:24

abhizer requested a review from mihaibudiu January 30, 2024 20:25

mihaibudiu reviewed Jan 30, 2024

View reviewed changes

abhizer added 2 commits January 31, 2024 22:26

[SQL] Add remaining float8 tests

72e45a4

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

abhizer force-pushed the fp_log_and_tests branch from aebbbdf to 72e45a4 Compare January 31, 2024 16:41

abhizer requested a review from mihaibudiu January 31, 2024 17:13

mihaibudiu approved these changes Jan 31, 2024

View reviewed changes

mihaibudiu merged commit ac51bb4 into main Jan 31, 2024
5 checks passed

mihaibudiu deleted the fp_log_and_tests branch January 31, 2024 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SQL] Add is_inf, is_nan, log for floats #1344

[SQL] Add is_inf, is_nan, log for floats #1344

abhizer commented Jan 26, 2024 •

edited

abhizer Jan 26, 2024

abhizer Jan 26, 2024

mihaibudiu Jan 26, 2024

mihaibudiu Jan 26, 2024

mihaibudiu Jan 26, 2024

mihaibudiu Jan 30, 2024

mihaibudiu Jan 26, 2024

abhizer Jan 27, 2024

mihaibudiu Jan 30, 2024

abhizer commented Jan 30, 2024

mihaibudiu left a comment

mihaibudiu Jan 30, 2024

mihaibudiu Jan 30, 2024

mihaibudiu Jan 30, 2024

abhizer Jan 31, 2024

mihaibudiu Jan 30, 2024

abhizer Jan 31, 2024

mihaibudiu Jan 30, 2024

mihaibudiu Jan 30, 2024

abhizer Jan 31, 2024

mihaibudiu Jan 30, 2024

mihaibudiu Jan 30, 2024

mihaibudiu Jan 30, 2024

abhizer Jan 31, 2024

abhizer commented Jan 31, 2024

mihaibudiu Jan 31, 2024

[SQL] Add is_inf, is_nan, log for floats #1344

[SQL] Add is_inf, is_nan, log for floats #1344

Conversation

abhizer commented Jan 26, 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

abhizer commented Jan 30, 2024

mihaibudiu left a 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

Choose a reason for hiding this comment

abhizer commented Jan 31, 2024

Choose a reason for hiding this comment

abhizer commented Jan 26, 2024 •

edited