Implementing math power function for SQL #2324

comphead · 2022-04-23T14:36:42Z

Which issue does this PR close?

Closes #1493

Rationale for this change

Implementing power function for SQL

What changes are included in this PR?

Implementing power function for SQL

Are there any user-facing changes?

User can use math POWER function in SQL statements

…into master

comphead · 2022-04-23T14:44:17Z

Implement math POWER function with signatures like https://www.postgresql.org/docs/14/functions-math.html

Example usage

    "SELECT power(i32, 3) as power_i32,
             power(i64, 3) as power_i64,
             power(f32, 3) as power_f32,
             power(f64, 3) as power_f64,
             power(2, 3) as power_int_scalar,
             power(2.5, 3) as power_float_scalar
      FROM test";

Output

    "+-----------+-----------+-----------+-----------+------------------+--------------------+",
    "| power_i32 | power_i64 | power_f32 | power_f64 | power_int_scalar | power_float_scalar |",
    "+-----------+-----------+-----------+-----------+------------------+--------------------+",
    "| 8         | 8         | 1         | 1         | 8                | 15.625             |",
    "| 125       | 125       | 15.625    | 15.625    | 8                | 15.625             |",
    "| 0         | 0         | 0         | 0         | 8                | 15.625             |",
    "| -2744     | -2744     | -3048.625 | -3048.625 | 8                | 15.625             |",
    "|           |           |           |           | 8                | 15.625             |",
    "+-----------+-----------+-----------+-----------+------------------+--------------------+",

datafusion/core/src/physical_plan/functions.rs

…into master

Ted-Jiang · 2022-04-25T12:16:55Z

datafusion/expr/src/function.rs

@@ -217,6 +217,11 @@ pub fn return_type(
            }
        }),

