sunchao commented on code in PR #24668:
URL: https://github.com/apache/datafusion/pull/24668#discussion_r3859826646
##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -78,6 +84,46 @@ impl std::fmt::Display for NegativeExpr {
}
}
+fn negate_timestamp_array<T: ArrowTimestampType>(array: &dyn Array) ->
Result<ArrayRef> {
+ let array = array.as_primitive::<T>();
+ let timezone = array.timezone().map(Arc::<str>::from);
+ let result = array.try_unary::<_, T, _>(|value| value.neg_checked())?;
+ Ok(Arc::new(result.with_timezone_opt(timezone)))
+}
+
+fn negate_scalar(scalar: ScalarValue) -> Result<ScalarValue> {
+ Ok(match scalar {
+ ScalarValue::Int8(value) =>
ScalarValue::Int8(value.map(i8::wrapping_neg)),
+ ScalarValue::Int16(value) =>
ScalarValue::Int16(value.map(i16::wrapping_neg)),
+ ScalarValue::Int32(value) =>
ScalarValue::Int32(value.map(i32::wrapping_neg)),
+ ScalarValue::Int64(value) =>
ScalarValue::Int64(value.map(i64::wrapping_neg)),
Review Comment:
**[P1] Make inequality pruning safe before wrapping scalar negation**
Pruning still rewrites `-i > MIN` into `i < -MIN`. These new wrapping cases
make `-MIN` equal `MIN`, so the resulting `i_min < MIN` predicate incorrectly
discards matching data. With a Parquet `TINYINT NOT NULL` column containing
`[1, 2]`, I reproduced:
```sql
SELECT i FROM t
WHERE -i > CAST(-128 AS TINYINT)
ORDER BY i;
```
Base `63f5b55f` returns both rows; head `00b519c3a` returns none. Execution
metrics confirm file-statistics pruning discards the file before reading rows.
At base, checked overflow causes pruning to fall back conservatively. I
reproduced the same regression for all four signed integer widths.
Could we make the pruning rewrite conservative for wrapping inequalities and
add an execution regression with pruning enabled, while keeping the intended
scalar/array consistency?
##########
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 integer double-negation ordering equivalence**
Removing this arm preserves `-(-i)`, whose properties no longer establish
the full ordering of an input sorted by `(i, j)`. On an unbounded stream with
constant `i = 0` and nondecreasing `j`, I reproduced:
```sql
SELECT i, j FROM fixed_prefix_stream
ORDER BY -(-i) ASC NULLS LAST, j ASC NULLS LAST
LIMIT 1;
```
Base `63f5b55f` uses `StreamingTableExec` with `fetch=1` and returns `(0,
0)` immediately. Head `00b519c3a` inserts `PartialSortExec: TopK(fetch=1),
common_prefix_length=[1]` and times out. The partial sort waits for `i` to
change, so a permanently fixed prefix never emits its first row. Both direct
`ORDER BY i, j LIMIT 1` and one-key `ORDER BY -(-i) LIMIT 1` still return
immediately on head.
Could we restore cancellation specifically for signed integers, where
wrapping makes it safe even at MIN, or preserve equivalent ordering metadata?
Extending the new streaming regression to two sort keys would cover this
without changing checked timestamp/interval behavior.
--
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]
