Re: [PR] speedup `date_trunc` (~7x faster) in some cases [datafusion]

[via GitHub]

findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229547803


##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -185,6 +187,21 @@ impl ScalarUDFImpl for DateTruncFunc {
         ) -> Result<ColumnarValue> {
             let parsed_tz = parse_tz(tz_opt)?;
             let array = as_primitive_array::<T>(array)?;
+
+            // fast path for fine granularities
+            if matches!(
+                granularity.as_str(),
+                "second" | "minute" | "millisecond" | "microsecond"
+            ) || (parsed_tz.is_none() && granularity.as_str() == "hour")
+            {
+                let result = general_date_trunc_array_fine_granularity(

Review Comment:
   > * Not sure whether chrono respects that
   
   it looks it does
   
   ```rust
   let timestamp = 
chrono::DateTime::parse_from_rfc3339("1910-01-01T00:00:00Z").unwrap();
   dbg!(&timestamp);
   let tz = chrono_tz::Asia::Kathmandu;
   // let tz = arrow::array::timezone::Tz::from_str("Asia/Kathmandu").unwrap();
   dbg!(&tz);
   let zoned_date_time = timestamp.with_timezone(&tz);
   dbg!(&zoned_date_time);
   let local_date_time = zoned_date_time.naive_local();
   dbg!(&local_date_time);
   let truncated_local = local_date_time.with_second(0).unwrap();
   dbg!(&truncated_local);
   let mapped_back = match truncated_local.and_local_timezone(tz) {
       MappedLocalTime::Single(mapped) => mapped,
       _ => panic!(),
   };
   dbg!(&mapped_back);
   let timestamp = mapped_back.to_utc();
   dbg!(&timestamp);
   dbg!(timestamp.timestamp());
   ```
   
   ```
   [tests/chrono.rs:7:1] &timestamp = 1910-01-01T00:00:00+00:00
   [tests/chrono.rs:10:1] &tz = Asia/Kathmandu
   [tests/chrono.rs:12:1] &zoned_date_time = 1910-01-01T05:41:16LMT
   [tests/chrono.rs:14:1] &local_date_time = 1910-01-01T05:41:16
   [tests/chrono.rs:16:1] &truncated_local = 1910-01-01T05:41:00
   [tests/chrono.rs:21:1] &mapped_back = 1910-01-01T05:41:00LMT
   [tests/chrono.rs:23:1] &timestamp = 1909-12-31T23:59:44Z
   [tests/chrono.rs:24:1] timestamp.timestamp() = -1893456016
   ```
   
   
   however either us, or arrow, makes assumption that offsets are whole 
minutes. this is visible in the output above 
(https://github.com/apache/datafusion/pull/16859/files#r2229281161).  maybe 
that's rendering in the CLI output rendering layer. it makes reasoning about 
this harder.
   
   ```
   SELECT cast(date_trunc('minute', arrow_cast(v, 'Timestamp(Second, 
Some("Asia/Kathmandu"))')) as bigint) FROM (VALUES ('1910-01-01T00:00:00Z')) 
t(v);
   ```
   
   - main: -1893456000_000000000
   - PR: -1893456000_000000000
   
   while per-chrono above, the correct result seems to be -1893456016_000000000
   
   
   



-- 
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: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org