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

Validate time with time zone well-formedness #9280

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Validate time with time zone well-formedness #9280

merged 2 commits into from
Sep 22, 2021

Conversation

findepi
Copy link
Member

@findepi findepi commented Sep 17, 2021 •
edited
Loading

For #5738

@findepi findepi requested review from martint and losipiuk September 17, 2021 08:38
@cla-bot cla-bot bot added the cla-signed label Sep 17, 2021
@findepi findepi closed this Sep 17, 2021
@findepi findepi deleted the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch September 17, 2021 08:39
@findepi findepi restored the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch September 17, 2021 08:39
@findepi findepi reopened this Sep 17, 2021
@findepi findepi force-pushed the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch from 1c8ac50 to f6b1bdf Compare September 17, 2021 08:40
@losipiuk
Copy link
Member

CI: Cannot instantiate class io.trino.type.TestTimeWithTimeZoneType

losipiuk
losipiuk reviewed Sep 17, 2021
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/type/SqlTimeWithTimeZone.java
if (picoseconds < 0 || picoseconds >= PICOSECONDS_PER_DAY) {
throw new IllegalArgumentException("picoseconds is out of range: " + picoseconds);
}
// TIME WITH TIME ZONE's valid offsets are [-14:00, 14:00]
Copy link
Member

Choose a reason for hiding this comment

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

Did we enforce it already (just in a more ugly way during query processing). Or will some queries which used to work may start failing now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I checked literal form prior, but that's not enough. Filed #9288.

@findepi findepi force-pushed the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch from f6b1bdf to 7b45344 Compare September 17, 2021 12:24
@findepi findepi mentioned this pull request Sep 17, 2021
Merged
@findepi findepi force-pushed the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch from 7b45344 to d1257c2 Compare September 17, 2021 12:43
@findepi findepi force-pushed the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch from d1257c2 to 2ef9875 Compare September 17, 2021 20:18
@findepi
Copy link
Member Author

findepi commented Sep 17, 2021

@martint ptal

1 similar comment
@findepi
Copy link
Member Author

findepi commented Sep 21, 2021

@martint ptal

@findepi
Copy link
Member Author

findepi commented Sep 21, 2021

CI #8691

@findepi
Copy link
Member Author

findepi commented Sep 21, 2021

@martint do you see things like #9288 as a prerequisite to merging this?

@martint
Copy link
Member

martint commented Sep 21, 2021

No, I think we can fix them independently. This provides an additional safety check for any place that constructs a SqlTimeWithTimeZone object

@findepi
Copy link
Member Author

findepi commented Sep 21, 2021

i am thinking the same.

@findepi findepi merged commit 0127c36 into trinodb:master Sep 22, 2021
@findepi findepi deleted the findepi/validate-time-with-time-zone-well-formedness-fe56d2 branch September 22, 2021 07:00
@github-actions github-actions bot added this to the 363 milestone Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@losipiuk losipiuk losipiuk approved these changes

@martint martint martint approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
363
Development

Successfully merging this pull request may close these issues.
3 participants
@findepi @losipiuk @martint