-
Notifications
You must be signed in to change notification settings - Fork 891
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
stable expressions as constant in vectorized filters #6203
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6203 +/- ##
==========================================
+ Coverage 65.19% 65.28% +0.09%
==========================================
Files 247 247
Lines 57271 57480 +209
Branches 12732 12765 +33
==========================================
+ Hits 37337 37525 +188
- Misses 18055 18089 +34
+ Partials 1879 1866 -13
... and 72 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
@fabriziomello, @gayyappan: please review this pull request.
|
return (func_volatile(func_id) == PROVOLATILE_VOLATILE); | ||
} | ||
|
||
static bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function contains some logic. So, could you add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I believe you are just looking for expressions that can be constified at runtime (as opposed to plantime) here. It would help to add a comment.
I was thrown off by the commit message that said we can now support ts> now() - '1 day'::interval.
We already do plan time constification for that in some cases. I suggest also modifying the commit message to elaborate that this change now introduces support for ts < now() , ts> now() etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already do plan time constification for that in some cases.
That's a different thing, it's for runtime chunk exclusion that is done in this small scope only IIRC. The underlying DecompressChunk executor doesn't see the constified values, and I don't think there's a good way to pass these values down to the underlying nodes. So what I'm doing is also constifying the values there, so that we can perform vectorized filters with them.
I'll try to improve the comments and the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. To clarify: ts > now() - <> works with plantime chunk exclusion. But I wasn't aware that it doesn't get passed along via the filters to decompress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice speedup, one minor comment. Rest LGTM.
This allows vectorizing common filters such as ts > now() - interval '1 day'.
This means vectorizing very common filters such as
ts > now() - interval '1 day'
.The only query with stable expressions we have in tsbench shows a 130% improvement:
https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=2853&var-run2=2855&var-threshold=0.05&var-use_historical_thresholds=false
Disable-check: force-changelog-file