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

filter and sort by field of related collection #72

Merged
merged 24 commits into from
Jun 4, 2024

Conversation

hallettj
Copy link
Collaborator

@hallettj hallettj commented May 30, 2024

Filter and sort by fields of a related collection.

This involves a change where relationship references can be "unified". We might have multiple references to the same relationship - for example one that selects fields, another that filters. When these are registered they are unified if they have the same key in the query request collection_relationships map, and they don't have incompatibilities such as differing predicates or offsets. Unifying involves merging field/column selections so that we can get necessary data with a single $lookup.

Previously we blindly copied documents from relationship $lookup pipelines to top-level row sets. But unification means that relationship pipelines may produce fields that were not requested by query request fields or aggregates. This change updates $replaceWith stages to prune data coming from $lookup pipelines to select only requested data.

The changes here are mostly to mongodb-specific code, with only small adjustments to the database-agnostic logic in ndc-query-plan.

Tickets:

@hallettj hallettj self-assigned this May 30, 2024
@hallettj
Copy link
Collaborator Author

I tried testing the the latest engine version, and found that the integration tests I had didn't pass because the new engine version uses Exists in cases where the old version used BinaryComparisonOperator. I pushed more commits to update the engine version the test config uses, and I fixed up the connector's handling of Exists expressions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this module might be a good candidate for some property tests.

Copy link
Contributor

@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, but please consider adding some property tests or unit tests for unify_relationship_references.rs.

Copy link
Collaborator

@codedmart codedmart left a comment

Choose a reason for hiding this comment

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

👍

@hallettj
Copy link
Collaborator Author

hallettj commented Jun 4, 2024

Looks good, but please consider adding some property tests or unit tests for unify_relationship_references.rs.

You're right, I added some tests

@hallettj hallettj merged commit bab5c93 into main Jun 4, 2024
1 check passed
@hallettj hallettj deleted the jesse/filter-by-field-of-relation branch June 4, 2024 22:10
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.

3 participants