Skip to content

Commit

Permalink
fix: misc phys. expression display bugs (#5387)
Browse files Browse the repository at this point in the history 
* fix: `TRY_CAST` phys. expr display

* refactor: move precedence from `BinaryExpr` to `Operator`

This is useful in other contexts as well, e.g. for formatting phys.
expressions.

* fix: display of nested phys. binary expr should use parenthesis

Found while working on #4695.
  • Loading branch information
@crepererum
crepererum authored Feb 24, 2023
1 parent ac33ebc commit cef119d
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 39 deletions.
33 changes: 2 additions & 31 deletions datafusion/expr/src/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,35 +238,6 @@ impl BinaryExpr {
pub fn new(left: Box<Expr>, op: Operator, right: Box<Expr>) -> Self {
Self { left, op, right }
}

/// Get the operator precedence
/// use <https://www.postgresql.org/docs/7.0/operators.htm#AEN2026> as a reference
pub fn precedence(&self) -> u8 {
match self.op {
Operator::Or => 5,
Operator::And => 10,
Operator::NotEq
| Operator::Eq
| Operator::Lt
| Operator::LtEq
| Operator::Gt
| Operator::GtEq => 20,
Operator::Plus | Operator::Minus => 30,
Operator::Multiply | Operator::Divide | Operator::Modulo => 40,
Operator::IsDistinctFrom
| Operator::IsNotDistinctFrom
| Operator::RegexMatch
| Operator::RegexNotMatch
| Operator::RegexIMatch
| Operator::RegexNotIMatch
| Operator::BitwiseAnd
| Operator::BitwiseOr
| Operator::BitwiseShiftLeft
| Operator::BitwiseShiftRight
| Operator::BitwiseXor
| Operator::StringConcat => 0,
}
}
}

impl Display for BinaryExpr {
Expand All @@ -283,7 +254,7 @@ impl Display for BinaryExpr {
) -> fmt::Result {
match expr {
Expr::BinaryExpr(child) => {
let p = child.precedence();
let p = child.op.precedence();
if p == 0 || p < precedence {
write!(f, "({child})")?;
} else {
Expand All @@ -295,7 +266,7 @@ impl Display for BinaryExpr {
Ok(())
}

let precedence = self.precedence();
let precedence = self.op.precedence();
write_child(f, self.left.as_ref(), precedence)?;
write!(f, " {} ", self.op)?;
write_child(f, self.right.as_ref(), precedence)
Expand Down
29 changes: 29 additions & 0 deletions datafusion/expr/src/operator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,35 @@ impl Operator {
| Operator::StringConcat => None,
}
}

/// Get the operator precedence
/// use <https://www.postgresql.org/docs/7.0/operators.htm#AEN2026> as a reference
pub fn precedence(&self) -> u8 {
match self {
Operator::Or => 5,
Operator::And => 10,
Operator::NotEq
| Operator::Eq
| Operator::Lt
| Operator::LtEq
| Operator::Gt
| Operator::GtEq => 20,
Operator::Plus | Operator::Minus => 30,
Operator::Multiply | Operator::Divide | Operator::Modulo => 40,
Operator::IsDistinctFrom
| Operator::IsNotDistinctFrom
| Operator::RegexMatch
| Operator::RegexNotMatch
| Operator::RegexIMatch
| Operator::RegexNotIMatch
| Operator::BitwiseAnd
| Operator::BitwiseOr
| Operator::BitwiseShiftLeft
| Operator::BitwiseShiftRight
| Operator::BitwiseXor
| Operator::StringConcat => 0,
}
}
}

impl fmt::Display for Operator {
Expand Down
92 changes: 91 additions & 1 deletion datafusion/physical-expr/src/expressions/binary.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,34 @@ impl BinaryExpr {

impl std::fmt::Display for BinaryExpr {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "{} {} {}", self.left, self.op, self.right)
// Put parentheses around child binary expressions so that we can see the difference
// between `(a OR b) AND c` and `a OR (b AND c)`. We only insert parentheses when needed,
// based on operator precedence. For example, `(a AND b) OR c` and `a AND b OR c` are
// equivalent and the parentheses are not necessary.

fn write_child(
f: &mut std::fmt::Formatter,
expr: &dyn PhysicalExpr,
precedence: u8,
) -> std::fmt::Result {
if let Some(child) = expr.as_any().downcast_ref::<BinaryExpr>() {
let p = child.op.precedence();
if p == 0 || p < precedence {
write!(f, "({child})")?;
} else {
write!(f, "{child}")?;
}
} else {
write!(f, "{expr}")?;
}

Ok(())
}

let precedence = self.op.precedence();
write_child(f, self.left.as_ref(), precedence)?;
write!(f, " {} ", self.op)?;
write_child(f, self.right.as_ref(), precedence)
}
}

Expand Down Expand Up @@ -4249,4 +4276,67 @@ mod tests {

Ok(())
}

#[test]
fn test_display_and_or_combo() {
let expr = BinaryExpr::new(
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(1)),
Operator::And,
lit(ScalarValue::from(2)),
)),
Operator::And,
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(3)),
Operator::And,
lit(ScalarValue::from(4)),
)),
);
assert_eq!(expr.to_string(), "1 AND 2 AND 3 AND 4");

