sunchao commented on code in PR #24668:
URL: https://github.com/apache/datafusion/pull/24668#discussion_r3855118651
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1411,11 +1339,6 @@ impl TreeNodeRewriter for Simplifier<'_> {
//
Expr::Not(inner) => Transformed::yes(negate_clause(*inner)),
- //
- // Rules for Negative
- //
- Expr::Negative(inner) =>
Transformed::yes(distribute_negation(*inner)),
Review Comment:
**[P2] Preserve timestamp-column support when retaining double negation**
Removing this arm also stops cancelling `-(-timestamp_column)`. I reproduced
`SELECT -(-ts) AS x FROM t` using a registered `TimestampNanosecondArray`
containing `2020-01-01` and `2020-01-02`: base `63f5b55f` returns both
timestamps, while head `c37e6666` fails with `Invalid arithmetic operation:
!Timestamp(ns)`. SQL analysis and physical planning explicitly accept timestamp
negation, but `NegativeExpr::evaluate` passes arrays to Arrow's `neg_wrapping`,
whose fallback does not support timestamp arrays. Timestamp literals still work
through the separate scalar implementation.
The single-negation kernel gap already existed, but retaining both nodes
newly breaks these previously working double-negation queries. Could we add
timestamp-array negation support and a column-based execution regression test
alongside this change, while preserving the intended overflow behavior?
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -3115,47 +3038,24 @@ mod tests {
}
#[test]
- fn test_simplify_negated_bitwise_and() {
- // !c3 & c3 --> 0
- let expr = (-col("c3_non_null")) & col("c3_non_null");
- let expected = lit(0i64);
-
- assert_eq!(simplify(expr), expected);
- // c3 & !c3 --> 0
- let expr = col("c3_non_null") & (-col("c3_non_null"));
- let expected = lit(0i64);
-
- assert_eq!(simplify(expr), expected);
- }
-
- #[test]
- fn test_simplify_negated_bitwise_or() {
- // !c3 | c3 --> -1
- let expr = (-col("c3_non_null")) | col("c3_non_null");
- let expected = lit(-1i64);
-
- assert_eq!(simplify(expr), expected);
-
- // c3 | !c3 --> -1
- let expr = col("c3_non_null") | (-col("c3_non_null"));
- let expected = lit(-1i64);
-
- assert_eq!(simplify(expr), expected);
- }
-
- #[test]
- fn test_simplify_negated_bitwise_xor() {
- // !c3 ^ c3 --> -1
- let expr = (-col("c3_non_null")) ^ col("c3_non_null");
- let expected = lit(-1i64);
-
- assert_eq!(simplify(expr), expected);
-
- // c3 ^ !c3 --> -1
- let expr = col("c3_non_null") ^ (-col("c3_non_null"));
- let expected = lit(-1i64);
+ fn test_preserve_arithmetic_negation() {
+ let c3 = col("c3_non_null");
+ let expressions = [
+ (-c3.clone()) & c3.clone(),
+ c3.clone() & (-c3.clone()),
+ (-c3.clone()) | c3.clone(),
+ c3.clone() | (-c3.clone()),
+ (-c3.clone()) ^ c3.clone(),
+ c3.clone() ^ (-c3.clone()),
+ -bitwise_and(col("c3"), c3.clone()),
+ -bitwise_or(col("c3"), c3.clone()),
+ // The inner negation can overflow for the signed minimum.
+ -(-c3),
Review Comment:
**[P2] Parenthesize retained nested negations in SQL unparsing**
This preserved expression shape breaks SQL generated from optimized plans. I
reproduced this by optimizing `SELECT -(-i) AS x FROM t` over a registered
Int64 column, then calling
`Unparser::default().with_pretty(pretty).plan_to_sql(&plan)?.to_string()`. Base
`63f5b55f` emits `SELECT t.i AS x FROM t`; head `c37e6666` emits `SELECT --t.i
AS x FROM t`. The adjacent minus signs start a SQL comment, so reparsing fails
with `Expected: an expression, found: EOF` in both pretty modes.
`datafusion/sql/src/unparser/expr.rs` recursively emits bare unary-minus
nodes without nesting parentheses. Direct unparsing of an unoptimized double
negative already had this gap, but removing cancellation now exposes it in
previously working optimized-plan workflows. Could we parenthesize nested unary
minus in the unparser and add an optimized-plan SQL roundtrip regression test?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
