alamb commented on code in PR #25815: URL: https://github.com/apache/datafusion/pull/25815#discussion_r4122641983
########## datafusion/sqllogictest/test_files/date_bin_errors.slt: ########## @@ -93,25 +94,27 @@ select date_bin( ---- NULL -# Source timestamp scaling to nanoseconds overflows: should return NULL, not panic -query P -select date_bin( +# Source timestamp scaling to nanoseconds overflows, so the bin is computed in +# i128 instead (previously NULL, and before that a panic). The result cannot be Review Comment: I don't think the history of what this used to produce is particulary helpful here -- we could just slim this comment down to note it causes nanoseconds to overflow ########## datafusion/sqllogictest/test_files/date_bin_errors.slt: ########## @@ -93,25 +94,27 @@ select date_bin( ---- NULL -# Source timestamp scaling to nanoseconds overflows: should return NULL, not panic -query P -select date_bin( +# Source timestamp scaling to nanoseconds overflows, so the bin is computed in +# i128 instead (previously NULL, and before that a panic). The result cannot be +# displayed as a date, so show its raw value. +query I +select arrow_cast(date_bin( interval '1 nanosecond', arrow_cast(9223372036854775807, 'Timestamp(Second, None)'), timestamp '1970-01-01 00:00:00' -); +), 'Int64'); ---- -NULL +9223372036854775807 Review Comment: why do we need the extra cast to `int64` here? ########## datafusion/sqllogictest/test_files/datetime/timestamps.slt: ########## @@ -1792,9 +1817,112 @@ FROM ( ) ORDER BY b ASC NULLS LAST ---- +1653-02-10T06:13:20 1970-01-01T00:00:00 -NULL -NULL +2286-11-20T17:46:40 + +# A negative month stride can move a bin past its source, so date_bin keeps Review Comment: 🤔 but wouldn't all dates get shifted backwards (and thus still be sorted)? This may be related to what @mhilton is proposing for semantics of `date_bin` with timezones here: https://github.com/apache/datafusion/issues/10602#issuecomment-5845897142 ########## datafusion/sqllogictest/test_files/date_bin_errors.slt: ########## @@ -73,15 +73,16 @@ select date_bin( ---- NULL -# Extreme timestamp overflow: source - origin overflows i64 (should return NULL, not panic) +# Extreme timestamp overflow: source - origin overflows i64, so the bin is +# computed in i128 instead (previously NULL, and before that a panic) Review Comment: again here the history of what this used to do is probably not helpful in the comments ########## datafusion/sqllogictest/test_files/datetime/timestamps.slt: ########## @@ -1728,7 +1728,7 @@ physical_plan 02)--SortExec: TopK(fetch=2), expr=[column1@0 ASC NULLS LAST], preserve_partitioning=[false] 03)----DataSourceExec: partitions=1, partition_sizes=[1] -# date_bin remains conservatively unordered because per-row failures become NULL +# Typed bounds allow date_trunc to propagate ordering through timezone-free date_bin Review Comment: this plan still has a `SortExec` in it -- if the ordering propagated correctly I think the SortExec should be removed, right? Or is the idea we have only a single SortExec in the plan Perhaps we could (as a different PR) we could use the `CREATE EXTERNAL TABLE ... ORDERED BY ` syntax so there aren't any sorts in the plan when the sorts are passed on ########## datafusion/functions/src/datetime/date_bin.rs: ########## @@ -273,16 +273,24 @@ impl ScalarUDFImpl for DateBinFunc { let date_value = &input[1]; let reference = input.get(2); - // Scaling these representations to nanoseconds can overflow and turn - // otherwise valid input rows into NULL. Unknown ranges use the Null - // type and can hide one of these representations. The generated NULLs - // need not have the same placement as the source ordering. - let scale_can_overflow = matches!( - date_value.range.data_type(), - Null | Timestamp(Second | Millisecond | Microsecond, _) | Time64(Microsecond) - ); + // DATE_BIN preserves the order of its second argument. Values whose + // nanosecond form overflows i64 are binned in i128, so a non-null + // input only becomes NULL when its bin cannot be represented: a bin + // starting before the minimum value of the type, or a month bin + // outside the range of `DateTime<Utc>`. These extremes are accepted + // rather than giving up the ordering for all inputs. + // + // A negative month stride can move a bin past its source (see + // `bin_months`), so its output is not monotonic, even for ordinary + // dates. Only propagate the ordering for strides known not to be one. Review Comment: A lot of this comment is describing implementation details that I think are confusing. We could probably cut it down to something much more concise like ```suggestion // DATE_BIN preserves the order of its second argument. // // A negative month stride can move a bin past its source (see // `bin_months`), so its output is not monotonic, even for ordinary // dates. ``` ########## datafusion/functions/src/datetime/date_bin.rs: ########## @@ -273,17 +273,13 @@ impl ScalarUDFImpl for DateBinFunc { let date_value = &input[1]; let reference = input.get(2); - // Scaling these representations to nanoseconds can overflow and turn - // otherwise valid input rows into NULL. Unknown ranges use the Null - // type and can hide one of these representations. The generated NULLs - // need not have the same placement as the source ordering. - let scale_can_overflow = matches!( - date_value.range.data_type(), - Null | Timestamp(Second | Millisecond | Microsecond, _) | Time64(Microsecond) - ); - - if !scale_can_overflow - && step.sort_properties == SortProperties::Singleton + // DATE_BIN preserves the order of its second argument. Values whose + // nanosecond form overflows i64 are binned in i128, so a non-null + // input only becomes NULL when its bin cannot be represented: a bin + // starting before the minimum value of the type, or a month bin + // outside the range of `DateTime<Utc>`. These extremes are accepted + // rather than giving up the ordering for all inputs. + if step.sort_properties == SortProperties::Singleton Review Comment: > Could we reject unsupported negative month strides, or avoid propagating ordering for them until their semantics are made monotonic? This would preserve the optimization for ordinary positive strides. - see https://github.com/apache/datafusion/issues/10602#issuecomment-5845897142 from @mhilton for a discussion on date_bin semantics when timezones are involved -- 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]
