Skip to content
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

Optimizer now simplifies multiplication, division, module arg is a literal Decimal zero or one #3782

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Optimizer now simplifies multiplication, division, module arg is a literal Decimal zero or one #3782

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c531481
Optimizer now simplifies multiplication when either left or right arg…
drrtuy Oct 10, 2022
47a13f5
Merge remote-tracking branch 'apache/master' into simpl_mul_div_mod_d…
alamb Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 92 additions & 3 deletions datafusion/optimizer/src/simplify_expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
use crate::expr_simplifier::{ExprSimplifier, SimplifyContext};
use crate::{expr_simplifier::SimplifyInfo, OptimizerConfig, OptimizerRule};
use arrow::array::new_null_array;
use arrow::datatypes::{DataType, Field, Schema};
use arrow::datatypes::{DataType, Field, Schema, DECIMAL128_MAX_PRECISION};
use arrow::error::ArrowError;
use arrow::record_batch::RecordBatch;
use datafusion_common::{DFSchema, DataFusionError, Result, ScalarValue};
Expand All @@ -34,6 +34,47 @@ use datafusion_expr::{
};
use datafusion_physical_expr::{create_physical_expr, execution_props::ExecutionProps};

static POWS_OF_TEN: [i128; 38] = [
1,
10,
100,
1000,
10000,
100000,
1000000,
10000000,
100000000,
1000000000,
10000000000,
100000000000,
1000000000000,
10000000000000,
100000000000000,
1000000000000000,
10000000000000000,
100000000000000000,
1000000000000000000,
10000000000000000000,
100000000000000000000,
1000000000000000000000,
10000000000000000000000,
100000000000000000000000,
1000000000000000000000000,
10000000000000000000000000,
100000000000000000000000000,
1000000000000000000000000000,
10000000000000000000000000000,
100000000000000000000000000000,
1000000000000000000000000000000,
10000000000000000000000000000000,
100000000000000000000000000000000,
1000000000000000000000000000000000,
10000000000000000000000000000000000,
100000000000000000000000000000000000,
1000000000000000000000000000000000000,
10000000000000000000000000000000000000,
];

/// Optimizer Pass that simplifies [`LogicalPlan`]s by rewriting
/// [`Expr`]`s evaluating constants and applying algebraic
/// simplifications
Expand Down Expand Up @@ -73,6 +114,7 @@ fn is_zero(s: &Expr) -> bool {
| Expr::Literal(ScalarValue::UInt64(Some(0))) => true,
Expr::Literal(ScalarValue::Float32(Some(v))) if *v == 0. => true,
Expr::Literal(ScalarValue::Float64(Some(v))) if *v == 0. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, _s)) if *v == 0 => true,
_ => false,
}
}
Expand All @@ -89,6 +131,9 @@ fn is_one(s: &Expr) -> bool {
| Expr::Literal(ScalarValue::UInt64(Some(1))) => true,
Expr::Literal(ScalarValue::Float32(Some(v))) if *v == 1. => true,
Expr::Literal(ScalarValue::Float64(Some(v))) if *v == 1. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, _s)) => {
*_s < DECIMAL128_MAX_PRECISION && POWS_OF_TEN[*_s as usize] == *v
Copy link
Contributor

@isidentical isidentical Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Question: Is there a specific reason to embed a POWS_OF_TEN array instead of dynamically calculating it at the runtime? (or the same goes to taking to log10() of the v, instead of raising _s) E.g.

i128::pow(10, *_s as u32) == *v

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pow call make the patch lesser but will do a real multiplication instead of a pointer deref.
This is perf degradaton IMHO. In the worst case there will be 6 such multiplications(2 for Multiply, 2 for Divide, 2 for Modulo) and there might be multiple scalars binary ops in the expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other side I don't see much benefit from using pow except there will be no static array. Not a big profit given a potential perf gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is your argument in support for using pow() though?

Copy link
Contributor

@alamb alamb Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this code will run ~1 per query / expression, so I don't expect the any performance difference to be measurable.

For what it is worth, I think both approaches are reasonable. Thank you both for the discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the function or the logic will be call many time in the runtime, it is better to use static value or const value which will get good performance.

}
_ => false,
}
}
Expand Down Expand Up @@ -1031,6 +1076,19 @@ mod tests {

assert_eq!(simplify(expr_a), expected);
assert_eq!(simplify(expr_b), expected);

let expr = binary_expr(
col("c2"),
Operator::Multiply,
Expr::Literal(ScalarValue::Decimal128(Some(10000000000), 38, 10)),
);
assert_eq!(simplify(expr), expected);
let expr = binary_expr(
Expr::Literal(ScalarValue::Decimal128(Some(10000000000), 31, 10)),
Operator::Multiply,
col("c2"),
);
assert_eq!(simplify(expr), expected);
}

#[test]
Expand Down Expand Up @@ -1068,13 +1126,39 @@ mod tests {
let expr = binary_expr(col("c2_non_null"), Operator::Multiply, lit(0));
assert_eq!(simplify(expr), lit(0));
}
// A * Decimal128(0) --> 0 if A is not nullable
{
let expr = binary_expr(
col("c2_non_null"),
Operator::Multiply,
Expr::Literal(ScalarValue::Decimal128(Some(0), 31, 10)),
);
assert_eq!(
simplify(expr),
Expr::Literal(ScalarValue::Decimal128(Some(0), 31, 10))
);
let expr = binary_expr(
Expr::Literal(ScalarValue::Decimal128(Some(0), 31, 10)),
Operator::Multiply,
col("c2_non_null"),
);
assert_eq!(
simplify(expr),
Expr::Literal(ScalarValue::Decimal128(Some(0), 31, 10))
);
}
}

#[test]
fn test_simplify_divide_by_one() {
let expr = binary_expr(col("c2"), Operator::Divide, lit(1));
let expected = col("c2");

assert_eq!(simplify(expr), expected);
let expr = binary_expr(
col("c2"),
Operator::Divide,
Expr::Literal(ScalarValue::Decimal128(Some(10000000000), 31, 10)),
);
assert_eq!(simplify(expr), expected);
}

Expand Down Expand Up @@ -1138,7 +1222,12 @@ mod tests {
fn test_simplify_modulo_by_one_non_null() {
let expr = binary_expr(col("c2_non_null"), Operator::Modulo, lit(1));
let expected = lit(0);

assert_eq!(simplify(expr), expected);
let expr = binary_expr(
col("c2_non_null"),
Operator::Modulo,
Expr::Literal(ScalarValue::Decimal128(Some(10000000000), 31, 10)),
);
assert_eq!(simplify(expr), expected);
}

Expand Down