Propagate types from the v3 schema into the v2 query request #22
Conversation
daniel-chambers
force-pushed
the
daniel/type-propagation
branch
from
967d7e0
to
0e832ae
Compare
| pub fn column_ref( | ||
| column_name: &ColumnSelector, | ||
| column: &ComparisonColumn, | ||
| collection_name: Option<&str>, | ||
| ) -> Result<String, MongoAgentError> { | ||
| if column.path.as_ref().map(|path| !path.is_empty()).unwrap_or(false) { | ||
| return Err(MongoAgentError::NotImplemented("comparisons against root query table columns")) | ||
| } |
There was a problem hiding this comment.
Good thinking! I reopened MDB-6 because I think we can support this, but that's a separate piece of work.
| fn v3_to_v2_query( | ||
| context: &QueryContext, | ||
| collection_relationships: &BTreeMap<String, v3::Relationship>, | ||
| root_collection_object_type: &WithNameRef<schema::ObjectType>, | ||
| query: v3::Query, | ||
| collection_object_type: &WithNameRef<schema::ObjectType>, |
There was a problem hiding this comment.
It could be helpful to move some of these arguments into QueryContext, especially since there are two arguments with the same type. But it's not something I'd hold the PR up for.
There was a problem hiding this comment.
I thought about it but if we did that we'd need to stop the caller from being the one constructing and passing the QueryContext and make it internal use only. Right now, this struct is used for the caller to pass contextual information, not as a way for this module to have convenient access to "global" information.
I'm going to defer doing this, because frankly this whole module will be deleted when we eventually get rid of the v2 compat layer and go v3 native.
| // v2 does not support navigating the same relationship more than once across multiple | ||
| // order by elements and having different predicates used on the same relationship in | ||
| // different order by elements. This appears to be technically supported by NDC. | ||
| return Err(ConversionError::NotImplemented("Relationships used in order by elements cannot contain different predicates when used more than once")) |
There was a problem hiding this comment.
Hmm, yes this seems like it would be a little tricky to fully support
| .transpose()?, | ||
| }) | ||
| } | ||
|
|
||
| fn merge_order_by_relations(rels1: &mut HashMap<String, v2::OrderByRelation>, rels2: HashMap<String, v2::OrderByRelation>) -> Result<(), ConversionError> { |
There was a problem hiding this comment.
Nice improvement - thanks!
| let v3_relationship = lookup_relationship(collection_relationships, &relationship)?; | ||
| let v3_collection = context.find_collection(&v3_relationship.target_collection)?; | ||
| let collection_object_type = context.find_object_type(&v3_collection.r#type)?; | ||
| let in_table = v2::ExistsInTable::RelatedTable { relationship }; |
There was a problem hiding this comment.
Maybe need to capture a predicate here? But that is beyond the scope of propagating types so just something to think about following up on.
There was a problem hiding this comment.
I don't think so. In v3 there's no predicate on the v3::ExistsInCollection::Related, and in v2 additional predicates are already inlined into the overall predicate expression.
It's different in order by because order by isn't a predicate structure, so it needed a place to put additional non-join predicates (ie permissions predicates). Where clause predicates are a predicate structure so the permissions predicates are inlined into the overall predicate expression.
| v2_relations.insert( | ||
| path_element.relationship, | ||
| v2::OrderByRelation { | ||
| r#where: where_expr, | ||
| subrelations, | ||
| } | ||
| ); |
There was a problem hiding this comment.
Ok so you're hoisting the order_by predicates. Good thinking. I forget if we still need to do something similar for comparison targets. I'm not sure if my TODO comment still applies to that case.
There was a problem hiding this comment.
No, with comparison targets in predicates, and predicates from the permissions system (which is what these order by predicates are for) should already been inlined into the predicate body.
crates/mongodb-connector/src/main.rs
Outdated
| #[macro_use] | ||
| extern crate lazy_static; |
There was a problem hiding this comment.
You don't need macro_use anymore. You can import macros in the modules where they are used, in this case with use lazy_static::lazy_static
There was a problem hiding this comment.
Ah nice one. Fixed.
|
@hallettj Thanks for fixing those integration tests. 🙏 |
In v2 queries, the types of columns or scalar values were often inlined into the query. In v3, the types are omitted from the query and the connector is expected to remember the types from the schema stored in its state.
This PR updates the code that maps v3 queries into v2 queries to ensure all types are inlined into the v2 query from the v3 schema.
Some bugs were fixed along the way:
v2::OrderBy.relations) map. They now are.v2::ComparisonColumn.pathproperty, but this property only ever supported the current table and the query root table, and never an arbitrary path of relationships like v3.Also: v3 scalar types were being generated every time they were needed; however, this is static data, so they have been generated once and stored in a lazy static and are now reused everywhere.
Issue ticket number and link
JIRA: MDB-7