Replace RootAndCurrentTables with TableScope, which keeps track of any tables in scope for an exists, instead of only root and current.#674
Conversation
…y tables in scope for an exists, instead of only root and current.
…ssing PathElements, since they cannot access additional tables using named scopes anyways
| pub struct RootAndCurrentTables { | ||
| /// The root (top-most) table in the query. | ||
| pub root_table: TableSourceAndReference, | ||
| pub struct TableScope { |
There was a problem hiding this comment.
Replace RootAndCurrentTables with TableScope
This struct will keep track of the current table and any tables in scope.
We also make the current_table property private so we can control when and how it is accessed.
This also helped with making sure we look at all the places this property is accessed.
Open to making that property public if there's opposition to this particular part of the change.
tables_in_scope should remain private to make sure scope errors are handled, and that there are no mistakes in accessing scoped tables.
| } | ||
| } | ||
| /// Get the table source and reference for the current table | ||
| pub fn current_table(&self) -> &TableSourceAndReference { |
There was a problem hiding this comment.
gets us the current table. We use this anywhere the current_table property was accessed before. This method cannot fail.
| } | ||
| /// Create a scope from an exists expression or path. The ancestor tables up until the closest query will stay in scope | ||
| #[must_use] | ||
| pub fn new_from_scope(&self, current_table: TableSourceAndReference) -> Self { |
There was a problem hiding this comment.
Creates a scope for an exists expression, while keeping the previous table in scope.
There was a problem hiding this comment.
A lot of this feels like you're re-implementing nonempty::NonEmpty<TableSourceAndReference> from https://docs.rs/nonempty/latest/nonempty/
|
|
||
| impl TableScope { | ||
| /// Create a scope from a Query. There will be no tables available through scopes | ||
| pub fn new(current_table: TableSourceAndReference) -> Self { |
There was a problem hiding this comment.
New scopes should be created for Query and PathElement nodes.
| root_table: table_name_and_reference.clone(), | ||
| current_table: table_name_and_reference, | ||
| }, | ||
| &helpers::TableScope::new(table_name_and_reference), |
There was a problem hiding this comment.
Replace RootAndCurrentTables with with TableScope
| scope: _, | ||
| scope, | ||
| } => { | ||
| let table_source_and_reference = current_table_scope.scoped_table(scope)?; |
There was a problem hiding this comment.
Apply scope before path
| source: table.source, | ||
| }, | ||
| }; | ||
| let new_current_table_scope = current_table_scope.new_from_scope(table); |
There was a problem hiding this comment.
Create a scope for unrelated exists expression, while keeping previous tables in scope.
| source: table.source, | ||
| }, | ||
| }; | ||
| let new_current_table_scope = current_table_scope.new_from_scope(table.clone()); |
There was a problem hiding this comment.
Create scope for related exists expression, keeping previous table in scope.
| root_table: root_and_current_tables.root_table.clone(), | ||
| current_table: TableSourceAndReference { | ||
| let new_current_table_scope = | ||
| current_table_scope.new_from_scope(TableSourceAndReference { |
There was a problem hiding this comment.
Create a new scope, keeping previous scopes around. This is for Nested Collections, which I think is fine, but admittedly was not a focus of this update.
| @@ -174,25 +174,18 @@ pub fn translate_query_part( | |||
| select: &mut sql::ast::Select, | |||
| ) -> Result<(), Error> { | |||
| // the root table and the current table are the same at this point | |||
There was a problem hiding this comment.
should probably delete this comment since it's not longer relevant
|
Going ahead and merging in the interest of saving time |
BenoitRanque
merged commit 56141d9
into
benoit/eng-362-update-ndc-postgres-to-ndc_models-020
…y tables in scope for an exists, instead of only root and current. (#674) <!-- The PR description should answer 2 (maybe 3) important questions: --> ### What <!-- What is this PR trying to accomplish (and why, if it's not obvious)? --> `ComparisonTarget::RootCollectionColumn` was removed, to be replaced by [named scopes](https://github.com/hasura/ndc-spec/blob/36855ff20dcbd7d129427794aee9746b895390af/rfcs/0015-named-scopes.md). This PR implements the replacement functionality. <!-- Consider: do we need to add a changelog entry? --> ### How <!-- How is it trying to accomplish it (what are the implementation steps)? --> This PR replaces RootAndCurrentTables, with TableScope, a struct that keeps track of the current table and any tables in scope for exists expression. See the accompanying review for details on the code itself.
…y tables in scope for an exists, instead of only root and current. (#674) <!-- The PR description should answer 2 (maybe 3) important questions: --> ### What <!-- What is this PR trying to accomplish (and why, if it's not obvious)? --> `ComparisonTarget::RootCollectionColumn` was removed, to be replaced by [named scopes](https://github.com/hasura/ndc-spec/blob/36855ff20dcbd7d129427794aee9746b895390af/rfcs/0015-named-scopes.md). This PR implements the replacement functionality. <!-- Consider: do we need to add a changelog entry? --> ### How <!-- How is it trying to accomplish it (what are the implementation steps)? --> This PR replaces RootAndCurrentTables, with TableScope, a struct that keeps track of the current table and any tables in scope for exists expression. See the accompanying review for details on the code itself.
2 participants
What
ComparisonTarget::RootCollectionColumnwas removed, to be replaced by named scopes.This PR implements the replacement functionality.
How
This PR replaces RootAndCurrentTables, with TableScope, a struct that keeps track of the current table and any tables in scope for exists expression.
See the accompanying review for details on the code itself.