Skip to content
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.

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

Minor: fix tz in NowFunc #10960

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

marvinlanhenke
Copy link
Contributor

Which issue does this PR close?

Closes #10957.

Rationale for this change

Timezone was specified as "+00:00" instead of "UTC". This caused a missmatch in temporal_coercion.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core datafusion crate sqllogictest labels Jun 17, 2024
@marvinlanhenke
Copy link
Contributor Author

marvinlanhenke commented Jun 18, 2024

@alamb PTAL.

Since "+00:00" and "UTC" are the 'same' - is this viable? Or do we have to extend the logic here in order to provide both variants (and possibly not introduce a breaking change)?

@alamb
Copy link
Contributor

alamb commented Jun 18, 2024

I responded on #10957 (comment)

@erratic-pattern
Copy link
Contributor

erratic-pattern commented Jun 18, 2024

Since "+00:00" and "UTC" are the 'same' - is this viable? Or do we have to extend the logic here in order to provide both variants (and possibly not introduce a breaking change)?

+1 on including type coercion logic for "UTC" and "+00:00" and leaving the signature of NowFunc unchanged. This would fix the error described in #10957 more generally without introducing any breaking changes

@github-actions github-actions bot added logical-expr Logical plan and expressions and removed core Core datafusion crate sqllogictest labels Jun 18, 2024
@marvinlanhenke
Copy link
Contributor Author

...perhaps someone can help me out on this. I still haven't found a way to create a timestamp with timezone in order to create a proper sqllogictest.

@alamb
Copy link
Contributor

alamb commented Jun 18, 2024

...perhaps someone can help me out on this. I still haven't found a way to create a timestamp with timezone in order to create a proper sqllogictest.

There are some examples here: #10602

You can also use arrow_cast (there are some examples here https://datafusion.apache.org/user-guide/sql/scalar_functions.html#arrow-cast)

@marvinlanhenke
Copy link
Contributor Author

I think this should be fixed now. I added some sqllogictest as well to validate the new behavior - thanks @alamb for the pointer on arrow_cast. PTAL @alamb @erratic-pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions sqllogictest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timezone UTC is not considered equal to +00:00
3 participants