Skip to content

fix projection of refs in foreign collections #99

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

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented Aug 12, 2024

Handle refs from foreign collections.

@WaVEV WaVEV requested a review from timgraham August 12, 2024 17:22
@WaVEV WaVEV marked this pull request as ready for review August 12, 2024 17:22

# Unwrap Ref (in this part of the pipeline we can't do references over $projected fields).
expr_ = expr
while isinstance(expr_, Ref):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't know if there could be a ref of refs. (it could, but Django creates it?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we've triaged pretty much all of the complicated queries by now, so if there's no test for it, I wouldn't worry about it.

@timgraham
Copy link
Collaborator

timgraham commented Aug 12, 2024

Can any of this be separate commits? It's a best practice not to mix non-behavior change refactors with bug fixes. For example, reclassifying test_ticket_24605 shouldn't be mixed in with other work. Edit: test categorization done in #100.

@@ -34,12 +34,19 @@ def __init__(self, *args, **kwargs):
def _get_group_alias_column(self, expr, annotation_group_idx):
"""Generate a dummy field for use in the ids fields in $group."""
replacement = None
if isinstance(expr, Col):
col = expr

Copy link
Collaborator

Choose a reason for hiding this comment

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

My general rule is not to include blank lines when you have a comment separating a logical section. And if you feel like you need a blank line, add a comment instead.

@WaVEV
Copy link
Collaborator Author

WaVEV commented Aug 13, 2024

Can any of this be separate commits? It's a best practice not to mix non-behavior change refactors with bug fixes. For example, reclassifying test_ticket_24605 shouldn't be mixed in with other work. Edit: test categorization done in #100.

This could be split into two commits or three if the last one is the features, I think. However, the refactor of project method and the fix for the incorrect references must go together.
The other part involves the management of references in the group by. All the code is related to the management of references in group by, project and the other stages, that implies changes in the flow of references. So, they are coupled enough to be in one PR, but we can split them into two commits.
EDIT: I decided to put only the changes that fix the expected failure test, the other changes belongs to aggregate_regress test suite.

@WaVEV WaVEV force-pushed the refactor-projection-and-expressions branch from e9b7484 to 243934f Compare August 13, 2024 04:35
@WaVEV WaVEV changed the title Refactor projection and expressions. Handle refs from foreign collections.. Aug 13, 2024
@WaVEV WaVEV changed the title Handle refs from foreign collections.. Handle refs from foreign collections. Aug 13, 2024
@timgraham
Copy link
Collaborator

The --debug-sql output went from:

{'$project': {'_id': 1, 'isbn': 1, 'name': 1, 'pages': 1, 'rating': 1, 'price': 1, 'contact_id': 1, 'publisher_id': 1, 'pubdate': 1, 'store_name': '$name', 'annotations_store_books': 1, 'annotations_store': 1}}

to

{'$project': defaultdict(<CLASS 'dict'>, {'annotations_store': {'store_name': '$annotations_store.name'}, '_id': '$_id', 'isbn': '$isbn', 'name': '$name', 'pages': '$pages', 'rating': '$rating', 'price': '$price', 'contact_id': '$contact_id', 'publisher_id': '$publisher_id', 'pubdate': '$pubdate'})}.

  1. Can we cast the defaultdict back to dict so it doesn't appear in the output like that? (The same issue affects $addFields.)

  2. Is it worth leaving column references as 1 for brevity/simplicity or would this add too much complication to the code?

@WaVEV
Copy link
Collaborator Author

WaVEV commented Aug 13, 2024

The --debug-sql output went from:

{'$project': {'_id': 1, 'isbn': 1, 'name': 1, 'pages': 1, 'rating': 1, 'price': 1, 'contact_id': 1, 'publisher_id': 1, 'pubdate': 1, 'store_name': '$name', 'annotations_store_books': 1, 'annotations_store': 1}}

to

{'$project': defaultdict(<CLASS 'dict'>, {'annotations_store': {'store_name': '$annotations_store.name'}, '_id': '$_id', 'isbn': '$isbn', 'name': '$name', 'pages': '$pages', 'rating': '$rating', 'price': '$price', 'contact_id': '$contact_id', 'publisher_id': '$publisher_id', 'pubdate': '$pubdate'})}.

  1. Can we cast the defaultdict back to dict so it doesn't appear in the output like that? (The same issue affects $addFields.)
  2. Is it worth leaving column references as 1 for brevity/simplicity or would this add too much complication to the code?
  1. Yes, will do.
  2. Also yes, we can just project if the name and the column's name are the same. I think it won't be so complicated.

@WaVEV WaVEV force-pushed the refactor-projection-and-expressions branch from 76b6b4e to 593917e Compare August 14, 2024 00:47
@WaVEV WaVEV requested a review from timgraham August 14, 2024 04:09
@timgraham timgraham force-pushed the refactor-projection-and-expressions branch from 593917e to d9488a1 Compare August 14, 2024 14:41
@timgraham timgraham changed the title Handle refs from foreign collections. fix projection of refs in foreign collections Aug 14, 2024
@WaVEV WaVEV merged commit bdd2a40 into mongodb:main Aug 14, 2024
3 checks passed
@WaVEV WaVEV deleted the refactor-projection-and-expressions branch August 14, 2024 15:49
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.

2 participants