Skip to content

Support round() function with two parameters #5807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 1, 2023
Merged

Support round() function with two parameters #5807

merged 5 commits into from
Apr 1, 2023

Conversation

viirya
Copy link
Member

@viirya viirya commented Mar 30, 2023 •
edited by alamb
Loading

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?

@github-actions 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
viirya commented Mar 30, 2023
View reviewed changes
datafusion/sql/tests/integration_test.rs Outdated
Comment on lines 203 to 206
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are by rust-fmt, I think.

viirya
viirya commented Mar 30, 2023
View reviewed changes
datafusion/sql/tests/integration_test.rs Outdated
Comment on lines 2888 to 2893
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);
}
Copy link
Member Author

@viirya viirya Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file, I only manually changed this test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

alamb
alamb approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment on lines +171 to +172
let mut decimal_places =
&(Arc::new(Int64Array::from_value(0, args[0].len())) as ArrayRef);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, not sure if it is worth.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@viirya
Copy link
Member Author

viirya commented Mar 31, 2023

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

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 31, 2023
@viirya
Copy link
Member Author

viirya commented Mar 31, 2023

Added math.slt to sqllogictests.

alamb
alamb approved these changes Apr 1, 2023
View reviewed changes
datafusion/physical-expr/src/math_expressions.rs
Comment on lines +171 to +172
let mut decimal_places =
&(Arc::new(Int64Array::from_value(0, args[0].len())) as ArrayRef);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alamb alamb changed the title Physical round expression supports two parameters Support round() function with two parameters Apr 1, 2023
@alamb alamb merged commit 771c20c into apache:main Apr 1, 2023
@viirya viirya mentioned this pull request Apr 5, 2023
Closed
mcheshkov added a commit to cube-js/arrow-datafusion that referenced this pull request May 12, 2025
@mcheshkov
Support round() function with two parameters (apache#5807)
459bd0e 
Can drop this after rebase on commit 771c20c "Support round() function with two parameters (apache#5807)", first released in 22.0.0
mcheshkov added a commit to cube-js/arrow-datafusion that referenced this pull request May 19, 2025
@mcheshkov
Support round() function with two parameters (apache#5807)
ab4a1ae 
Can drop this after rebase on commit 771c20c "Support round() function with two parameters (apache#5807)", first released in 22.0.0
mcheshkov added a commit to cube-js/arrow-datafusion that referenced this pull request May 19, 2025
@mcheshkov
Support round() function with two parameters (apache#5807)
961e2f4 
Can drop this after rebase on commit 771c20c "Support round() function with two parameters (apache#5807)", first released in 22.0.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
2 participants
@viirya @alamb