let expr = BinaryExpr::new(
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(1)),
Operator::Or,
lit(ScalarValue::from(2)),
)),
Operator::Or,
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(3)),
Operator::Or,
lit(ScalarValue::from(4)),
)),
);
assert_eq!(expr.to_string(), "1 OR 2 OR 3 OR 4");

let expr = BinaryExpr::new(
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(1)),
Operator::And,
lit(ScalarValue::from(2)),
)),
Operator::Or,
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(3)),
Operator::And,
lit(ScalarValue::from(4)),
)),
);
assert_eq!(expr.to_string(), "1 AND 2 OR 3 AND 4");

let expr = BinaryExpr::new(
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(1)),
Operator::Or,
lit(ScalarValue::from(2)),
)),
Operator::And,
Arc::new(BinaryExpr::new(
lit(ScalarValue::from(3)),
Operator::Or,
lit(ScalarValue::from(4)),
)),
);
assert_eq!(expr.to_string(), "(1 OR 2) AND (3 OR 4)");
}
}
14 changes: 7 additions & 7 deletions datafusion/physical-expr/src/expressions/try_cast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl TryCastExpr {

impl fmt::Display for TryCastExpr {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "CAST({} AS {:?})", self.expr, self.cast_type)
write!(f, "TRY_CAST({} AS {:?})", self.expr, self.cast_type)
}
}

Expand Down Expand Up @@ -132,7 +132,7 @@ pub fn try_cast(
Ok(Arc::new(TryCastExpr::new(expr, cast_type)))
} else {
Err(DataFusionError::NotImplemented(format!(
"Unsupported CAST from {expr_type:?} to {cast_type:?}"
"Unsupported TRY_CAST from {expr_type:?} to {cast_type:?}"
)))
}
}
Expand All @@ -155,7 +155,7 @@ mod tests {

// runs an end-to-end test of physical type cast
// 1. construct a record batch with a column "a" of type A
// 2. construct a physical expression of CAST(a AS B)
// 2. construct a physical expression of TRY_CAST(a AS B)
// 3. evaluate the expression
// 4. verify that the resulting expression is of type B
// 5. verify that the resulting values are downcastable and correct
Expand All @@ -171,7 +171,7 @@ mod tests {

// verify that its display is correct
assert_eq!(
format!("CAST(a@0 AS {:?})", $TYPE),
format!("TRY_CAST(a@0 AS {:?})", $TYPE),
format!("{}", expression)
);

Expand Down Expand Up @@ -202,7 +202,7 @@ mod tests {

// runs an end-to-end test of physical type cast
// 1. construct a record batch with a column "a" of type A
// 2. construct a physical expression of CAST(a AS B)
// 2. construct a physical expression of TRY_CAST(a AS B)
// 3. evaluate the expression
// 4. verify that the resulting expression is of type B
// 5. verify that the resulting values are downcastable and correct
Expand All @@ -218,7 +218,7 @@ mod tests {

// verify that its display is correct
assert_eq!(
format!("CAST(a@0 AS {:?})", $TYPE),
format!("TRY_CAST(a@0 AS {:?})", $TYPE),
format!("{}", expression)
);

Expand Down Expand Up @@ -542,7 +542,7 @@ mod tests {
let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);

let result = try_cast(col("a", &schema).unwrap(), &schema, DataType::LargeBinary);
result.expect_err("expected Invalid CAST");
result.expect_err("expected Invalid TRY_CAST");
}

// create decimal array with the specified precision and scale
Expand Down

0 comments on commit cef119d

Please sign in to comment.