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.

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

Implement Display for Expr, improve operator display #971

Merged
merged 4 commits into from
Sep 22, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
18 changes: 9 additions & 9 deletions datafusion/src/execution/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1696,15 +1696,15 @@ mod tests {
.await?;

let expected = vec![
"+----+----+--------------+-----------------------------------+----------------------------------+------------------------------------------+--------------+----------------+--------------+--------------+--------------+",
"| c1 | c2 | ROW_NUMBER() | FIRST_VALUE(test.c2 Plus test.c1) | LAST_VALUE(test.c2 Plus test.c1) | NTH_VALUE(test.c2 Plus test.c1,Int64(1)) | SUM(test.c2) | COUNT(test.c2) | MAX(test.c2) | MIN(test.c2) | AVG(test.c2) |",
"+----+----+--------------+-----------------------------------+----------------------------------+------------------------------------------+--------------+----------------+--------------+--------------+--------------+",
"| 0 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 |",
"| 0 | 2 | 1 | 2 | 2 | 2 | 2 | 1 | 2 | 2 | 2 |",
"| 0 | 3 | 1 | 3 | 3 | 3 | 3 | 1 | 3 | 3 | 3 |",
"| 0 | 4 | 1 | 4 | 4 | 4 | 4 | 1 | 4 | 4 | 4 |",
"| 0 | 5 | 1 | 5 | 5 | 5 | 5 | 1 | 5 | 5 | 5 |",
"+----+----+--------------+-----------------------------------+----------------------------------+------------------------------------------+--------------+----------------+--------------+--------------+--------------+",
"+----+----+--------------+--------------------------------+-------------------------------+---------------------------------------+--------------+----------------+--------------+--------------+--------------+",
"| c1 | c2 | ROW_NUMBER() | FIRST_VALUE(test.c2 + test.c1) | LAST_VALUE(test.c2 + test.c1) | NTH_VALUE(test.c2 + test.c1,Int64(1)) | SUM(test.c2) | COUNT(test.c2) | MAX(test.c2) | MIN(test.c2) | AVG(test.c2) |",
"+----+----+--------------+--------------------------------+-------------------------------+---------------------------------------+--------------+----------------+--------------+--------------+--------------+",
"| 0 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 |",
"| 0 | 2 | 1 | 2 | 2 | 2 | 2 | 1 | 2 | 2 | 2 |",
"| 0 | 3 | 1 | 3 | 3 | 3 | 3 | 1 | 3 | 3 | 3 |",
"| 0 | 4 | 1 | 4 | 4 | 4 | 4 | 1 | 4 | 4 | 4 |",
"| 0 | 5 | 1 | 5 | 5 | 5 | 5 | 1 | 5 | 5 | 5 |",
"+----+----+--------------+--------------------------------+-------------------------------+---------------------------------------+--------------+----------------+--------------+--------------+--------------+",
];

// window function shall respect ordering
Expand Down
2 changes: 1 addition & 1 deletion datafusion/src/logical_plan/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ mod tests {
.build()?;

let expected = "Projection: #employee_csv.id\
\n Filter: #employee_csv.state Eq Utf8(\"CO\")\
\n Filter: #employee_csv.state = Utf8(\"CO\")\
\n TableScan: employee_csv projection=Some([0, 3])";

assert_eq!(expected, format!("{:?}", plan));
Expand Down
53 changes: 43 additions & 10 deletions datafusion/src/logical_plan/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -941,6 +941,33 @@ impl Not for Expr {
}
}

impl std::fmt::Display for Expr {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Expr::BinaryExpr {
ref left,
ref right,
ref op,
} => write!(f, "{} {} {}", left, op, right),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used at the moment, as the implementation uses debug.

Expr::AggregateFunction {
/// Name of the function
ref fun,
/// List of expressions to feed to the functions as arguments
ref args,
/// Whether this is a DISTINCT aggregation or not
ref distinct,
} => fmt_function(f, &fun.to_string(), *distinct, args, true),
Expr::ScalarFunction {
/// Name of the function
ref fun,
/// List of expressions to feed to the functions as arguments
ref args,
} => fmt_function(f, &fun.to_string(), false, args, true),
_ => write!(f, "{:?}", self),
}
}
}

