[SERVER-55219] Add a test to detect any changes in explain format Created: 16/Mar/21 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Querying |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Anton Korshunov | Assignee: | Backlog - Query Optimization |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||
| Assigned Teams: |
Query Optimization
|
||||
| Sprint: | Query Execution 2021-04-05, Query Execution 2021-05-31, Query Execution 2021-06-14, Query Execution 2021-06-28 | ||||
| Participants: | |||||
| Description |
|
Dev Tools wants to be notified about all changes in the explain format. To make sure none of such changes get pass unnoticed, they suggested to create a test which would validate explain output for various query plans. The idea is to cover all possible plans that we can generate. I'm not sure whether it would be feasible at all to come up with such a test, and then support it. This idea also is not quite aligned with our approach to testing explain, since we're validating only specific parts of the output for certain type of queries, rather than checking the entire output. We need to investigate and decide whether such a test would be beneficial. |
| Comments |
| Comment by Kyle Suarez [ 07/Apr/21 ] |
|
Flagging this for team discussion in QE Needs Triage as Anton missed the last two meetings. |
| Comment by Anton Korshunov [ 31/Mar/21 ] |
|
anna.henningsen chuck.kalmanek Thanks for the feedback! I'm flagging it for re-triage by Query Execution. |
| Comment by Anna Henningsen [ 30/Mar/21 ] |
|
Yeah, that’s a fair point, this ticket was originally opened inspired by the SBE changes to the executionStats explain format, but obviously we also rely on fields from the queryPlanner section. Unfortunately, access to these properties is spread out quite a bit over the relevant parts of Compass, so I can’t guarantee for completeness of this list:
|
| Comment by David Storch [ 29/Mar/21 ] |
|
Thanks anna.henningsen, very helpful! Can you clarify what you mean by the following?
I interpret this to mean that the format of the queryPlanner section as a whole, and all its nitty-gritty details, must remain stable. However, explain output in general will change as the internals of the system evolves, since the output is a reflection of the internally implementation details of the query engine. For example, if we overhaul the query optimizer (something we might very well do in the future), then the format of the queryPlanner section is likely to radically change. |
| Comment by Anna Henningsen [ 29/Mar/21 ] |
|
Sure, writing down what exactly we care about is easy:
All that being said – if I were to write a test for this, I’d probably write a (partial?) JSON schema and validate a number of explain plans against it as they are coming from the server. If that’s feasible here, I’d say it’s worth it, if not, that’s also okay as long as communication happens in cases of explain plan changes. |
| Comment by Charles Kalmanek [ 25/Mar/21 ] |
|
When we first discussed the changes to the explain plan, it seemed that there might be large changes in the future that could break Compass visual explain plan. During the meeting we had earlier this month, the consensus from the Query team seemed to be that the fields Compass is using are very likely to be stable into the distant future. I'm not thinking that this ticket isn't really needed. anna.henningsen Is this ok with you? Do you want to list the fields Compass cares about (or add a link to the code) here for documentation? |
| Comment by Anton Korshunov [ 25/Mar/21 ] |
|
chuck.kalmanek Before taking any actions the Query team would like to learn more about the requirements behind this request. Was the idea to generate and test explain output for all possible plans, or for a small subset of queries. Apart from catching changes in the explain format, in order to notify downstream teams, was there any other reason behind this proposal? |