You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "waynexia (via GitHub)" <gi...@apache.org> on 2025/07/22 23:02:22 UTC

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

waynexia opened a new pull request, #16859:
URL: https://github.com/apache/datafusion/pull/16859

   ## Which issue does this PR close?
   
   <!--
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
   -->
   
   - follow-up of #14593
   
   ## Rationale for this change
   
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   Follows the comment https://github.com/apache/datafusion/pull/14593#discussion_r1951334920 to implement an array version of `date_trunc`.
   
   While implementing, I found that there is a lot of code handling boring calendar things, which is totally unrelated to some small granularities (less or equal to "hour"). So I also cut them off. Benchmark shows this simplification can bring 4x performance.
   
   ## What changes are included in this PR?
   
   a faster implementation for `date_trunc` with granularities from "microsecond" to "hour"
   
   ```
   date_trunc_minute_1000  time:   [16.600 µs 16.643 µs 16.684 µs]
   Found 8 outliers among 100 measurements (8.00%)
     8 (8.00%) low mild
   
   date_trunc_minute_1000  time:   [2.3474 µs 2.3519 µs 2.3579 µs]
                           change: [-85.946% -85.909% -85.870%] (p = 0.00 < 0.05)
                           Performance has improved.
   Found 1 outliers among 100 measurements (1.00%)
     1 (1.00%) high severe
   ```
   
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   ## Are these changes tested?
   
   using existing unit tests
   
   <!--
   We typically require tests for all PRs in order to:
   1. Prevent the code from being accidentally broken by subsequent changes
   2. Serve as another way to document the expected behavior of the code
   
   If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
   -->
   
   ## Are there any user-facing changes?
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please add the `api change` label.
   -->
   


-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229625741


##########
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")

Review Comment:
   make sense 👍 346ae5952



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229264131


##########
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:
   You're right. For minute and smaller granularities the new code path is always fine¹
   For hour it's also fine, for zone-less data.
   
   - (¹) in the past (early XX century and earlier) named zones often had offset changes not multiple of minutes at list this is what https://www.timeanddate.com/time/zone/nepal/kathmandu shows me. Not sure whether chrono respects that
   <img width="831" height="259" alt="image" src="https://github.com/user-attachments/assets/eea59fb7-02e2-4613-9be1-e248865b2a92" />
   
   - for zone-less data (timestamp without time zone), day granularity can also be applied with just divide and mul, just like hour. Or am i blind again?



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229609157


##########
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:
   Thank you for your very detailed check and explanation. I find another example with an extra search (that website is great)...
   
   <img width="681" height="306" alt="image" src="https://github.com/user-attachments/assets/3816bbc1-6d50-492d-9a07-23663826d700" />
   



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229628338


##########
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"

Review Comment:
   Add some comments in 346ae5952 



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#issuecomment-3114284084

   🤖: Benchmark completed
   
   <details><summary>Details</summary>
   <p>
   
   
   ```
   group                     date-trunc-fast                        main
   -----                     ---------------                        ----
   date_trunc_minute_1000    1.00      4.7±0.01µs        ? ?/sec    6.17     28.7±0.03µs        ? ?/sec
   ```
   
   
   </p>
   </details>
   
   


-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229550787


##########
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"