+        BuiltinScalarFunction::Power => match &input_expr_types[0] {
+            DataType::Int32 | DataType::Int64 => Ok(DataType::Int64),


what about Int8,Int16, UInt....

Now its changed to be like in https://www.postgresql.org/docs/14/functions-math.html

So the signature is either i64 or f64.
If user column is another col type with less size like i32, f32, i16, etc it will work, however the return type is i64 or f64.
Let me know if this is ok

comphead · 2022-04-26T15:45:53Z

Hi @Ted-Jiang @xudong963, can you please review again, I'm looking forward to commence to other datafusion tasks

alamb

Thanks for the contribution @comphead -- looking good 👍

alamb · 2022-04-26T19:37:20Z

datafusion/expr/src/function.rs

+            vec![
+                TypeSignature::Exact(vec![DataType::Int64, DataType::Int64]),
+                TypeSignature::Exact(vec![DataType::Float64, DataType::Float64]),
+            ],


It appears that postgres supports dp (aka double precision, aka Float64) and Numeric

https://www.postgresql.org/docs/12/functions-math.html

I think for the initial implementation it would be fine to support Int64 as well, but longer term we should add coercion rules to convert arguments to Float64 before they are actually passed to power

alamb · 2022-04-26T19:38:31Z

datafusion/physical-expr/src/math_expressions.rs

+        arg1.iter()
+            .zip(arg2.iter())
+            .map(|(a1, a2)| match (a1, a2) {
+                (Some(a1), Some(a2)) => Some($FUNC(a1, a2.try_into().unwrap())),


I wonder if .unwrap() is needed here -- it will cause a panic if the argument can't be converted to whatever $FUNC wants

Does it work if you do

Suggested change

(Some(a1), Some(a2)) => Some($FUNC(a1, a2.try_into().unwrap())),

(Some(a1), Some(a2)) => Some($FUNC(a1, a2.try_into()?)),

Instead?

I came up with (Some(a1), Some(a2)) => Some($FUNC(a1, a2.try_into().ok()?)),

alamb · 2022-04-26T19:41:00Z

datafusion/core/tests/sql/functions.rs

+    let ctx = SessionContext::new();
+    ctx.register_table("test", Arc::new(table))?;
+    let sql = r"SELECT power(i32, 3) as power_i32,
+                 power(i64, 3) as power_i64,


A test that takes two columns in addition to a column and a scalar would also be good -- like power(i64, i64) perhaps

…atafusion into update_upstream

alamb

Other than the erroneously included test, I think this is looking good. Thank you @comphead . Nice first contribution

alamb · 2022-04-27T17:57:18Z

datafusion/core/tests/sql/functions.rs

@@ -445,3 +445,169 @@ async fn case_builtin_math_expression() {
        assert_batches_sorted_eq!(expected, &results);
    }
 }
+
+#[tokio::test]
+async fn case_sensitive_identifiers_aggregates() {


I still think this test is accidentally included in this PR. It currently exists in aggregates.rs:

https://github.com/apache/arrow-datafusion/blob/cc496f87caf8d13e4c17025e0d3b86643a21a243/datafusion/core/tests/sql/aggregates.rs#L1112

Fixed, probably a merge issue. Tried to be in sync with fast changing files

alamb · 2022-04-28T12:53:37Z

Thanks again all!

comphead added 2 commits April 23, 2022 07:17

Implementing POWER function

5b90d3a

Merge branch 'master' of https://github.com/comphead/arrow-datafusion …

e4e9d0e

…into master

github-actions bot added datafusion Changes in the datafusion crate development-process Related to development process of DataFusion labels Apr 23, 2022

Delete pv.yaml

2484973

comphead marked this pull request as ready for review April 23, 2022 14:44

xudong963 added the enhancement New feature or request label Apr 23, 2022

xudong963 reviewed Apr 23, 2022

View reviewed changes

datafusion/core/src/physical_plan/functions.rs Outdated Show resolved Hide resolved

comphead and others added 7 commits April 23, 2022 10:34

Delete build-ballista-docker.sh

6687009

Delete ballista.dockerfile

07e7f59

aligining with latest upstream changes

d8c73c1

Merge branch 'master' of https://github.com/comphead/arrow-datafusion …

afe40c4

…into master

Merge branch 'apache:master' into master

5c80524

Readding docker files

e2fd339

Merge branch 'master' of https://github.com/comphead/arrow-datafusion …

f177e39

…into master

comphead requested a review from xudong963 April 23, 2022 19:39

Formatting

6e0cf10

Ted-Jiang reviewed Apr 25, 2022

View reviewed changes

comphead and others added 3 commits April 25, 2022 08:10

Leaving only 64bit types

caf2413

Merge branch 'master' into update_upstream

1d50bb6

Merge branch 'master' into update_upstream

167cabd

alamb reviewed Apr 26, 2022

View reviewed changes

comphead and others added 2 commits April 27, 2022 01:57

Adding tests, remove type conversion

8faa7f2

Merge branch 'master' into update_upstream

84e6c58

comphead requested a review from alamb April 27, 2022 09:00

comphead added 2 commits April 27, 2022 05:14

fix for cast

d60d217

Merge branch 'update_upstream' of https://github.com/comphead/arrow-d…

ef3040c

…atafusion into update_upstream

alamb approved these changes Apr 27, 2022

View reviewed changes

Update functions.rs

e3b234f

Ted-Jiang approved these changes Apr 28, 2022

View reviewed changes

alamb merged commit c3c02cf into apache:master Apr 28, 2022

comphead deleted the update_upstream branch April 28, 2022 12:59

comphead mentioned this pull request Jul 11, 2022

Implement from_unixtime function #2871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing math power function for SQL #2324

Implementing math power function for SQL #2324

comphead commented Apr 23, 2022

comphead commented Apr 23, 2022

Ted-Jiang Apr 25, 2022

comphead Apr 25, 2022

comphead commented Apr 26, 2022

alamb left a comment

alamb Apr 26, 2022

alamb Apr 26, 2022

comphead Apr 27, 2022 •

edited

Loading

alamb Apr 26, 2022

comphead Apr 27, 2022

alamb left a comment

alamb Apr 27, 2022

comphead Apr 27, 2022

alamb commented Apr 28, 2022

	(Some(a1), Some(a2)) => Some($FUNC(a1, a2.try_into().unwrap())),
	(Some(a1), Some(a2)) => Some($FUNC(a1, a2.try_into()?)),

Implementing math power function for SQL #2324

Implementing math power function for SQL #2324

Conversation

comphead commented Apr 23, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

comphead commented Apr 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Apr 26, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Apr 28, 2022

comphead Apr 27, 2022 •

edited

Loading