Implementing math power function for SQL by comphead · Pull Request #2324 · apache/datafusion

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             |",
    "+-----------+-----------+-----------+-----------+------------------+--------------------+",

…into master

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

        }),

+        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

+            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

+        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

+    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

 }
+
+#[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!

* Implementing POWER function * Delete pv.yaml * Delete build-ballista-docker.sh * Delete ballista.dockerfile * aligining with latest upstream changes * Readding docker files * Formatting * Leaving only 64bit types * Adding tests, remove type conversion * fix for cast * Update functions.rs (cherry picked from commit c3c02cf) Can drop this after rebase on commit c3c02cf "Implementing math power function for SQL (apache#2324)", first released in 8.0.0 # Conflicts: # datafusion/core/src/logical_plan/mod.rs # datafusion/core/src/physical_plan/functions.rs # datafusion/core/tests/sql/functions.rs # datafusion/cube_ext/Cargo.toml # datafusion/expr/src/built_in_function.rs # datafusion/expr/src/function.rs # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/from_proto.rs # datafusion/proto/src/to_proto.rs # dev/docker/ballista.dockerfile

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 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

Comment thread datafusion/core/src/physical_plan/functions.rs 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

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

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?

Uh oh!

comphead commented Apr 23, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead commented Apr 26, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 28, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

comphead Apr 27, 2022 •

edited

Loading