You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/10 10:18:29 UTC
[GitHub] [arrow-datafusion] Dandandan opened a new pull request #309: Simplify math expression code (use unary kernel)
Dandandan opened a new pull request #309:
URL: https://github.com/apache/arrow-datafusion/pull/309
# Which issue does this PR close?
Closes #308
# Rationale for this change
Simplifies code, probably is faster too. Also involves one cast less which causes more rounding errors.
# What changes are included in this PR?
Use `arity::unary` function to simplify math expression code
# Are there any user-facing changes?
No
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #309: Simplify math expression code (use unary kernel)
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #309:
URL: https://github.com/apache/arrow-datafusion/pull/309#issuecomment-836651151
🎉
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #309: Simplify math expression code (use unary kernel)
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #309:
URL: https://github.com/apache/arrow-datafusion/pull/309#discussion_r629241895
##########
File path: datafusion/src/physical_plan/math_expressions.rs
##########
@@ -17,37 +17,21 @@
//! Math expressions
-use arrow::array::{make_array, Array, ArrayData, Float32Array, Float64Array};
-use arrow::buffer::Buffer;
-use arrow::datatypes::{DataType, ToByteSlice};
-
use super::{ColumnarValue, ScalarValue};
use crate::error::{DataFusionError, Result};
-
-macro_rules! compute_op {
- ($ARRAY:expr, $FUNC:ident, $TYPE:ident) => {{
- let len = $ARRAY.len();
- let result = (0..len)
- .map(|i| $ARRAY.value(i).$FUNC() as f64)
- .collect::<Vec<f64>>();
- let data = ArrayData::new(
- DataType::Float64,
- len,
- Some($ARRAY.null_count()),
- $ARRAY.data().null_buffer().cloned(),
- 0,
- vec![Buffer::from(result.to_byte_slice())],
- vec![],
- );
- Ok(make_array(data))
- }};
-}
+use arrow::array::{Float32Array, Float64Array};
+use arrow::datatypes::DataType;
+use std::sync::Arc;
macro_rules! downcast_compute_op {
($ARRAY:expr, $NAME:expr, $FUNC:ident, $TYPE:ident) => {{
let n = $ARRAY.as_any().downcast_ref::<$TYPE>();
match n {
- Some(array) => compute_op!(array, $FUNC, $TYPE),
+ Some(array) => {
+ let res: $TYPE =
+ arrow::compute::kernels::arity::unary(array, |x| x.$FUNC());
Review comment:
This avoids creating intermediate `Vec` and uses the (efficient) unary kernel
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan merged pull request #309: Simplify math expression code (use unary kernel)
Posted by GitBox <gi...@apache.org>.
Dandandan merged pull request #309:
URL: https://github.com/apache/arrow-datafusion/pull/309
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #309: Simplify math expression code (use unary kernel)
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #309:
URL: https://github.com/apache/arrow-datafusion/pull/309#discussion_r629241387
##########
File path: datafusion/tests/sql.rs
##########
@@ -631,7 +631,7 @@ async fn sqrt_f32_vs_f64() -> Result<()> {
// sqrt(f32)'s plan passes
let sql = "SELECT avg(sqrt(c11)) FROM aggregate_test_100";
let actual = execute(&mut ctx, sql).await;
- let expected = vec![vec!["0.6584408485889435"]];
+ let expected = vec![vec!["0.6584407806396484"]];
Review comment:
Small diff, since we avoid `as f64` now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #309: Simplify math expression code (use unary kernel)
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #309:
URL: https://github.com/apache/arrow-datafusion/pull/309#discussion_r629241387
##########
File path: datafusion/tests/sql.rs
##########
@@ -631,7 +631,7 @@ async fn sqrt_f32_vs_f64() -> Result<()> {
// sqrt(f32)'s plan passes
let sql = "SELECT avg(sqrt(c11)) FROM aggregate_test_100";
let actual = execute(&mut ctx, sql).await;
- let expected = vec![vec!["0.6584408485889435"]];
+ let expected = vec![vec!["0.6584407806396484"]];
Review comment:
Small diff, Isince we avoid `as f64` now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org