Skip to content

fix collisions with projected columns and order by columns #108

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

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented Aug 18, 2024

No description provided.

@WaVEV WaVEV requested a review from timgraham August 18, 2024 18:31
@timgraham timgraham force-pushed the fix-collisions-with-columns-and-order-by-columns branch from 7141e8c to fdc643b Compare August 19, 2024 23:40
@timgraham
Copy link
Collaborator

I made a couple edits on the Django fork (see last commit there), so a few more aggregation_regress tests cases can run. Do you want to check the "To triage:" list under expected failures before merging (rather than adding more details in a follow-up commit)?

@timgraham timgraham force-pushed the fix-collisions-with-columns-and-order-by-columns branch from fdc643b to 1698963 Compare August 20, 2024 00:16
@WaVEV
Copy link
Collaborator Author

WaVEV commented Aug 20, 2024

I made a couple edits on the Django fork (see last commit there), so a few more aggregation_regress tests cases can run. Do you want to check the "To triage:" list under expected failures before merging (rather than adding more details in a follow-up commit)?

yes, what is "the triage"? where can I find it?

"aggregation_regress.tests.AggregationTests.test_annotation",
"aggregation_regress.tests.AggregationTests.test_more_more3",
"aggregation_regress.tests.AggregationTests.test_more_more_more3",
# To triage:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great tests, in the first one we have two issues:

  1. It is related with joins.
  2. the way that we are storing object_ids in Django models. we cannot store a char[24].

Note that : collection.find({'_id': '66c42726db971422470255ed'}) it won't match the thing that have the id ObjectId('66c42726db971422470255ed').

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the second test is fixed, just another collision. Fixed in the last commit and test removed from features.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should open a new issue for the first one. It's out of aggregation's scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. I squashed the third commit with the second since it was actually a regression in that commit.

@WaVEV WaVEV force-pushed the fix-collisions-with-columns-and-order-by-columns branch from 48736c3 to 2621db9 Compare August 21, 2024 03:18
@timgraham timgraham force-pushed the fix-collisions-with-columns-and-order-by-columns branch from 2621db9 to 9dd5f08 Compare August 21, 2024 23:57
@timgraham timgraham changed the title Fix collisions with projected columns and order by columns. fix collisions with projected columns and order by columns Aug 21, 2024
@timgraham timgraham merged commit 9dd5f08 into mongodb:main Aug 22, 2024
3 checks passed
@WaVEV WaVEV deleted the fix-collisions-with-columns-and-order-by-columns branch August 22, 2024 03:25
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