Fix broken pushdown queries #1098
Conversation
JamesGuthrie
force-pushed
the
jg/fix-broken-pushdown
branch
4 times, most recently
from
6b5cc12 to
d9974ff
Compare
| // | ||
| // Depending on the query, we may need to look further back in time than | ||
| // the `start` to deliver results from `start` onwards. For instance, a | ||
| // range query such as `rate(metric_one[5m])` in the range `(T1, T2)` will |
There was a problem hiding this comment.
range queries do not have lookbacks. The range is as stated and the only thing that matters is the bucket width.
JamesGuthrie
force-pushed
the
jg/fix-broken-pushdown
branch
from
d9974ff to
7e564eb
Compare
JamesGuthrie
requested review from
niksajakovljevic
and removed request for
a team
JamesGuthrie
changed the title
JamesGuthrie
force-pushed
the
jg/fix-broken-pushdown
branch
from
7e564eb to
604ffab
Compare
|
This fixes timescale/promscale_extension#72 |
| // The timeFilter's time range is determined by `findMinMaxTime` on the | ||
| // expression being evaluated, and is the widest span of _all_ | ||
| // subexpressions (effectively [min(scan_start), end]). | ||
| // This means that, for instance, if the following vector selector and |
There was a problem hiding this comment.
Nit: I am getting a bit confused with wording here. From what I understand there is an instant vector selector and range vector selector. So in both cases it is vector selector.
There was a problem hiding this comment.
How about this:
| // This means that, for instance, if the following vector selector and | |
| // This means that, for instance, if the following range selector and |
| @@ -2506,6 +2506,10 @@ func TestPromQLQueryEndpoint(t *testing.T) { | |||
| name: "two pushdowns, same metric different matchers", | |||
| query: `sum(rate(metric_2{foo = "bar"}[5m]))/sum(rate(metric_2[5m]))`, | |||
| }, | |||
| { | |||
| name: "one pushdown, matrix", | |||
There was a problem hiding this comment.
Can you maybe add some tests with offset?
There was a problem hiding this comment.
Can you be more specific? There are already quite a few tests which use offsets in promql_query_endpoint_test.go. Is there a specific case that you would like to exercise?
There was a problem hiding this comment.
I was thinking something like: rate(metric_2[1m])/metric_2 offset 2m
| filter := metadata.timeFilter | ||
| sh := metadata.selectHints | ||
| var start, end string | ||
| if sh != nil { |
There was a problem hiding this comment.
Nit: Since this only apply when selectHints are present I think it would be nice to document in what cases are they set. Changing start/end time seems scary to me so I'd like us to have a good understanding when that happens.
JamesGuthrie
force-pushed
the
jg/fix-broken-pushdown
branch
from
604ffab to
ec03992
Compare
Under certain circumstances, pushdown queries using promscale-extension
aggregates fail.
The failure comes from the fact that the start and end parameters passed
to the aggregate don't correctly align with the actual data which is
aggregated over by the query.
A call to the prom_rate aggregate looks something like the following:
SELECT
prom_rate(start, end, step_size, range, sample_time, sample_value)
FROM a_metric
WHERE a_metric.t >= data_start
AND a_metric.t <= data_end
If the time span [data_start, data_end] delivers results which are
outside of the time span [start, end], the aggregate function errors.
In general, this bug appeared when an aggregate pushdown function (such
as PromQL rate) is used together with a vector selector, such that the
lookback of the vector selector is larger than that of the aggregate
pushdown.
The fix is to build the SQL query using the correct values for
data_start and data_end.
JamesGuthrie
force-pushed
the
jg/fix-broken-pushdown
branch
from
225629a to
2904a7f
Compare
|
FYI I've resolved conflicts and squashed. |
Description
Under certain circumstances, pushdown queries using promscale-extension
aggregates fail.
The failure comes from the fact that the start and end parameters passed
to the aggregate don't correctly align with the actual data which is
aggregated over by the query.
A call to the prom_rate aggregate looks something like the following:
If the time span [data_start, data_end] delivers results which are
outside of the time span [start, end], the aggregate function errors.
In general, this bug appeared when an aggregate pushdown function (such
as PromQL rate) is used together with a vector selector, such that the
lookback of the vector selector is larger than that of the aggregate
pushdown.
The fix is to build the SQL query using the correct values for
data_start and data_end.
Merge requirements
Please take into account the following non-code changes that you may need to make with your PR: