Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove v2 types #71

Merged
merged 48 commits into from
May 28, 2024
Merged

refactor: remove v2 types #71

merged 48 commits into from
May 28, 2024

Conversation

hallettj
Copy link
Collaborator

Removes v2 types, and replaces them with a new set of internal types that closely match ndc models. The root of the new types is called QueryPlan instead of QueryRequest. It is a denormalized version of a query request. In particular QuerPlan

  • puts type annotations on fields, variable references, etc.
  • uses a Type type that inlines object type references
  • collects relationship reference details from all fields, filters, and sorts and places relevant details in one map in each sub-query (instead of having one relations map at the QueryRequest level, the new types have a map on each Query value for relations with that query's collection as a source, and that are referenced in that query)
  • collects one map of join details at the QueryPlan level (the analog of QueryRequest) for existence checks on unrelated collections

The changes simplify emitting MongoDB aggregation code, and lower the impedence mismatch to using features of v3 that aren't reflected in v2.

I preserved and updated all the existing tests except one that tests filtering by fields of a nested object of a related collection which is something that worked in v2, but will require an update to the latest ndc-spec to work in v3.

There are some more changes necessary for filtering and sorting by fields of a related collection. We need to adjust the way query request expressions map to MongoDB expressions. I thought it would be best to get this refactor in, and put those changes in a separate PR.

Copy link
Collaborator

@dmoverton dmoverton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. This is a big improvement. Just a few minor suggested changes.

It might be useful to move the ndc-query-plan crate to the SDK at some point so it can be used by other connectors.

hallettj and others added 3 commits May 28, 2024 12:01
@hallettj
Copy link
Collaborator Author

Looks good. This is a big improvement. Just a few minor suggested changes.

It might be useful to move the ndc-query-plan crate to the SDK at some point so it can be used by other connectors.

Thanks! I had the same thought.

@hallettj hallettj merged commit 7877f8e into main May 28, 2024
1 check passed
@hallettj hallettj deleted the jesse/query-plan branch May 28, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants