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

Support all time_bucket variants in plan time chunk exclusion #6552

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Support all time_bucket variants in plan time chunk exclusion #6552

merged 1 commit into from
Jan 22, 2024

Conversation

svenklemm
Copy link
Member

Since the optional time_bucket arguments like offset, origin and timezone shift the output by at most bucket width we can optimize these similar to how we optimize the other time_bucket constraints.

Fixes #4825

@github-actions GitHub Actions
Copy link

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

Powered by pull-review

@svenklemm svenklemm added this to the TimescaleDB 2.14 milestone Jan 20, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 20, 2024 •
edited
Loading

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (2174a71) 79.72% compared to head (6a87bbf) 79.68%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/planner/expand_hypertable.c 75.67% 1 Missing and 8 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6552      +/-   ##
==========================================
- Coverage   79.72%   79.68%   -0.05%     
==========================================
  Files         188      188              
  Lines       36871    36809      -62     
  Branches     9327     9313      -14     
==========================================
- Hits        29397    29330      -67     
- Misses       3118     3129      +11     
+ Partials     4356     4350       -6

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

@fabriziomello
Copy link
Contributor

Thanks for working on it. Can I close an old WIP PR that intend to do part of this job #4854?

@svenklemm
Copy link
Member Author

Thanks for working on it. Can I close an old WIP PR that intend to do part of this job #4854?

Yes this should be a superset of the features of your PR

@fabriziomello fabriziomello mentioned this pull request Jan 20, 2024
Closed
@svenklemm svenklemm requested a review from cevian January 21, 2024 12:04
@svenklemm svenklemm force-pushed the exclude_time_bucket_tz branch 2 times, most recently from dfa376d to 6a87bbf Compare January 21, 2024 13:24
@svenklemm
Support all time_bucket variants in plan time chunk exclusion
6a87bbf 
Since the optional time_bucket arguments like offset, origin and
timezone shift the output by at most bucket width we can optimize
these similar to how we optimize the other time_bucket constraints.

Fixes timescale#4825
jnidzwetzki
jnidzwetzki approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@jnidzwetzki jnidzwetzki left a comment

Choose a reason for hiding this comment

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

LGTM

src/planner/expand_hypertable.c
@@ -493,10 +514,11 @@ ts_transform_time_bucket_comparison(Expr *node)

/*
* When the time_bucket constraint matches the start of the bucket
* and we have a less than constraint we can skip adding the
* full bucket.
* and we have a less than constraint and no offset we can skip
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:

Suggested change
* and we have a less than constraint and no offset we can skip
* and we have a less than constraint and no offset we can skip

cevian
cevian reviewed Jan 22, 2024
View reviewed changes
src/planner/expand_hypertable.c

Const *width = linitial(time_bucket->args);
Oid opno = op->opno;
Assert(list_length(time_bucket->args) == 2 || list_length(time_bucket->args) == 3 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where a 4 arg variant would bail out above?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is only 2,3 and 5 args versions

src/planner/expand_hypertable.c
*/
if (strategy == BTLessStrategyNumber && integralValue % integralWidth == 0)
if (strategy == BTLessStrategyNumber && list_length(time_bucket->args) == 2 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

should we try to handle the offset % period = 0 case here too? This is just an optimization? In that case probably not worth handling. But if there are correctness implications...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just an optimization, with the additional args it gets a bit more complicated

@svenklemm svenklemm merged commit acd69d4 into timescale:main Jan 22, 2024
40 of 44 checks passed
@timescale-automation
Copy link

Automated backport to 2.13.x not done: cherry-pick failed.

Git status

HEAD detached at origin/2.13.x
You are currently cherry-picking commit acd69d4cd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   test/expected/plan_expand_hypertable-13.out
	modified:   test/expected/plan_expand_hypertable-14.out
	modified:   test/expected/plan_expand_hypertable-15.out
	modified:   test/expected/plan_expand_hypertable-16.out
	modified:   test/sql/include/plan_expand_hypertable_query.sql

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   src/planner/expand_hypertable.c

Job log

@timescale-automation timescale-automation added the auto-backport-not-done Automated backport of this PR has failed non-retriably (e.g. conflicts) label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cevian cevian cevian left review comments

@jnidzwetzki jnidzwetzki jnidzwetzki approved these changes

@antekresic antekresic antekresic approved these changes

@erimatnor erimatnor Awaiting requested review from erimatnor

@gayyappan gayyappan Awaiting requested review from gayyappan

@fabriziomello fabriziomello Awaiting requested review from fabriziomello

Assignees

@svenklemm svenklemm

Labels
auto-backport-not-done Automated backport of this PR has failed non-retriably (e.g. conflicts)
Projects
None yet
Milestone
TimescaleDB 2.14
Development

Successfully merging this pull request may close these issues.

[Bug]: Continuous aggregate performance with time_bucket and timezone
6 participants
@svenklemm @fabriziomello @timescale-automation @cevian @antekresic @jnidzwetzki