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.

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

Consider timezones with UTC and +00:00 to be the same #10960

Merged
merged 8 commits into from
Jun 21, 2024
Merged

Consider timezones with UTC and +00:00 to be the same #10960

merged 8 commits into from
Jun 21, 2024

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 •
edited
Loading

@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 alamb mentioned this pull request Jun 18, 2024
Closed
@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 •
edited
Loading

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

alamb
alamb approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @marvinlanhenke and @samuelcolvin -- this looks good to me

cc @waitingkuo

@alamb
Copy link
Contributor

alamb commented Jun 20, 2024

I added some additional comments and merged up from main.

@erratic-pattern if you have time, it would be great if you could review this PR as well

@alamb alamb changed the title Minor: fix tz in NowFunc Minor: Timezones with UTC and +00:00 offsets are the same Jun 20, 2024
@alamb alamb changed the title Minor: Timezones with UTC and +00:00 offsets are the same Fix Timezones with UTC and +00:00 offsets are the same Jun 20, 2024
@alamb alamb changed the title Fix Timezones with UTC and +00:00 offsets are the same Consider timezones with UTC and +00:00 to be the same Jun 20, 2024
@alamb alamb merged commit 4a0c7f3 into apache:main Jun 21, 2024
25 of 26 checks passed
@alamb
Copy link
Contributor

alamb commented Jun 21, 2024

Thanks everyone!

xinlifoobar pushed a commit to xinlifoobar/datafusion that referenced this pull request Jun 22, 2024
@marvinlanhenke @alamb @xinlifoobar
Consider timezones with UTC and +00:00 to be the same (apache#10960)
aee6e4d 
* feat: add temporal_coercion check

* fix: add return stmt

* chore: add slts

* fix: remove println

* Update datafusion/expr/src/type_coercion/binary.rs

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
xinlifoobar pushed a commit to xinlifoobar/datafusion that referenced this pull request Jun 22, 2024
@marvinlanhenke @alamb @xinlifoobar
Consider timezones with UTC and +00:00 to be the same (apache#10960)
87cf939 
* feat: add temporal_coercion check

* fix: add return stmt

* chore: add slts

* fix: remove println

* Update datafusion/expr/src/type_coercion/binary.rs

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Jun 24, 2024
@marvinlanhenke @alamb @appletreeisyellow
Consider timezones with UTC and +00:00 to be the same (apache#10960)
21c0f6e 
* feat: add temporal_coercion check

* fix: add return stmt

* chore: add slts

* fix: remove println

* Update datafusion/expr/src/type_coercion/binary.rs

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@appletreeisyellow appletreeisyellow mentioned this pull request Jun 24, 2024
Draft
@alamb alamb mentioned this pull request Jun 24, 2024
Open
9 tasks
appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Jun 25, 2024
@marvinlanhenke @alamb @appletreeisyellow
Consider timezones with UTC and +00:00 to be the same (apache#10960)
4f49e3e 
* feat: add temporal_coercion check

* fix: add return stmt

* chore: add slts

* fix: remove println

* Update datafusion/expr/src/type_coercion/binary.rs

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@appletreeisyellow appletreeisyellow mentioned this pull request Jun 25, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
logical-expr Logical plan and expressions sqllogictest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timezone UTC is not considered equal to +00:00
3 participants
@marvinlanhenke @alamb @erratic-pattern