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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return INTERVAL from duration_in(TEXT, StateAgg). #378

Merged
merged 1 commit into from
Apr 15, 2022
Merged

Conversation

epgts
Copy link
Contributor

@epgts epgts commented Mar 28, 2022

No description provided.

@epgts epgts requested a review from WireBaron March 28, 2022 18:09
@epgts epgts changed the title Try to return INTERVAL from duration_in. Partially working... Return INTERVAL from duration_in(TEXT, StateAgg). Mar 30, 2022
@epgts epgts marked this pull request as ready for review March 30, 2022 20:46
Copy link
Contributor

@WireBaron WireBaron left a comment

Choose a reason for hiding this comment

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

Timestamps are a bit weird, and this does return different results from what postgres does for some calculations. I think this is actually fine, but I do want to think on this a little more.

@@ -279,11 +292,13 @@ mod tests {
None,
);
assert_eq!(
31536120000000,
"8760:02:00",
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably we wouldn't want to call out to postgres for every timestamptz calculation, but it is worth considering that this a bit different from the postgres result:

postgres=# select ('2020-12-31 00:02:00+00'::timestamptz - '2020-01-01 00:00:00+00'::timestamptz)::interval;
     interval      
-------------------
 365 days 00:02:00
(1 row)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's interesting! So it's the responsibility of the caller to do the math and fill out all fields of Interval?! What a pain. I'll look into it. Maybe I'm just holding it wrong.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It took me a while to figure out how and where postgresql is doing this, but I did find it, and I've documented it here, and I'm calling the same function.

Thanks!

@epgts epgts marked this pull request as draft March 31, 2022 13:36
@epgts epgts marked this pull request as ready for review April 11, 2022 16:09
Copy link
Contributor

@WireBaron WireBaron left a comment

Choose a reason for hiding this comment

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

Looks good!

@epgts
Copy link
Contributor Author

epgts commented Apr 15, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 15, 2022

Build succeeded:

@bors bors bot merged commit ad84397 into main Apr 15, 2022
@bors bors bot deleted the eg/state-interval branch April 15, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants