martin-g commented on code in PR #19788:
URL: https://github.com/apache/datafusion/pull/19788#discussion_r2686349470
##########
datafusion/functions/benches/trunc.rs:
##########
@@ -74,6 +74,52 @@ fn criterion_benchmark(c: &mut Criterion) {
})
});
}
+
+ // Scalar benchmarks - to measure optimized performance
+ let scalar_f64_args = vec![ColumnarValue::Scalar(
+ datafusion_common::ScalarValue::Float64(Some(3.14159265)),
+ )];
+ let scalar_arg_fields = vec![Field::new("a", DataType::Float64,
false).into()];
+ let scalar_return_field = Field::new("f", DataType::Float64, false).into();
+ let config_options = Arc::new(ConfigOptions::default());
Review Comment:
nit: This variable shadows the same one from line 40
##########
datafusion/functions/src/math/trunc.rs:
##########
@@ -110,6 +110,51 @@ impl ScalarUDFImpl for TruncFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
+ let value = &args.args[0];
+
+ // Scalar fast path for float types with scalar or default precision
+ if let ColumnarValue::Scalar(scalar) = value {
+ // Get precision: default 0 or from second scalar arg
+ let precision = if args.args.len() >= 2 {
+ match &args.args[1] {
+ ColumnarValue::Scalar(Int64(Some(p))) => *p,
+ ColumnarValue::Scalar(Int64(None)) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
+ }
+ _ => {
+ // Precision is an array - fall through to array path
+ return make_scalar_function(trunc, vec![])(&args.args);
+ }
+ }
+ } else {
+ 0 // default precision
+ };
+
+ match scalar {
+ ScalarValue::Float64(v) => {
+ let result = v.map(|x| {
+ if precision == 0 {
+ if x == 0.0 { 0.0 } else { x.trunc() }
+ } else {
+ compute_truncate64(x, precision)
+ }
+ });
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(result)));
+ }
+ ScalarValue::Float32(v) => {
+ let result = v.map(|x| {
+ if precision == 0 {
+ if x == 0.0 { 0.0 } else { x.trunc() }
Review Comment:
```suggestion
x.trunc()
```
##########
datafusion/functions/src/math/trunc.rs:
##########
@@ -110,6 +110,51 @@ impl ScalarUDFImpl for TruncFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
+ let value = &args.args[0];
+
+ // Scalar fast path for float types with scalar or default precision
+ if let ColumnarValue::Scalar(scalar) = value {
+ // Get precision: default 0 or from second scalar arg
+ let precision = if args.args.len() >= 2 {
+ match &args.args[1] {
+ ColumnarValue::Scalar(Int64(Some(p))) => *p,
+ ColumnarValue::Scalar(Int64(None)) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
Review Comment:
I think this should check the `scalar` type to decide whether to return
Float64 or Float32
##########
datafusion/functions/src/math/trunc.rs:
##########
@@ -110,6 +110,51 @@ impl ScalarUDFImpl for TruncFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
+ let value = &args.args[0];
+
+ // Scalar fast path for float types with scalar or default precision
+ if let ColumnarValue::Scalar(scalar) = value {
+ // Get precision: default 0 or from second scalar arg
+ let precision = if args.args.len() >= 2 {
+ match &args.args[1] {
+ ColumnarValue::Scalar(Int64(Some(p))) => *p,
+ ColumnarValue::Scalar(Int64(None)) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
+ }
+ _ => {
+ // Precision is an array - fall through to array path
+ return make_scalar_function(trunc, vec![])(&args.args);
+ }
+ }
+ } else {
+ 0 // default precision
+ };
+
+ match scalar {
+ ScalarValue::Float64(v) => {
+ let result = v.map(|x| {
+ if precision == 0 {
+ if x == 0.0 { 0.0 } else { x.trunc() }
Review Comment:
```suggestion
x.trunc()
```
##########
datafusion/functions/src/math/trunc.rs:
##########
Review Comment:
Not introduced in this PR but why `f32::round()` is used here instead of
`f32::trunc()` ?
Same for f64 below.
```rust
fn main() {
let factor = 10_f64;
let r = (3.76_f64 * factor).round() / factor;
let t = (3.76_f64 * factor).trunc() / factor;
println!("round: {r}\ntrunc: {t}");
}
```
prints:
```
round: 3.8
trunc: 3.7
```
##########
datafusion/functions/src/math/trunc.rs:
##########
@@ -110,6 +110,51 @@ impl ScalarUDFImpl for TruncFunc {
}
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
+ let value = &args.args[0];
+
+ // Scalar fast path for float types with scalar or default precision
+ if let ColumnarValue::Scalar(scalar) = value {
+ // Get precision: default 0 or from second scalar arg
+ let precision = if args.args.len() >= 2 {
+ match &args.args[1] {
+ ColumnarValue::Scalar(Int64(Some(p))) => *p,
+ ColumnarValue::Scalar(Int64(None)) => {
+ return
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)));
+ }
+ _ => {
+ // Precision is an array - fall through to array path
+ return make_scalar_function(trunc, vec![])(&args.args);
+ }
+ }
+ } else {
+ 0 // default precision
+ };
+
+ match scalar {
+ ScalarValue::Float64(v) => {
Review Comment:
Fast path for `ScalarValue::Null` too ?!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
