sunchao commented on code in PR #24668:
URL: https://github.com/apache/datafusion/pull/24668#discussion_r3857150340
##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -128,7 +178,9 @@ impl PhysicalExpr for NegativeExpr {
interval: &Interval,
children: &[&Interval],
) -> Result<Option<Vec<Interval>>> {
- let negated_interval = interval.arithmetic_negate()?;
+ let Some(negated_interval) = negate_interval(interval)? else {
Review Comment:
**[P1] Preserve wrapped minima during inverse constraint propagation**
The new forward-overflow fallback permits an unsafe inverse result here. For
input statistics `i: [-128, 1]`, the predicate `-i < 0` gives the negation node
`[NULL, -1]` (unbounded below). Negating those endpoints succeeds as `[1,
NULL]`, so intersecting with the child incorrectly infers `i = 1`. But array
negation wraps `-128` back to `-128`, which also satisfies the predicate.
`FilterExec` then publishes exact singleton statistics and removes the required
sort.
I reproduced this on a single-partition Int8 Parquet scan containing rows
`[1, -128]`, with min/max statistics and Parquet pruning/filter pushdown
disabled:
```sql
SELECT i FROM t
WHERE -i < CAST(0 AS TINYINT)
ORDER BY i LIMIT 1;
```
Head `4e9d57f3` returns `1`, not `-128`. Base `63f5b55f` raised an overflow
error during forward analysis; the new fallback exposes this silent
wrong-result path. Could `negate_interval` also fall back for signed intervals
whose lower endpoint is unbounded, and add a statistics-backed execution
regression? I verified that this guard restores `SortExec` and the correct
result in an isolated experiment.
##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -160,13 +212,31 @@ impl PhysicalExpr for NegativeExpr {
}
}
- /// The ordering of a [`NegativeExpr`] is simply the reverse of its child.
+ /// Negation reverses the child ordering unless signed integer values may
wrap.
fn get_properties(&self, children: &[ExprProperties]) ->
Result<ExprProperties> {
+ let (range, overflowed) = match negate_interval(&children[0].range)? {
+ Some(range) => (range, false),
+ None => (
+ Interval::make_unbounded(&children[0].range.data_type())?,
+ true,
+ ),
+ };
+ let may_wrap = children[0].range.data_type().is_signed_integer()
+ && (children[0].range.lower().is_null() || overflowed);
Review Comment:
**[P2] Retain ordering for safe integer widening on unbounded inputs**
This guard also drops ordering when the signed minimum is provably
unreachable. For an unbounded CSV source declared with `i INTEGER NOT NULL` and
`WITH ORDER (i ASC NULLS LAST)`, I tested:
```sql
SELECT i FROM neg_order_stream
ORDER BY -CAST(i AS BIGINT) DESC NULLS LAST;
```
Base `63f5b55f` builds a `StreamingTableExec` without a sort. Head
`4e9d57f3` inserts a global `SortExec` and fails `SanityCheckPlan` with `Cannot
execute pipeline breaking queries`. `Int32 -> Int64` is an exact widening
conversion, so its result can never be `Int64::MIN` and negation safely
reverses the declared ordering. However, `cast_expr_properties` currently
replaces the source range with unbounded Int64, causing this condition to
classify it as potentially wrapping.
Could we retain the representable source-domain bounds for strict integer
widening casts so this guard can preserve safe ordering, with an
unbounded-source planning regression test? The same new rejection also affects
`ORDER BY -(-i)`, although wrapping integer-array double negation preserves
every input value.
--
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]
