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

Cleanup public API #2531

Merged
merged 1 commit into from Oct 13, 2020
Merged

Cleanup public API #2531

merged 1 commit into from Oct 13, 2020

Conversation

k-rus
Copy link
Contributor

@k-rus k-rus commented Oct 12, 2020 •
edited

Removes unlrelated column schedule_interval from
timescaledb_information.continuous_aggregates view and simplifies it.
Renames argument cagg in refresh_continuous_aggregate into
continuous_aggregate as in add_continuous_aggregate_policy.

Part of #2521

@k-rus k-rus requested a review from a team as a code owner October 12, 2020 12:15
@k-rus k-rus requested review from pmwkaa, WireBaron and svenklemm and removed request for a team October 12, 2020 12:15
@k-rus k-rus added this to the 2.0.0 milestone Oct 12, 2020
@k-rus k-rus added the cleanup label Oct 12, 2020
@k-rus k-rus self-assigned this Oct 12, 2020
@k-rus k-rus requested a review from erimatnor October 12, 2020 12:16
erimatnor
erimatnor reviewed Oct 12, 2020
View reviewed changes
Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

There's a typo in the commit title and I think these changes require an update script.

@k-rus k-rus changed the title Clenup public API Cleanup public API Oct 12, 2020
@codecov
Copy link

codecov bot commented Oct 12, 2020 •
edited

Codecov Report

Merging #2531 into master will decrease coverage by 1.44%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2531      +/-   ##
==========================================
- Coverage   91.66%   90.21%   -1.45%     
==========================================
  Files         144      212      +68     
  Lines       21899    34178   +12279     
==========================================
+ Hits        20073    30833   +10760     
- Misses       1826     3345    +1519
Flag Coverage Δ
#cron ?
#pr ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/adts/simplehash.h 95.54% <ø> (+0.23%) ⬆️
src/adts/vec.h 93.65% <ø> (+0.54%) ⬆️
src/catalog.h 100.00% <ø> (ø)
src/chunk.c 95.31% <ø> (-0.60%) ⬇️
src/chunk_append/transform.c 96.42% <ø> (ø)
src/compression_with_clause.c 87.06% <ø> (+4.28%) ⬆️
src/copy.c 87.86% <ø> (+0.71%) ⬆️
src/cross_module_fn.c 70.00% <ø> (+1.45%) ⬆️
src/custom_type_cache.c 81.81% <ø> (ø)
src/debug_guc.c 92.63% <ø> (ø)
... and 281 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85428bc...45468d5. Read the comment docs.

@k-rus k-rus marked this pull request as draft October 12, 2020 13:03
@k-rus k-rus force-pushed the api-cleanup branch 2 times, most recently from 2e1b2fc to 9592061 Compare October 12, 2020 13:34
@k-rus k-rus marked this pull request as ready for review October 12, 2020 13:42
@k-rus k-rus requested a review from erimatnor October 12, 2020 13:43
@k-rus
Copy link
Contributor Author

k-rus commented Oct 12, 2020

There's a typo in the commit title and I think these changes require an update script.

@erimatnor Thank you! I fixed both, I believe.

gayyappan
gayyappan reviewed Oct 12, 2020
View reviewed changes
test/sql/updates/post.continuous_aggs.v2.sql Outdated
@@ -12,7 +12,7 @@ CALL refresh_continuous_aggregate('cagg.realtime_mat',NULL,NULL);

SELECT * FROM cagg.realtime_mat ORDER BY bucket, location;

SELECT view_name, schedule_interval, materialized_only, materialization_hypertable_name FROM timescaledb_information.continuous_aggregates ORDER BY view_name::text;
SELECT view_name, materialized_only, materialization_hypertable_name FROM timescaledb_information.continuous_aggregates ORDER BY view_name::text;
Copy link
Contributor

Choose a reason for hiding this comment

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

this test should still compare the schedule_interval : select from continuous_aggregates view (pre 2.0). b) Select schedule_interval from jobs table for 2.0+ releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gayyappan Does it mean that there should be two versions of the query? Does it mean new version of this update file needs to be introduced?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is executed post update, we don't need 2 versions. selecting schedule_interval for the policies from the jobs view should suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test for schedule_interval

@k-rus k-rus marked this pull request as draft October 12, 2020 17:45
@k-rus k-rus marked this pull request as ready for review October 12, 2020 19:44
@k-rus k-rus requested a review from gayyappan October 12, 2020 19:44
@k-rus
Cleanup public API
45468d5 
Removes unlrelated column schedule_interval from
timescaledb_information.continuous_aggregates view and simplifies it.
Renames argument cagg in refresh_continuous_aggregate into
continuous_aggregate as in add_continuous_aggregate_policy.

Part of timescale#2521
@k-rus k-rus merged commit 85095b6 into timescale:master Oct 13, 2020
@k-rus k-rus deleted the api-cleanup branch October 13, 2020 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmwkaa pmwkaa pmwkaa approved these changes

@erimatnor erimatnor erimatnor approved these changes

@WireBaron WireBaron Awaiting requested review from WireBaron

@svenklemm svenklemm Awaiting requested review from svenklemm

@gayyappan gayyappan Awaiting requested review from gayyappan

Assignees

@k-rus k-rus

Labels
None yet
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@k-rus @pmwkaa @erimatnor @gayyappan