Skip to content

Moving some interpolated functions out of experimental #650

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 1 commit into from
Feb 6, 2023
Merged

Moving some interpolated functions out of experimental #650

merged 1 commit into from
Feb 6, 2023

Conversation

thatzopoulos
Copy link
Contributor

Moved interpolation functions for average, delta, and rate out of the toolkit_experimental schema. Tests now check that the output of these functions match the output of the new toolkit_experimental.interpolated_* versions from this PR.

@thatzopoulos
Copy link
Contributor Author

Open to suggestions on what to call intervalparam since we can't call it interval anymore

thatzopoulos
thatzopoulos commented Dec 7, 2022
View reviewed changes
extension/src/stabilization_info.rs Outdated

crate::functions_stabilized_at! {
STABLE_FUNCTIONS
"1.13.0" => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Marked this as stable in 1.13.0 since it is not going in the upcoming release

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not going into the upcoming release (we can actually discuss this since we pushed the release out to next week), shouldn't this be marked as stable in 1.14.0?

@davidkohn88
Copy link
Contributor

Open to suggestions on what to call intervalparam since we can't call it interval anymore

Why can't we call it interval anymore? I think you just need to add double quotes around the name in your SQL and it'll work just fine.

@davidkohn88
Copy link
Contributor

One thing I'd like to do is a) write a short readme that codifies our decision about when we are doing the keep the experimental version around for one release thing and b) add a comment that these will be removed in the next release and mentions the readme.

The readme could be something like this:

We decided that when we are stabilizing accessors that have no changes from their current function signatures we are going  to move the function to the public schema and stabilize but also re-create the version in the experimental schema and keep it around for one version. 

We are doing this because some people may have started using experimental accessors in their code and this gives them a version to update their code without it breaking. We are not going to do this when we are stabilizing a full function family but only for accessors that work on already stabilized function families, these type of accessors are safer to use in production settings as their is little risk that something will be stored in, say, a continuous aggregate that would break, and we'd like to encourage people to use them and not cause unnecessary breakage pain when possible. 

We will only keep them around for one version however so they will have a limited time to update their code and not continue using things in the experimental schema indefinitely.

@epgts does that capture what we discussed reasonably well?

@thatzopoulos thatzopoulos reopened this Dec 9, 2022
WireBaron
WireBaron reviewed Dec 10, 2022
View reviewed changes
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.

Not sure if you were looking for a review yet since this is still marked as draft, but added a few comments for you.

extension/src/counter_agg.rs Outdated
summary: CounterSummary<'a>,
start: crate::raw::TimestampTz,
interval: crate::raw::Interval,
intervalparam: crate::raw::Interval,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe duration or perhaps interval_len (general suggestion for all params with this name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to duration

extension/src/counter_agg.rs Outdated
// Public facing interpolated_delta
extension_sql!(
"\n\
CREATE FUNCTION toolkit_experimental.interpolated_delta(summary countersummary, start timestamptz, intervalparam interval, prev countersummary, next countersummary) RETURNS double precision AS $$\n\
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see this indented and perhaps separated a bit to make it more readable. You can use the extension_sql blocks we use for CREATE AGGREGATE as a guide.

extension/src/stabilization_info.rs Outdated
"1.13.0" => {
interpolated_average(timeweightsummary,timestamp with time zone,interval,timeweightsummary,timeweightsummary),
interpolated_delta(countersummary,timestamp with time zone,interval,countersummary,countersummary),
interpolated_rate(countersummary,timestamp with time zone,interval,countersummary,countersummary),
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent these to be consistent with the following blocks.

extension/src/stabilization_info.rs Outdated

crate::functions_stabilized_at! {
STABLE_FUNCTIONS
"1.13.0" => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not going into the upcoming release (we can actually discuss this since we pushed the release out to next week), shouldn't this be marked as stable in 1.14.0?

@thatzopoulos thatzopoulos marked this pull request as ready for review December 12, 2022 20:13
@thatzopoulos thatzopoulos force-pushed the th/stabilize-some-interpolated-functions branch 2 times, most recently from ffac152 to fd834d4 Compare December 13, 2022 17:41
@thatzopoulos thatzopoulos force-pushed the th/stabilize-some-interpolated-functions branch from b89ac62 to 48decc1 Compare February 6, 2023 19:25
@thatzopoulos
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 6, 2023

Build succeeded:

@bors bors bot merged commit d3b1fe1 into main Feb 6, 2023
@bors bors bot deleted the th/stabilize-some-interpolated-functions branch February 6, 2023 19:55
@jerryxwu jerryxwu mentioned this pull request Feb 8, 2023
Merged
nbraem added a commit to nbraem/docs that referenced this pull request Jul 23, 2024
@nbraem
change interpolated_delta and interpolated_rate to stable
1adb15f 
according to timescale/timescaledb-toolkit#650
@nbraem nbraem mentioned this pull request Jul 23, 2024
Merged
11 tasks
billy-the-fish added a commit to timescale/docs that referenced this pull request Aug 1, 2024
@nbraem @billy-the-fish
change interpolated_delta and interpolated_rate to stable (#3339)
832a3d8 
according to timescale/timescaledb-toolkit#650

Co-authored-by: Iain Cox <[email protected]>
billy-the-fish added a commit to timescale/docs that referenced this pull request Aug 1, 2024
@nbraem @billy-the-fish
change interpolated_delta and interpolated_rate to stable (#3339)
c67c31f 
according to timescale/timescaledb-toolkit#650

Co-authored-by: Iain Cox <[email protected]>
billy-the-fish added a commit to timescale/docs that referenced this pull request Aug 12, 2024
@nbraem @billy-the-fish
change interpolated_delta and interpolated_rate to stable (#3339)
39ba5cf 
according to timescale/timescaledb-toolkit#650

Co-authored-by: Iain Cox <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syvb syvb Awaiting requested review from syvb

@epgts epgts Awaiting requested review from epgts

2 more reviewers

@rtwalker rtwalker rtwalker approved these changes

@WireBaron WireBaron WireBaron approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thatzopoulos @davidkohn88 @rtwalker @WireBaron