sunchao commented on code in PR #25692:
URL: https://github.com/apache/datafusion/pull/25692#discussion_r4112775686
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -269,14 +269,22 @@ impl ScalarUDFImpl for DateBinFunc {
}
fn output_ordering(&self, input: &[ExprProperties]) ->
Result<SortProperties> {
- // The DATE_BIN function preserves the order of its second argument.
let step = &input[0];
let date_value = &input[1];
let reference = input.get(2);
- if step.sort_properties.eq(&SortProperties::Singleton)
+ // Scaling these representations to nanoseconds can overflow and turn
+ // otherwise valid input rows into NULL. The generated NULLs need not
+ // have the same placement as the source ordering.
+ let scale_can_overflow = matches!(
+ date_value.range.data_type(),
+ Timestamp(Second | Millisecond | Microsecond, _) |
Time64(Microsecond)
Review Comment:
[P2] Handle unknown range types before preserving ordering
This guard still misses the targeted scaling overflow when the timestamp
comes from another expression. `date_trunc` preserves ordering but inherits the
default `evaluate_bounds`, which returns an unbounded `DataType::Null`
interval. `ScalarFunctionExpr::get_properties` retains that unknown type, so
`scale_can_overflow` is false even when the actual source is
`Timestamp(Second)`.
On head `13dd764cd2`, this query removes the final sort and returns `[NULL,
1970-01-01T00:00:00, NULL]`, despite `ASC NULLS LAST`:
```sql
SELECT date_bin(INTERVAL '1 second', date_trunc('second', ts)) AS b
FROM (
SELECT arrow_cast(column1, 'Timestamp(Second)') AS ts
FROM (VALUES
(-10000000000::bigint), (0::bigint), (10000000000::bigint)
)
ORDER BY ts LIMIT 3
)
ORDER BY b ASC NULLS LAST;
```
The expected result is `[1970-01-01T00:00:00, NULL, NULL]`. I reproduced the
same gap for millisecond and microsecond inputs. These nested cases also fail
on the merge base, so this is an uncovered case of the original bug rather than
a newly introduced regression. Temporarily treating `DataType::Null` as
potentially overflowing restores the outer sort and correct results for all
three.
Could we conservatively reject unknown range types (or positively match the
representations known not to overflow during scaling) and add a
nested-expression regression alongside the direct-column 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]
