fix: omit cross-shard aggregate helpers on single-shard queries - #1474
fix: omit cross-shard aggregate helpers on single-shard queries#1474murex971 wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| unreachable!("statement was checked to be SELECT"); | ||
| }; | ||
| self.rewrite_aggregates(&mut select, mem, &mut plan, self.db_schema)?; | ||
| if !plan.aggregates.is_noop() { |
There was a problem hiding this comment.
I think it would cool to have a rollback_aggregates function (should be pretty easy to remove the helper columns we added) and only then call deparse. That way, queries without aggregates bypass the deparse step and remain fast!
| offset, | ||
| direct_stmt, | ||
| } => { | ||
| if request.is_executable() |
There was a problem hiding this comment.
I wouldn't mind moving this to apply_after_parser (maybe its own function), to keep this code clean. The apply_after_parser also has better context on how to rewrite requests correctly (since apply is adjacent too), so there is more opportunity for code re-use.
|
I think just a couple of small changes needed. Longer term we would want to not run the aggregate rewriter at all if the query is direct to shard, but that requires a bigger refactor (parse before rewrite, not rewrite before parse like we do now). |
|
yeah, ideal architecture would be first decide query route then rewrite, but it would have been a bigger refactor |
|
👀 |
2 participants
Summary
AVG/STDDEV/VARIANCErewrites add__pgdog_*helper columns before routing. When the query later hits a single shard, those helpers were still sent to Postgres and shown into the client result.direct_stmtand restore it after routing when the route is not cross-shard (simple Query and Parse/prepared statements). Cross-shard queries still use the helper rewrite.stddev()visible in client response #1425