Auto-generated SQL instead of hand-written comments#10061
Auto-generated SQL instead of hand-written comments#10061
Conversation
| /// (SELECT | ||
| /// state | ||
| /// FROM | ||
| /// instance | ||
| /// WHERE | ||
| /// id = <instance_id> AND | ||
| /// time_deleted IS NULL | ||
| /// ), |
There was a problem hiding this comment.
Fun fact, this comment was already out-of-sync with the real query.
In reality, this is:
SELECT
CASE WHEN active_propolis_id IS NULL THEN state ELSE $1 END
FROM
instance
WHERE
id = $2 AND time_deleted IS NULLWhich is a subtle-but-important difference, but the sorta thing you might care about if you're trying to see how the query works.
|
Love this |
| /// kind = <kind> AND | ||
| /// time_deleted IS NULL | ||
| /// ) <= 1, | ||
| /// '<interface_id>', |
There was a problem hiding this comment.
Hah, another fun fact - this was also out of date, except that #10059 (comment) changes it to be not-wrong-anymore. 😅 (Irrelevant since it's going away.)
| /// -- record, have overlapping IP blocks of either family. | ||
| /// overlap AS MATERIALIZED ( | ||
| /// SELECT | ||
| /// -- NOTE: This cast always fails, we just use _how_ it fails to |
There was a problem hiding this comment.
If this comment doesn't already exist elsewhere, might be worth moving it to a Rust comment where the SQL is being generated.
(Same issue on the other block comments that document inner parts of the CTEs.)
| /// <new_target_time_made_target> | ||
| /// FROM new_target | ||
| /// ``` | ||
| /// query we generate. |
There was a problem hiding this comment.
What do you think about putting in the filename where you can find the snapshot?
|
looks like |
3 participants
Inspired by #10050
This PR adds expectorate tests, and removes "hand-written SQL comments", because they can easily
get out-of-sync with reality.
Removed "giant SQL block comment" for:
DeleteQuery: Already has expectorate testinsert_target_query: Already has expectorate testlink_ip_pool_to_external_silo_query: Already has expectorate testFilterOverlappingIpRanges- new expectorate test addedInsertVpcSubnetQuery- new expectorate test added