-
Notifications
You must be signed in to change notification settings - Fork 69
Description
(This is followup from #9570 where we added additional branching queries.)
When we compare a nullable column against a Rust value with the type Option<T>, we often box the query and branch based on its value to either compare with eq or check for is_null; e.g. from main:
omicron/nexus/db-queries/src/db/datastore/bgp.rs
Lines 109 to 122 in 4c9d4a0
| let query = match config.vrf.clone() { | |
| Some(v) => query.filter(dsl::vrf.eq(v)), | |
| None => query.filter(dsl::vrf.is_null()), | |
| }; | |
| let query = match config.shaper.clone() { | |
| Some(v) => query.filter(dsl::shaper.eq(v)), | |
| None => query.filter(dsl::shaper.is_null()), | |
| }; | |
| let query = match config.checker.clone() { | |
| Some(v) => query.filter(dsl::checker.eq(v)), | |
| None => query.filter(dsl::checker.is_null()), | |
| }; |
This is because eq() faithfully reproduces the SQL behavior of some_column = NULL returning false even if the value of some_column is itself NULL.
However, I believe we could replace these branches by using https://docs.rs/diesel/latest/diesel/expression_methods/trait.PgExpressionMethods.html#method.is_not_distinct_from, which behaves more like we'd expect from Option comparisons in Rust:
This behaves identically to the = operator, except that NULL is treated as a normal value.
The value of this is pretty small on examples like the above (although we potentially get to drop the boxing of the query to type-erase it); it becomes more of a win if the branching contains multiple other filters that have to be manually kept in sync. An example from #9570, where these branches are identical except for addr.eq(addr) vs addr.is_null():
omicron/nexus/db-queries/src/db/datastore/bgp.rs
Lines 891 to 910 in 2a5ffb0
| // Query the main peer config table. For unnumbered peers, | |
| // addr is NULL; for numbered peers, addr matches. | |
| let active = if addr.is_some() { | |
| peer_dsl::switch_port_settings_bgp_peer_config | |
| .filter(db_peer::port_settings_id.eq(port_settings_id)) | |
| .filter(db_peer::addr.eq(addr)) | |
| .select(db_peer::allow_export_list_active) | |
| .limit(1) | |
| .first_async::<bool>(&conn) | |
| .await | |
| } else { | |
| peer_dsl::switch_port_settings_bgp_peer_config | |
| .filter(db_peer::port_settings_id.eq(port_settings_id)) | |
| .filter(db_peer::addr.is_null()) | |
| .filter(db_peer::interface_name.eq(interface_name.to_owned())) | |
| .select(db_peer::allow_export_list_active) | |
| .limit(1) | |
| .first_async::<bool>(&conn) | |
| .await | |
| }; |