functions: support trunc() function with one or two args #6942

Syleechan · 2023-07-13T07:00:00Z

Which issue does this PR close?

Closes #.

Rationale for this change

before change the trunc function only with one arg and truncate the number after the decimal point
after change the trunc function with two args, and can truncate the decimal customarily, see the detail from the link below
https://www.postgresql.org/docs/current/functions-math.html#:~:text=trunc%20(%20numeric,2)%20%E2%86%92%2042.43

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

liukun4515 · 2023-07-13T07:02:09Z

can describe the issue？

Syleechan · 2023-07-13T07:20:54Z

can describe the issue？

see the relationale for this change

alamb · 2023-07-16T13:56:39Z

THanks @Syleechan -- I plan to review this over the next day or two!

alamb

Thank you for the contribution @Syleechan

I left some style suggestions, but overall I think this PR is close

The only thing I think is needed to merge is some SQL level tests -- perhaps you could add some sqllogic tests (readme is here)

https://github.com/apache/arrow-datafusion/blob/9338880d8f64f8143e348a60beee8af2789fa8ae/datafusion/core/tests/sqllogictests/test_files/scalar.slt#L892-L914 along side the 1 argument version

Also, I think it would be good to include a test of calling trunc(1, 2, 3) with more than two arguments and ensure we get a Planning or Execution error rather than an InternalError. Internal errors signal a bug in DataFusion

alamb · 2023-07-17T21:29:27Z

datafusion/expr/src/expr_fn.rs

@@ -501,7 +501,7 @@ scalar_expr!(
 scalar_expr!(Degrees, degrees, num, "converts radians to degrees");
 scalar_expr!(Radians, radians, num, "converts degrees to radians");
 nary_scalar_expr!(Round, round, "round to nearest integer");
-scalar_expr!(Trunc, trunc, num, "truncate toward zero");
+nary_scalar_expr!(Trunc, trunc, "truncate toward to precision");


I don't think this function truncates toward precision -- instead it truncates towards zero with an optional precision

Suggested change

nary_scalar_expr!(Trunc, trunc, "truncate toward to precision");

nary_scalar_expr!(Trunc, trunc, "truncate toward zero, with optional precision");

alamb · 2023-07-17T21:31:46Z

datafusion/physical-expr/src/math_expressions.rs

+    let mut _precision = ColumnarValue::Scalar(Int32(Some(0)));
+
+    let num = &args[0];
+    if args.len() == 2 {
+        _precision = ColumnarValue::Array(args[1].clone());
+    }


The use of _ as a prefix of a variable is used to tell the compiler it can be unused. In this case I think it is used and thus the variable should be named precision

A more "idiomatic" rust way (and avoiding the mut) of writing this might be

Suggested change

let mut _precision = ColumnarValue::Scalar(Int32(Some(0)));

let num = &args[0];

if args.len() == 2 {

_precision = ColumnarValue::Array(args[1].clone());

}

let precision = if args.len() == 1 {

ColumnarValue::Scalar(Int32(Some(0)))

} else {

ColumnarValue::Array(args[1].clone())

};

datafusion/physical-expr/src/math_expressions.rs

alamb · 2023-07-17T21:38:50Z

datafusion/physical-expr/src/math_expressions.rs

+}
+
+fn compute_truncate32(x: f32, y: usize) -> f32 {
+    let s = format!("{:.precision$}", x, precision = y);


You can probably make this quite a bit more efficient by avoiding strings, for example something like (untested):

let factor = 10.0f32.pow(precision); (x * factor).round() / factor;

# Conflicts: # datafusion/physical-expr/src/math_expressions.rs

Syleechan · 2023-07-18T12:09:35Z

thanks for your suggestion.Now I try with sql level test.But it shows with the message:

Running "scalar.slt"
External error: query failed: DataFusion error: Error during planning: No function matches the given name and argument types 'trunc(Float64, Int64)'. You might need to add explicit type casts.
        Candidate functions:
        trunc(Float64/Float32)

I am not familiar with the sql test function, so can you tell me how to deal with the error. @alamb

alamb · 2023-07-18T20:50:51Z

I am not familiar with the sql test function, so can you tell me how to deal with the error. @alamb

I can help, but probably not until tomorrow.

I don't think the error you are seeing is related to the testing function, it is related somehow to how the function is registered with datafusion

Syleechan · 2023-07-19T02:40:52Z

I am not familiar with the sql test function, so can you tell me how to deal with the error. @alamb

I can help, but probably not until tomorrow.

I don't think the error you are seeing is related to the testing function, it is related somehow to how the function is registered with datafusion

thanks for your help.

Syleechan · 2023-07-20T04:11:50Z

I have found the issue and fixed it, please help to review again, thanks @alamb

alamb

This looks good to me @Syleechan -- thank you very much.

alamb · 2023-07-20T17:03:13Z

datafusion/expr/src/built_in_function.rs

@@ -1072,6 +1072,15 @@ impl BuiltinScalarFunction {
                ],
                self.volatility(),
            ),
