Support `round()` function with two parameters by viirya · Pull Request #5807 · apache/datafusion

viirya · 2023-03-30T21:42:11Z

Which issue does this PR close?

Closes #4191.
Closes #2420.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

viirya · 2023-03-30T21:43:07Z

datafusion/sql/tests/integration_test.rs

+    let sql = "insert into person (id, first_name, last_name) values (1, 'Alan', 'Turing')";
    let plan = r#"
 Dml: op=[Insert] table=[person]
  Projection: CAST(column1 AS UInt32) AS id, column2 AS first_name, column3 AS last_name


These changes are by rust-fmt, I think.

viirya · 2023-03-30T21:43:40Z

datafusion/sql/tests/integration_test.rs

 fn round_decimal() {
-    let sql = "SELECT round(price/3, 2) FROM test_decimal";
-    let expected = "Projection: round(test_decimal.price / Int64(3), Int64(2))\
+    let sql = "SELECT round(price/3, CAST(2 AS INT)) FROM test_decimal";
+    let expected = "Projection: round(test_decimal.price / Int64(3), CAST(Int64(2) AS Int32))\
        \n  TableScan: test_decimal";
    quick_test(sql, expected);
 }


In this file, I only manually changed this test.

Reverted this change. But I don't revert the format change.

alamb

Looks good to me -- thank you @viirya

I recommend also adding end to end (sql level) coverage for this change too using sqllogictests: https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests

Maybe to https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/select.slt or a new file like math.slt

Can you please add coverage for the test case in #2420 ? I think it would be straight forward to add as a sqllogictest -- here is an example of how to do so https://github.com/apache/arrow-datafusion/blob/df28b0132534c99e2366dd59811e57f9732d6e29/datafusion/core/tests/sqllogictests/test_files/ddl.slt#L206

datafusion/physical-expr/src/math_expressions.rs

alamb · 2023-03-31T16:48:47Z

datafusion/physical-expr/src/math_expressions.rs

+    let mut decimal_places =
+        &(Arc::new(Int64Array::from_value(0, args[0].len())) as ArrayRef);


It might be more efficient to make a ColumnarValue to avoid creating a large empty array. That would complicate the implementation as it would have to handle both the array argument form and the scalar argument form

Yea, not sure if it is worth.

Ah, there is other function that also create an array like that for one arg case, e.g. log, so I will change them all in a follow up.

I agree it is reasonable to start with a working implementation and optimize as a follow on 👍

alamb · 2023-03-31T16:52:55Z

datafusion/physical-expr/src/math_expressions.rs

+            as_float32_array(&result).expect("failed to initialize function round");
+
+        let expected = Float32Array::from(vec![
+            125.0, 125.2, 125.23, 125.235, 125.2345, 125.2345, 130.0, 100.0, 0.0, 0.0,


I spot checked that these answers are consistent with postgres 👍 (including negative)

postgres=# select round(125.2345, -1); round ------- 130 (1 row) postgres=# select round(125.2345, -2); round ------- 100 (1 row) postgres=# select round(125.2345, 3); round --------- 125.235 (1 row)

Co-authored-by: Andrew Lamb <[email protected]>

viirya · 2023-03-31T18:22:22Z

Thanks @alamb. I'll add end to end test to sqllogictests later.

viirya · 2023-03-31T20:18:39Z

Added math.slt to sqllogictests.

alamb · 2023-04-01T10:21:15Z

datafusion/physical-expr/src/math_expressions.rs

+    let mut decimal_places =
+        &(Arc::new(Int64Array::from_value(0, args[0].len())) as ArrayRef);


I agree it is reasonable to start with a working implementation and optimize as a follow on 👍

Can drop this after rebase on commit 771c20c "Support round() function with two parameters (apache#5807)", first released in 22.0.0

Physical round expression supports two parameters

71affd0

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sql SQL Planner labels Mar 30, 2023

viirya commented Mar 30, 2023

View reviewed changes

fix

466caf9

alamb approved these changes Mar 31, 2023

View reviewed changes

Update datafusion/physical-expr/src/math_expressions.rs

f969814

Co-authored-by: Andrew Lamb <[email protected]>

viirya added 2 commits March 31, 2023 12:21

fix format

b424f8f

Add sqllogictests test for math

64aa003

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 31, 2023

alamb approved these changes Apr 1, 2023

View reviewed changes

alamb changed the title ~~Physical round expression supports two parameters~~ Support round() function with two parameters Apr 1, 2023

alamb merged commit 771c20c into apache:main Apr 1, 2023

viirya mentioned this pull request Apr 5, 2023

Use ScalarValue for single input on math expression #5890

Closed

jizezhang mentioned this pull request Nov 4, 2025

Deduplicate Spark function code with native/default datafusion function code #17964

Open

14 tasks

		let mut decimal_places =
		&(Arc::new(Int64Array::from_value(0, args[0].len())) as ArrayRef);

Comments

Conversation

viirya commented Mar 30, 2023 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Mar 30, 2023 • 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!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Mar 31, 2023

Uh oh!

viirya commented Mar 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

viirya commented Mar 30, 2023 •

edited by alamb

Loading

viirya Mar 30, 2023 •

edited

Loading