Repository navigation
Conversation
| } else if !stat_order_by.is_empty() { | ||
| format!("{} ORDER BY {}", final_query, stat_order_by.join(", ")) |
There was a problem hiding this comment.
If user has set ORDER BY group and stat has ORDER BY x, shouldn't we get ORDER BY group, x here? It seems like we're ignoring the stat order if user has set an explicit order.
For example if you have some timeseries where timestamps are in arbitrary order, you might want to order by something that brings out a foreground vs background. But because arbitrary order of the timestamps, we do not get that secondary order and lines are all connected all over the place.
| /// ordering on the final, outermost query only — nested ORDER BY is | ||
| /// rejected by some backends (T-SQL error 1033) and is semantically | ||
| /// meaningless in derived tables elsewhere. | ||
| order_by: Vec<String>, |
There was a problem hiding this comment.
I think we should pick a better name for this like layer_order or inner_order to distinguish the fact that this is not the order_by as provided by users.
| /// the stat metadata. | ||
| /// - `Transformed` → appends to the existing `order_by` and preserves the | ||
| /// query and stat metadata. | ||
| pub fn wrap_with_order_by( |
There was a problem hiding this comment.
It sort of feels like now that order_by is a field instead of part of the query, this should become a StatResult method like with_order().
This branch has not been deployed
2 participants
Fix #576
This PR fixes two dialect issues uncovered in #569 but shelved until that was done. The issues are
T-SQL: Only allow ORDER BY in the outermost query. DRAW line encoded it in a nested query. This was fixed not by changing the dialect but by changing the stat to hoist the order by up
MySQL: Only allows a single reference to a temp table per query. Boxplot et al violated that in its quantile computations. This turned out to be unfixable in the query generation and instead MySQL now opts out of temp table generation (together with MS SQL and Oracle). As the list have now grown, I've added a cleanup step to these so that, unless the process errors out mid-execution, we don't leave tables hanging.