#[allow(clippy::boxed_local)]
fn rewrite_boxed<R>(boxed_expr: Box<Expr>, rewriter: &mut R) -> Result<Box<Expr>>
where
Expand Down Expand Up @@ -1572,8 +1599,14 @@ fn fmt_function(
fun: &str,
distinct: bool,
args: &[Expr],
display: bool,
) -> fmt::Result {
let args: Vec<String> = args.iter().map(|arg| format!("{:?}", arg)).collect();
let args: Vec<String> = match display {
true => args.iter().map(|arg| format!("{}", arg)).collect(),
false => args.iter().map(|arg| format!("{:?}", arg)).collect(),
};

// let args: Vec<String> = args.iter().map(|arg| format!("{:?}", arg)).collect();
let distinct_str = match distinct {
true => "DISTINCT ",
false => "",
Expand Down Expand Up @@ -1617,7 +1650,7 @@ impl fmt::Debug for Expr {
Expr::IsNull(expr) => write!(f, "{:?} IS NULL", expr),
Expr::IsNotNull(expr) => write!(f, "{:?} IS NOT NULL", expr),
Expr::BinaryExpr { left, op, right } => {
write!(f, "{:?} {:?} {:?}", left, op, right)
write!(f, "{:?} {} {:?}", left, op, right)
Comment on lines 1652 to +1653
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dandandan FYI i updated here to use display and i think thats what made it work. i was able to get the expected results on some tests after this as well.

Are you ok with this approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be ok with this as intermediate solution. Ideally we'll move the formatting to Display instead and use that for printing @alamb wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @Dandandan

}
Expr::Sort {
expr,
Expand All @@ -1636,10 +1669,10 @@ impl fmt::Debug for Expr {
}
}
Expr::ScalarFunction { fun, args, .. } => {
fmt_function(f, &fun.to_string(), false, args)
fmt_function(f, &fun.to_string(), false, args, false)
}
Expr::ScalarUDF { fun, ref args, .. } => {
fmt_function(f, &fun.name, false, args)
fmt_function(f, &fun.name, false, args, false)
}
Expr::WindowFunction {
fun,
Expand All @@ -1648,7 +1681,7 @@ impl fmt::Debug for Expr {
order_by,
window_frame,
} => {
fmt_function(f, &fun.to_string(), false, args)?;
fmt_function(f, &fun.to_string(), false, args, false)?;
if !partition_by.is_empty() {
write!(f, " PARTITION BY {:?}", partition_by)?;
}
Expand All @@ -1671,9 +1704,9 @@ impl fmt::Debug for Expr {
distinct,
ref args,
..
} => fmt_function(f, &fun.to_string(), *distinct, args),
} => fmt_function(f, &fun.to_string(), *distinct, args, true),
Expr::AggregateUDF { fun, ref args, .. } => {
fmt_function(f, &fun.name, false, args)
fmt_function(f, &fun.name, false, args, false)
}
Expr::Between {
expr,
Expand Down Expand Up @@ -1731,7 +1764,7 @@ fn create_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
Expr::BinaryExpr { left, op, right } => {
let left = create_name(left, input_schema)?;
let right = create_name(right, input_schema)?;
Ok(format!("{} {:?} {}", left, op, right))
Ok(format!("{} {} {}", left, op, right))
}
Expr::Case {
expr,
Expand Down Expand Up @@ -1879,12 +1912,12 @@ mod tests {
assert_eq!(
rewriter.v,
vec![
"Previsited #state Eq Utf8(\"CO\")",
"Previsited #state = Utf8(\"CO\")",
"Previsited #state",
"Mutated #state",
"Previsited Utf8(\"CO\")",
"Mutated Utf8(\"CO\")",
"Mutated #state Eq Utf8(\"CO\")"
"Mutated #state = Utf8(\"CO\")"
]
)
}
Expand Down
8 changes: 4 additions & 4 deletions datafusion/src/logical_plan/operators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,19 @@ mod tests {
fn test_operators() {
assert_eq!(
format!("{:?}", lit(1u32) + lit(2u32)),
"UInt32(1) Plus UInt32(2)"
"UInt32(1) + UInt32(2)"
Copy link
Contributor

Choose a reason for hiding this comment

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

that sure looks better

);
assert_eq!(
format!("{:?}", lit(1u32) - lit(2u32)),
"UInt32(1) Minus UInt32(2)"
"UInt32(1) - UInt32(2)"
);
assert_eq!(
format!("{:?}", lit(1u32) * lit(2u32)),
"UInt32(1) Multiply UInt32(2)"
"UInt32(1) * UInt32(2)"
);
assert_eq!(
format!("{:?}", lit(1u32) / lit(2u32)),
"UInt32(1) Divide UInt32(2)"
"UInt32(1) / UInt32(2)"
);
}
}
10 changes: 5 additions & 5 deletions datafusion/src/logical_plan/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ impl LogicalPlan {
/// // Format using display_indent
/// let display_string = format!("{}", plan.display_indent());
///
/// assert_eq!("Filter: #foo_csv.id Eq Int32(5)\
/// assert_eq!("Filter: #foo_csv.id = Int32(5)\
/// \n TableScan: foo_csv projection=None",
/// display_string);
/// ```
Expand All @@ -575,7 +575,7 @@ impl LogicalPlan {
///
/// ```text
/// Projection: #employee.id [id:Int32]\
/// Filter: #employee.state Eq Utf8(\"CO\") [id:Int32, state:Utf8]\
/// Filter: #employee.state = Utf8(\"CO\") [id:Int32, state:Utf8]\
/// TableScan: employee projection=Some([0, 3]) [id:Int32, state:Utf8]";
/// ```
///
Expand All @@ -592,7 +592,7 @@ impl LogicalPlan {
/// // Format using display_indent_schema
/// let display_string = format!("{}", plan.display_indent_schema());
///
/// assert_eq!("Filter: #foo_csv.id Eq Int32(5) [id:Int32]\
/// assert_eq!("Filter: #foo_csv.id = Int32(5) [id:Int32]\
/// \n TableScan: foo_csv projection=None [id:Int32]",
/// display_string);
/// ```
Expand Down Expand Up @@ -939,7 +939,7 @@ mod tests {
let plan = display_plan();

let expected = "Projection: #employee_csv.id\
\n Filter: #employee_csv.state Eq Utf8(\"CO\")\
\n Filter: #employee_csv.state = Utf8(\"CO\")\
\n TableScan: employee_csv projection=Some([0, 3])";

assert_eq!(expected, format!("{}", plan.display_indent()));
Expand All @@ -950,7 +950,7 @@ mod tests {
let plan = display_plan();

let expected = "Projection: #employee_csv.id [id:Int32]\
\n Filter: #employee_csv.state Eq Utf8(\"CO\") [id:Int32, state:Utf8]\
\n Filter: #employee_csv.state = Utf8(\"CO\") [id:Int32, state:Utf8]\
\n TableScan: employee_csv projection=Some([0, 3]) [id:Int32, state:Utf8]";

assert_eq!(expected, format!("{}", plan.display_indent_schema()));
Expand Down
16 changes: 8 additions & 8 deletions datafusion/src/optimizer/common_subexpr_eliminate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -714,8 +714,8 @@ mod test {
)?
.build()?;

let expected = "Aggregate: groupBy=[[]], aggr=[[SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a Multiply Int32(1) Minus test.b), SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a Multiply Int32(1) Minus test.b Multiply Int32(1) Plus #test.c)]]\
\n Projection: #test.a Multiply Int32(1) Minus #test.b AS BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a, #test.a, #test.b, #test.c\
let expected = "Aggregate: groupBy=[[]], aggr=[[SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a * Int32(1) - test.b), SUM(#BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a AS test.a * Int32(1) - test.b * Int32(1) + #test.c)]]\
\n Projection: #test.a * Int32(1) - #test.b AS BinaryExpr-*BinaryExpr--Column-test.bLiteral1Column-test.a, #test.a, #test.b, #test.c\
\n TableScan: test projection=None";

assert_optimized_plan_eq(&plan, expected);
Expand All @@ -737,7 +737,7 @@ mod test {
)?
.build()?;

let expected = "Aggregate: groupBy=[[]], aggr=[[Int32(1) Plus #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a), Int32(1) Minus #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a)]]\
let expected = "Aggregate: groupBy=[[]], aggr=[[Int32(1) + #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a), Int32(1) - #AggregateFunction-AVGfalseColumn-test.a AS AVG(test.a)]]\
\n Projection: AVG(#test.a) AS AggregateFunction-AVGfalseColumn-test.a, #test.a, #test.b, #test.c\
\n TableScan: test projection=None";

Expand All @@ -757,8 +757,8 @@ mod test {
])?
.build()?;

let expected = "Projection: #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) Plus test.a AS first, #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) Plus test.a AS second\
\n Projection: Int32(1) Plus #test.a AS BinaryExpr-+Column-test.aLiteral1, #test.a, #test.b, #test.c\
let expected = "Projection: #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) + test.a AS first, #BinaryExpr-+Column-test.aLiteral1 AS Int32(1) + test.a AS second\
\n Projection: Int32(1) + #test.a AS BinaryExpr-+Column-test.aLiteral1, #test.a, #test.b, #test.c\
\n TableScan: test projection=None";

assert_optimized_plan_eq(&plan, expected);
Expand All @@ -777,7 +777,7 @@ mod test {
])?
.build()?;

let expected = "Projection: Int32(1) Plus #test.a, #test.a Plus Int32(1)\
let expected = "Projection: Int32(1) + #test.a, #test.a + Int32(1)\
\n TableScan: test projection=None";

assert_optimized_plan_eq(&plan, expected);
Expand All @@ -794,8 +794,8 @@ mod test {
.project(vec![binary_expr(lit(1), Operator::Plus, col("a"))])?
.build()?;

let expected = "Projection: #Int32(1) Plus test.a\
\n Projection: Int32(1) Plus #test.a\
let expected = "Projection: #Int32(1) + test.a\
\n Projection: Int32(1) + #test.a\
\n TableScan: test projection=None";

assert_optimized_plan_eq(&plan, expected);
Expand Down
4 changes: 2 additions & 2 deletions datafusion/src/optimizer/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ mod tests {

let expected = "\
Projection: #test.a\
\n Filter: NOT #test.b And #test.c\
\n Filter: NOT #test.b AND #test.c\
\n TableScan: test projection=None";

assert_optimized_plan_eq(&plan, expected);
Expand All @@ -609,7 +609,7 @@ mod tests {

let expected = "\
Projection: #test.a\
\n Filter: NOT #test.b Or NOT #test.c\
\n Filter: NOT #test.b OR NOT #test.c\
\n TableScan: test projection=None";

assert_optimized_plan_eq(&plan, expected);
Expand Down
Loading