[db-queries] Decouples CTE usage from Diesel (RegionAllocation)

[smklein]

Diesel's complex type have been a pain point in the region allocation CTE in particular:

• It relies on several JOINs, which requires explicitly adding Diesel macros to make the type system happy
• With ongoing work by @jmpesp , it would require additional alias! calls to allow a table to appear in a SELECT clause in multiple spots
• Generally, the disconnect between "what SQL do I want to write" and "what invocations will make Diesel happy" has been "not really worth it" in this space.

This PR does the following:

• It relies heavily on https://docs.diesel.rs/master/diesel/query_builder/struct.SqlQuery.html , which is Diesel's "you do whatever you want" query type. Although this is still using Diesel, this usage is actually pretty aligned with other simpler DB interfaces - other crates (see: tokio_postgres) take arguments like "a String + list of bind parameters", in some form.
• It adds support in raw_query_builder.rs for a wrapper around Diesel's SqlQuery object, to make SQL injections less possible and to track bind parameter counting.
• It fully converts the RegionAllocation CTE to use this builder, interleaved with raw SQL.
• Since a large portion of the CTE was rote "repeated columns", I also added a function, accessible as AllColumnsOf<Table>::with_prefix(&'static str), to enumerate all the columns of a table as strings.
• I also added a simple EXPLAIN test to the CTE, to quickly validate that CockroachDB thinks it's producing valid output.

Here are my thoughts for future improvements:

• I spent a while trying to make the "query construction" a compile-time operation. I think that this is possible with nightly features. I think this is extremely difficult with stable rust. However, I think this is a great direction for future work, as statically-known queries would be easier to cache and validate.
• I'd like to encapsulate more type information about the constructed query, as an "input/output" object. Right now, we're relying on existing integration tests for validation, but it seems possible to send these "example" queries (like the ones I'm using in my EXPLAIN tests) to ask CockroachDB to validate type information for us.
• I want to make this format as digestible as possible. If there's anything I can do to make this easier to read, write, and validate, I'm totally on-board. I have been debating adding macro support for SQL formatting the raw strings, but I'm on the fence about whether or not that would make interleaved code harder to parse by humans.
  • As a follow-up: I'm auto-formatting the output of these queries in the EXPECTORATE-d output files

[david-crespo]

“this string could” reads like more of a warning that “this string won’t”, which could be misinterpreted as a reassurance

[smklein]

Updated to i_take_responsibility_for_validating_this_string - I'm okay wordsmithing this, since IMO it should be long, obvious, and hard to accidentally call.

Also wrapped this in a struct so a caller can't create the enum. I always forget that all enum variants are always accessible for construction.

[smklein]

Weirdly this PR seems to be causing failures on the integration tests, but not on the more "targeted to the query" tests in nexus-db-queries. I need to investigate further to see what's happening.

As a side-note: It's been kinda useful to use cockroachdb sqlfmt on the output of diesel::debug_query, emitted via expectorate. I haven't wired up the auto-formatting yet, but that seems useful to easily compare diffs of complicated queries.

[smklein]

I think I found the underlying issue breaking the tests. https://docs.rs/diesel/latest/diesel/dsl/fn.sql_query.html relies on Diesel's QueryableByName, which "gets the results wrong" when parsing the output of the query. AFAICT, the query itself is acting correctly, we're just parsing the output badly.

Basically: It returns a (SELECT dataset) UNION (SELECT region). If both dataset and region contain a field named id (they do), then we pick the "first column named id", which leads to obvious problems (e.g., we never read the region's id correctly).

I really wish there was a way to have columns queried by Diesel returned to a struct in the order they were parsed. I don't know why sql_query is imposing this constraint on us.

[hawkw]

this is wonderful :)

[smklein]

I hope it's the right direction, and I'd be stoked to move things more and more into "const fn"s as we can.

I think the choices made by Diesel (e.g. https://docs.diesel.rs/master/diesel/query_builder/struct.SqlQuery.html#method.sql, which just takes anything that impls AsRef<str> ) makes this a little too easy to foot-gun via SQL injection when building queries that side-step the norms of Diesel's type system.

I know it's possible to subvert this with &'static str too, and I wish there was a way to more explicitly say "compile-time-constructed string", but that's as close a proxy as I think I can get right now.

[smklein]

This means that you cannot deserialize this query into a tuple, and any structs used must implement QueryableByName.

so_that_was_a_lie.jpeg

Turns out you can use the newtype pattern, re-implement some Diesel traits (https://docs.diesel.rs/master/diesel/prelude/trait.RunQueryDsl.html to indicate that it's executable, https://docs.diesel.rs/master/diesel/query_builder/trait.Query.html , with specific SQL type to say "what the output should be deserialized from", and https://docs.diesel.rs/master/diesel/query_builder/trait.QueryFragment.html to plumb through the underlying SqlQuery so we can execute the raw SQL we constructed)

This seems to work. I'm using a new type named TypedSqlQuery that should be generally available for this purpose, of exposing a "strongly typed SQL fragment" on top of a "not-very-strongly typed construction".

[jmpesp]

My two cents: I really hope I'm not causing us to move away from what Diesel gives us. The entire time I had where things wouldn't compile was due to me missing a join on a table that I later used in a WHERE filter: this was a legit error that diesel was flagging. Once that was cleared up, everything worked.

I much prefer working with the older CTE, though I do love the other things this PR adds.

Generally, the disconnect between "what SQL do I want to write" and "what invocations will make Diesel happy" has been "not really worth it" in this space.

Even though I had a frustrating time trying to figure out what the problem was, I'd still argue that Diesel gives us more than just raw SQL. It's especially useful if we need to refactor or reshape things so we can share code, etc.

[smklein]

My two cents: I really hope I'm not causing us to move away from what Diesel gives us. The entire time I had where things wouldn't compile was due to me missing a join on a table that I later used in a WHERE filter: this was a legit error that diesel was flagging. Once that was cleared up, everything worked.

This is definitely not just you -- I can clarify the PR description? But I have heard from many people interfacing with CTEs and Diesel and struggling (@bnaecker , @jgallagher , @internet-diglett , and more) because the workflow is "figure out what SQL to write", and then "figure out how to convince Diesel to accept it". If translating SQL to Diesel bought us guarantees that we could not get any other way, I'd be more convinced, but for more complex queries like this, I think we can get sufficient protection -- and a flatter structure -- by using something much closer to raw SQL.

Even though I had a frustrating time trying to figure out what the problem was, I'd still argue that Diesel gives us more than just raw SQL. It's especially useful if we need to refactor or reshape things so we can share code, etc.

To expand a little bit on the value-proposition: When I originally added the CTE utilities, I had the same hopes. I wanted to use Diesel's type-safety guarantees to help us ensure things like:

• Are you only selecting columns from tables that exist in your FROM clauses?
• Similarly, when selecting columns from subqueries, do those types and names match?
• Do the types of your columns used as bind parameters match the underlying types within the SQL query itself?
• Is the SQL being constructed "valid"?

I still think these are all valid protections, that we should ensure! I also think that the EXPLAIN-based tests, in addition to very simple integration tests, provide protection against all these things. Furthermore, they do so with fewer types, without any need for aliasing, without any need for JOIN-based macros, and immediately with support for arbitrary Postgres / CockroachDB support that may or may not be represented in Diesel.

[jmpesp]

Looks good to me!