Review Comment:
   `minutes` is correct for modern zones, but not historical dates.
   The old code apparently has some issues with them too though (https://github.com/apache/datafusion/pull/16859/files#r2229547803)
   Let's add _a code comment_ about our conscious ignorance of historical zone offsets here.



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia merged PR #16859:
URL: https://github.com/apache/datafusion/pull/16859


-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229497695


##########
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:
   It doesn't look right to me either. First, it's not truncating to the minute, and second I have my doubts that TZ is correct with that time. I'd have to think about that a bit more but I think a ticket should be filed to have that looked at some more



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229234066


##########
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:
   > The `parsed_tz.is_none()` check
   
   ~it's there for `hour`, but not for `minute`.~
   ~Why have different zone-related conditions for `hour` and `minute`?~



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#issuecomment-3129966163

   Appreciate your detailed review! Learned a lot about timezone 🫨


-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#issuecomment-3114272105

   🤖 `./gh_compare_branch_bench.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_branch_bench.sh) Running
   Linux aal-dev 6.11.0-1016-gcp #16~24.04.1-Ubuntu SMP Wed May 28 02:40:52 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
   Comparing date-trunc-fast (f50fb3fed397815287669421c450e42007797bac) to a0ce581092430271e67552ec6370f100cbb80046 [diff](https://github.com/apache/datafusion/compare/a0ce581092430271e67552ec6370f100cbb80046..f50fb3fed397815287669421c450e42007797bac)
   BENCH_NAME=date_trunc
   BENCH_COMMAND=cargo bench --bench date_trunc
   BENCH_FILTER=
   BENCH_BRANCH_NAME=date-trunc-fast
   Results will be posted here when complete
   


-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2256602117


##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -185,6 +187,26 @@ 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(),
+                // For morden timezones, it's correct to truncate "minute" in this way.

Review Comment:
   morden -> modern?



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2225382275


##########
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:
   Does this assume zone offsets are multiples of a whole hour?
   
   (for example, Asia/Kathmandu is +05:45)



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2226279914


##########
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:
   The `parsed_tz.is_none()` check ensures this array is not associated with any timezone (hence the default UTC) and is safe to do in this way.
   
   Added a test case f50fb3fed for this case (TIL



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229256985


##########
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:
   Because (AFAIK) no timezone has a different, non-integer offset on minute like `+04:32:10`, so we can truncate all timestamps at minute level regardless of their timezone



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229557133


##########
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")

Review Comment:
   let's unlock the optimization for "day" in zoneless case.



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2268439364


##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -185,6 +187,26 @@ 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(),
+                // For morden timezones, it's correct to truncate "minute" in this way.

Review Comment:
   fix them all 😎 https://github.com/apache/datafusion/pull/17135 



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229234066


##########
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:
   > The `parsed_tz.is_none()` check
   
   it's there for `hour`, but not for `minute`.
   Why have different zone-related conditions for `hour` and `minute`?



-- 
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-unsubscribe@datafusion.apache.org

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


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

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

Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16859:
URL: https://github.com/apache/datafusion/pull/16859#discussion_r2229281161


##########
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:
   ```sql
   SELECT date_trunc('minute', arrow_cast('1910-01-01T00:00:00Z', 'Timestamp(Second, Some("Asia/Kathmandu"))'));
   SELECT date_trunc('minute', arrow_cast(v, 'Timestamp(Second, Some("Asia/Kathmandu"))')) FROM (VALUES ('1910-01-01T00:00:00Z')) t(v);
   ```
   
   ```
   +-----------------------------------------------------------------------------------------------------------------------+
   | date_trunc(Utf8("minute"),arrow_cast(Utf8("1910-01-01T00:00:00Z"),Utf8("Timestamp(Second, Some("Asia/Kathmandu"))"))) |
   +-----------------------------------------------------------------------------------------------------------------------+
   | 1910-01-01T05:41:16+05:41                                                                                             |
   +-----------------------------------------------------------------------------------------------------------------------+
   1 row(s) fetched.
   Elapsed 0.065 seconds.
   
   +----------------------------------------------------------------------------------------------+
   | date_trunc(Utf8("minute"),arrow_cast(t.v,Utf8("Timestamp(Second, Some("Asia/Kathmandu"))"))) |
   +----------------------------------------------------------------------------------------------+
   | 1910-01-01T05:41:16+05:41                                                                    |
   +----------------------------------------------------------------------------------------------+
   1 row(s) fetched.
   Elapsed 0.012 seconds.
   ```
   
   this doesn't look correct to me
   however, this is the same result as on `main`, so it's not a regression.
   Is it because the existing date_trunc code do similar logic as the new optimized code path?
   
   



-- 
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-unsubscribe@datafusion.apache.org

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


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