Skip to content

Add timezone to date_trunc fast path #18596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 11, 2025
Merged

Add timezone to date_trunc fast path #18596

merged 4 commits into from
Nov 11, 2025
+210 −22

Conversation

@hareshkh
Copy link
Contributor

@hareshkh hareshkh commented Nov 10, 2025 •
edited
Loading

Which issue does this PR close?

Rationale for this change

A check is recently added to invoke_with_args that checks for the output type of the result with the expected output type from the UDF - #17515. Because the fast path misses adding the timezone, the assertion added in this PR fails.

What changes are included in this PR?

Include timezone information in the fast path.

Are these changes tested?

Yes, added a unit test

Are there any user-facing changes?

No

@github-actions github-actions bot added the functions Changes to functions implementation label Nov 10, 2025
comphead
comphead reviewed Nov 10, 2025
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hareshkh I think this is LGTM, just wondering can this be also tested in .slt tests?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 11, 2025
@hareshkh
Copy link
Contributor Author

hareshkh commented Nov 11, 2025

@comphead

Verified that without this fix slt has the following error:

query error
SELECT
    date_trunc('millisecond', ts)
FROM ts_data_micros_kolkata
----
DataFusion error: Internal error: Function 'date_trunc' returned value of type 'Timestamp(Nanosecond, None)' while the following type was promised at planning time and expected: 'Timestamp(Nanosecond, Some("Asia/Kolkata"))'.
This issue was likely caused by a bug in DataFusion's code. Please help us to resolve this by filing a bug report in our issue tracker: https://github.com/apache/datafusion/issues
image

But after the fix:

query P
SELECT
    date_trunc('millisecond', ts)
FROM ts_data_micros_kolkata
----
2020-09-08T19:12:29.190+05:30
2020-09-08T18:12:29.190+05:30
2020-09-08T17:12:29.190+05:30

Pushed the SLT test now.

@hareshkh
Copy link
Contributor Author

hareshkh commented Nov 11, 2025

@comphead @alamb : If this looks good, can I cherry-pick this change onto branch-51 as well because the datatype assert will be released with DataFusion 51.

comphead
comphead approved these changes Nov 11, 2025
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hareshkh
For 51 release we already have a branch created branch-51 please make a cherry pick to this branch if you wanna have this hotfix.

Release 51 tracking is #17558

@comphead comphead added this pull request to the merge queue Nov 11, 2025
Merged via the queue into apache:main with commit cbf3f50 Nov 11, 2025
28 checks passed
@comphead comphead mentioned this pull request Nov 11, 2025
Open
37 tasks
hareshkh added a commit to hareshkh/datafusion that referenced this pull request Nov 11, 2025
@hareshkh
Add timezone to date_trunc fast path (apache#18596)
8e85e9d 
## 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 apache#123` indicates that this PR will close issue apache#123.
-->

- Closes apache#18597

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

A check is recently added to `invoke_with_args` that checks for the
output type of the result with the expected output type from the UDF -
apache#17515. Because the fast path
misses adding the timezone, the assertion added in this PR fails.

## What changes are included in this PR?

<!--
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.
-->

Include timezone information in the fast path.

## Are these changes tested?

<!--
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)?
-->

Yes, added a unit test

## Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
No

<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
@hareshkh
Copy link
Contributor Author

hareshkh commented Nov 11, 2025 •
edited
Loading

@comphead Thankyou for the review! <3
Here is the cherry-pick PR - #18629

@hareshkh hareshkh deleted the hareshkh/cast-date-trunc-fast-path branch November 11, 2025 18:30
xudong963 pushed a commit that referenced this pull request Nov 12, 2025
@hareshkh
[branch-51]: Add timezone to date_trunc fast path (#18596) (#18629)
6dcde50 
## 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. -->

- Closes #18597

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

A check is recently added to `invoke_with_args` that checks for the
output type of the result with the expected output type from the UDF -
#17515. Because the fast path
misses adding the timezone, the assertion added in this PR fails.

## What changes are included in this PR?

<!--
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.
-->

Include timezone information in the fast path.

## Are these changes tested?

<!--
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)?
-->

Yes, added a unit test and SLT test

## Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
No

<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->

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

- Closes #.

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

## What changes are included in this PR?

<!--
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?

<!--
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.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comphead comphead comphead approved these changes

Assignees

No one assigned

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

date_trunc output from fast path does not contain timezone information

2 participants

@hareshkh @comphead