Unify query part handling for rows and aggregates #492
Conversation
| // fields | ||
| let returning_select = match &query.fields { | ||
| Some(_) => { | ||
| let (_, returning_select) = crate::translation::query::root::translate_rows_query( |
There was a problem hiding this comment.
instead of calling translate_rows_query and translate_aggregate_query, and stitch everything together, we call translate_query which does all of this for us.
| /// affected, but it must return at least one. A `SelectSet` describes this as a type, so we can | ||
| /// convert an optional returning `Select` and an optional aggregate `Select` to a `SelectSet`, | ||
| /// failing if neither exists. | ||
| fn rows_and_aggregates_to_select_set( |
There was a problem hiding this comment.
done in translate_query.
| // fields | ||
| let returning_select = match &query.fields { | ||
| Some(_) => { | ||
| let (_, returning_select) = crate::translation::query::root::translate_rows_query( |
There was a problem hiding this comment.
Instead of calling the function and then set the from clause to the returned select outside of it, we just pass the from clause into the function and have it set it.
|
|
||
| /// Translate a query to sql ast. | ||
| /// We return a SELECT for the 'rows' field and a SELECT for the 'aggregates' field. | ||
| pub fn translate_query( |
There was a problem hiding this comment.
Rewritten a bit and moved to ::root.
| &join_field.query, | ||
| )?; | ||
|
|
||
| // add join expressions to row / aggregate selects |
There was a problem hiding this comment.
injecting the join predicate means we don't need to unpack afterwards.
There was a problem hiding this comment.
Test that limit and order by has been applied to the aggregates query.
| ORDER BY | ||
| "%1_Invoice"."InvoiceId" ASC | ||
| LIMIT | ||
| 10 OFFSET 5 |
There was a problem hiding this comment.
yep, has been applied.
There was a problem hiding this comment.
Now we apply the limit. We also changed the test a bit so it is easy to verify.
There was a problem hiding this comment.
the test request did not contain order for the album, so it was ambigious. fixed.
soupi
changed the title
2 participants
What
fixes https://hasurahq.atlassian.net/browse/PG-112
Previously, order by and limit clauses were not applied for the aggregates part of the query as the spec said it was not needed, however, that is a mistake which we now fix.
How
To fix this, we unify the way we handle rows and aggregates.
Callers will no longer need to call
translate_rows_queryandtranslate_aggregates_queryseparately and then try to stitch them together, but rather call one part,translate_query, which will call both.Each translate part will call the same function,
translate_query_partwhich translates the common elements (where, order by, limit, etc.), but will different in the way it handles fields or aggregates.This way, we make sure that an ndc-spec query is translated consistently between the two parts - rows and aggregates.