+            BuiltinScalarFunction::Trunc => Signature::one_of(


alamb · 2023-07-20T17:14:14Z

here is a follow on PR #7042 with test and doc updates

functions: support trunc() function with one or two args

3c87dee

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Jul 13, 2023

format code style

0cf022f

Syleechan added 3 commits July 13, 2023 20:26

modify truncate method

1fd4276

Merge remote-tracking branch 'upstream/main' into arithmetic_truncate

435f5ac

adjust code format

695922f

alamb reviewed Jul 17, 2023

View reviewed changes

Syleechan added 2 commits July 18, 2023 16:48

Merge remote-tracking branch 'upstream/main' into arithmetic_truncate

3d2abbb

# Conflicts: # datafusion/physical-expr/src/math_expressions.rs

Merge remote-tracking branch 'upstream/main' into arithmetic_truncate

d2854c3

format code

afe5867

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

Syleechan added 2 commits July 20, 2023 10:24

Merge remote-tracking branch 'upstream/main' into arithmetic_truncate

7346c1d

fix sql test error

9f49ca4

Syleechan requested a review from alamb July 20, 2023 04:11

alamb approved these changes Jul 20, 2023

View reviewed changes

alamb merged commit f05df7b into apache:main Jul 20, 2023
20 checks passed

alamb mentioned this pull request Jul 20, 2023

Minor: add test + docs for 2 argument trunc with columns #7042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

functions: support trunc() function with one or two args #6942

functions: support trunc() function with one or two args #6942

Syleechan commented Jul 13, 2023 •

edited

Loading

liukun4515 commented Jul 13, 2023

Syleechan commented Jul 13, 2023

alamb commented Jul 16, 2023

alamb left a comment

alamb Jul 17, 2023

alamb Jul 17, 2023

alamb Jul 17, 2023

Syleechan Jul 18, 2023

Syleechan commented Jul 18, 2023 •

edited

Loading

alamb commented Jul 18, 2023

Syleechan commented Jul 19, 2023

Syleechan commented Jul 20, 2023

alamb left a comment

alamb Jul 20, 2023

alamb commented Jul 20, 2023

	nary_scalar_expr!(Trunc, trunc, "truncate toward to precision");
	nary_scalar_expr!(Trunc, trunc, "truncate toward zero, with optional precision");

functions: support trunc() function with one or two args #6942

functions: support trunc() function with one or two args #6942

Conversation

Syleechan commented Jul 13, 2023 • edited Loading

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?

liukun4515 commented Jul 13, 2023

Syleechan commented Jul 13, 2023

alamb commented Jul 16, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 17, 2023

Choose a reason for hiding this comment

alamb Jul 17, 2023

Choose a reason for hiding this comment

alamb Jul 17, 2023

Choose a reason for hiding this comment

Syleechan Jul 18, 2023

Choose a reason for hiding this comment

Syleechan commented Jul 18, 2023 • edited Loading

alamb commented Jul 18, 2023

Syleechan commented Jul 19, 2023

Syleechan commented Jul 20, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 20, 2023

Choose a reason for hiding this comment

alamb commented Jul 20, 2023

Syleechan commented Jul 13, 2023 •

edited

Loading

Syleechan commented Jul 18, 2023 •

edited

Loading