Query the current aggregation bucket #6293
Conversation
This makes sure that when the default order is applied the column used in ORDER BY is also included in the SELECT projection
| impl AllowOnlySelectQueries { | ||
| /// Returns an error if the `set_expr` is not a `SELECT` expression. | ||
| fn visit_set_expr(&self, set_expr: &ast::SetExpr) -> Result<()> { | ||
| fn visit_set_expr(set_expr: &ast::SetExpr) -> Result<()> { |
There was a problem hiding this comment.
The changes in this file seem unrelated, right?
|
|
||
| pub trace: bool, | ||
|
|
||
| pub aggregation_current: Option<AggregationCurrent>, |
There was a problem hiding this comment.
When is this None? I assume when the query does not access aggregations
| components, | ||
| nested_path, | ||
| ) | ||
| .map(Some); |
There was a problem hiding this comment.
These changes also seem unrelated
| "exclude" => return Ok(Some(AggregationCurrent::Exclude)), | ||
| "include" => return Ok(Some(AggregationCurrent::Include)), | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
This could be a FromStr implementation on AggregationCurrent
| .expect("field names are valid"); | ||
|
|
||
| // Loading the current bucket is not supported for nested queries | ||
| aggregation_current = None; |
There was a problem hiding this comment.
One case where that might be wanted is when you query e.g., for a list of tokens and want stats for each of them, i.e., some structure like { tokens { id tokenStats(..) { .. } } }.
But it's fine to leave that as a TODO for now.
I am not entirely sure whether silently suppressing current: include is the right approach. I think we should produce an error during query validation for this case.
| /// | ||
| /// Contains a `--FILTERS;` comment that can be replaced with additional filters like `and c.block$ <= $1`. | ||
| /// The filters are applied to the SQL query that loads the time series entities, not to the aggregated values. | ||
| pub(crate) select_current_sql: String, |
There was a problem hiding this comment.
Nice! It would be good to have tests in the tests module that check that we generate the SQL that we expect to generate. That will be helpful if we ever need to change query generation (and helps understand what these queries look like)
| return Ok(None); | ||
| } | ||
|
|
||
| // Supporting window and/or multiple entity queries would make the existing SQL queries even more complicated. |
There was a problem hiding this comment.
It's been a long time since I looked at this, but as I remember for nested queries, the EntityQuery will have an EntityWindow; it will be more than one only if the child_type is an interface (i.e., not in our case) so that it should be possible to have a query select .. from (select <whatever we do for the only window> from agg_table union all <current_rollup_sql>) order by ..
But as I said, that can be done in a separate PR
| ), | ||
| }; | ||
|
|
||
| // This is not supported because the use of `SELECT *` does not always produce the expected results when combined with `UNION ALL`. |
There was a problem hiding this comment.
We can get rid of AttributesNames::All and simplify a bunch of stuff (also a separate PR)
| Ok(layout | ||
| .rollups | ||
| .iter() | ||
| .find(|rollup| rollup.agg_table.object.as_str() == entity.table.meta.object.as_str())) |
There was a problem hiding this comment.
This would be nicer as a method on Layout
| .expect("aggregation entities have timestamps"); | ||
|
|
||
| EntityOrder::Descending(ts.name.to_string(), ts.value_type) | ||
| } |
There was a problem hiding this comment.
I assume we need to resolve that here because the code in relational_queries doesn't have enough information to do it there. It's a little nasty that in some cases we turn the default into a real order, and in others we pass `EntityOrder::Default' and let query generation resolve it. But I am not sure how to make that more sensible (and definitely out of scope for this PR)
This PR adds support for querying the current, partially filled aggregation bucket with GraphQL.
How does it work?
When the
current: includeis specified in the GraphQL request for an aggregation, the graph-node loads the most recent time series entities that do not currently form a complete bucket and calculates the aggregates on the fly. Then, the results are merged with the relevant complete buckets to produce one response list.Current limitations
Todos: