Skip to content

Constant fold / optimize to_timestamp function during planning #387

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

Merged
merged 7 commits into from
May 25, 2021
Merged

Constant fold / optimize to_timestamp function during planning #387

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae0785a
optimize to_timestamp
msathis May 22, 2021
ff2a20b
cargo fmt
msathis May 22, 2021
c32d2f5
fix clippy
msathis May 22, 2021
43d01ef
Added testcase & removed optimization for invalid timestamps
msathis May 24, 2021
6663671
Added negative testcase
msathis May 24, 2021
21acdb5
Fix clippy
msathis May 25, 2021
2b9b9b7
fix clippy
alamb May 25, 2021
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
92 changes: 92 additions & 0 deletions datafusion/src/optimizer/constant_folding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use crate::optimizer::optimizer::OptimizerRule;
use crate::optimizer::utils;
use crate::physical_plan::functions::BuiltinScalarFunction;
use crate::scalar::ScalarValue;
use arrow::compute::kernels::cast_utils::string_to_timestamp_nanos;

/// Optimizer that simplifies comparison expressions involving boolean literals.
///
Expand Down Expand Up @@ -217,6 +218,35 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
Expr::ScalarFunction {
Copy link
Contributor

@alamb alamb May 23, 2021

Choose a reason for hiding this comment

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

Matching on the literal argument is good as the rewrite pass happens after all children are visited 👍

fun: BuiltinScalarFunction::ToTimestamp,
args,
} => {
if !args.is_empty() {
match &args[0] {
Expr::Literal(ScalarValue::Utf8(Some(val))) => {
match string_to_timestamp_nanos(val) {
Ok(timestamp) => Expr::Literal(
ScalarValue::TimestampNanosecond(Some(timestamp)),
),
_ => Expr::ScalarFunction {
fun: BuiltinScalarFunction::ToTimestamp,
args,
},
}
}
_ => Expr::ScalarFunction {
fun: BuiltinScalarFunction::ToTimestamp,
args,
},
}
} else {
Expr::ScalarFunction {
fun: BuiltinScalarFunction::ToTimestamp,
args,
}
}
}
Copy link
Contributor

@NGA-TRAN NGA-TRAN May 24, 2021

Choose a reason for hiding this comment

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

What happen if the args.is_empty()? Do we use a default value or return syntax error we simply return false for this expression?

Copy link
Contributor Author

@msathis msathis May 24, 2021

Choose a reason for hiding this comment

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

In case of all error cases including empty, we don't optimise. So the default non-optimised flow continues.

expr => {
// no rewrite possible
expr
Expand Down Expand Up @@ -632,6 +662,68 @@ mod tests {
return format!("{:?}", optimized_plan);
}

#[test]
fn to_timestamp_expr() {
let table_scan = test_table_scan().unwrap();
let proj = vec![Expr::ScalarFunction {
args: vec![Expr::Literal(ScalarValue::Utf8(Some(
"2020-09-08T12:00:00+00:00".to_string(),
)))],
fun: BuiltinScalarFunction::ToTimestamp,
}];
let plan = LogicalPlanBuilder::from(&table_scan)
.project(proj)
.unwrap()
.build()
.unwrap();

let expected = "Projection: TimestampNanosecond(1599566400000000000)\
\n TableScan: test projection=None"
.to_string();
let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
assert_eq!(expected, actual);
}

#[test]
fn to_timestamp_expr_wrong_arg() {
let table_scan = test_table_scan().unwrap();
let proj = vec![Expr::ScalarFunction {
args: vec![Expr::Literal(ScalarValue::Utf8(Some(
"I'M NOT A TIMESTAMP".to_string(),
Copy link
Contributor

@alamb alamb May 24, 2021

Choose a reason for hiding this comment

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

😆

)))],
fun: BuiltinScalarFunction::ToTimestamp,
}];
let plan = LogicalPlanBuilder::from(&table_scan)
.project(proj)
.unwrap()
.build()
.unwrap();

let expected = "Projection: totimestamp(Utf8(\"I\'M NOT A TIMESTAMP\"))\
\n TableScan: test projection=None";
let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
assert_eq!(expected, actual);
}

#[test]
fn to_timestamp_expr_no_arg() {
let table_scan = test_table_scan().unwrap();
let proj = vec![Expr::ScalarFunction {
args: vec![],
fun: BuiltinScalarFunction::ToTimestamp,
}];
let plan = LogicalPlanBuilder::from(&table_scan)
.project(proj)
.unwrap()
.build()
.unwrap();

let expected = "Projection: totimestamp()\
\n TableScan: test projection=None";
let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
assert_eq!(expected, actual);
}

#[test]
fn single_now_expr() {
let table_scan = test_table_scan().unwrap();
Expand Down