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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove useless function from regression tests #7750

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabriziomello
Copy link
Contributor

@fabriziomello fabriziomello commented Feb 19, 2025

The temporary function cagg_get_bucket_function was created to be used in the update script for 2.14.2 to 2.15.0 and for some regression tests, but in 2.16.0 (#7042) we added a new persistent function cagg_get_bucket_function_info as a replacement so used it instead.

Leftover from #7042 refactoring PR.

Disable-check: force-changelog-file

@fabriziomello fabriziomello added tech-debt Needs refactoring and improvement tasks related to the source code and its architecture. force-auto-backport Automatically backport this PR or fix of this issue, even if it's not marked as "bug" labels Feb 19, 2025
@fabriziomello fabriziomello self-assigned this Feb 19, 2025
@fabriziomello fabriziomello force-pushed the remove_useless_cagg_get_bucket_function branch from 8e5f08d to bd5e4e6 Compare February 19, 2025 13:50
Copy link

@svenklemm, @gayyappan: please review this pull request.

Powered by pull-review

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.94%. Comparing base (59f50f2) to head (a45ebd0).
Report is 789 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7750      +/-   ##
==========================================
+ Coverage   80.06%   81.94%   +1.87%     
==========================================
  Files         190      246      +56     
  Lines       37181    45258    +8077     
  Branches     9450    11305    +1855     
==========================================
+ Hits        29770    37087    +7317     
- Misses       2997     3715     +718     
- Partials     4414     4456      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fabriziomello fabriziomello force-pushed the remove_useless_cagg_get_bucket_function branch 2 times, most recently from 4e22a2d to 810a951 Compare February 20, 2025 16:13
Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

linking to C functions is problematic especially when this are removed as that will create a dependency on .so file

@fabriziomello fabriziomello force-pushed the remove_useless_cagg_get_bucket_function branch 2 times, most recently from 9548d4c to 7a1e2e7 Compare February 21, 2025 12:47
@fabriziomello fabriziomello changed the title Remove useless cagg_get_bucket_function function Remove useless function from regression tests Feb 21, 2025
@fabriziomello fabriziomello force-pushed the remove_useless_cagg_get_bucket_function branch from 7a1e2e7 to 0d1e769 Compare February 21, 2025 17:01
The temporary function `cagg_get_bucket_function` was created to be
used in the update script for 2.14.2 to 2.15.0 and for some regression
tests, but in 2.16.0 (timescale#7042) we added a new persistent function
`cagg_get_bucket_function_info` as a replacement so used it instead.

Leftover from timescale#7042 refactoring PR.
@fabriziomello fabriziomello force-pushed the remove_useless_cagg_get_bucket_function branch from 0d1e769 to a45ebd0 Compare February 21, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force-auto-backport Automatically backport this PR or fix of this issue, even if it's not marked as "bug" tech-debt Needs refactoring and improvement tasks related to the source code and its architecture.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants