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 query generation #33

Merged
merged 1 commit into from
May 24, 2024

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented May 20, 2024

fixes #13, #22

@WaVEV WaVEV marked this pull request as draft May 20, 2024 14:35
@WaVEV WaVEV requested review from Jibola and timgraham May 20, 2024 14:35
django_mongodb/__init__.py Outdated Show resolved Hide resolved
django_mongodb/_helpers.py Outdated Show resolved Hide resolved
django_mongodb/compiler.py Outdated Show resolved Hide resolved
django_mongodb/lookups.py Outdated Show resolved Hide resolved
django_mongodb/lookups.py Outdated Show resolved Hide resolved
@WaVEV WaVEV force-pushed the lookup-nodes-supports-mql branch from f62c157 to 74a3dfa Compare May 22, 2024 02:49
@WaVEV WaVEV marked this pull request as ready for review May 22, 2024 02:49
@timgraham timgraham changed the title Lookup nodes supports mql refactor query generation May 22, 2024
Copy link
Collaborator

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

Left open comments, but everything looks good so far!

@@ -139,209 +95,56 @@ def get_cursor(self):
cursor.limit(int(self.query.high_mark - self.query.low_mark))
return cursor

def add_filters(self, filters, query=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the add_filters method anymore. Is it no longer needed? Was this the function that got originally called on the WhereNode instruction?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, see django_mongodb/compiler.py old line 139.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is not longer needed, We have changed it using the as_mql method and it recursive structure. So, add filters was replaced by as_mql method of the WhereNode. This way to compute the filter is more Django like taking advantage of the tree structure.

Comment on lines +18 to +19
if lookup_name not in ("in", "range"):
value = value[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

All values come through as List[Any] types?

Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment two lines above. This is copied from the old _normalize_lookup_value().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, yeah, the thing is:
We use a Django method to normalize things, those methods returns: (sql_string, [parameters]) And we only need the parameters, and there is an especial case with in and range, that [parameters] must keeps as an array.
Django return the value in the way that sql_string % params works. So we need to unfold this structure.

@timgraham timgraham force-pushed the lookup-nodes-supports-mql branch 2 times, most recently from 854317a to 7c833f4 Compare May 24, 2024 16:27
@timgraham timgraham force-pushed the lookup-nodes-supports-mql branch from 7c833f4 to 3789b9f Compare May 24, 2024 16:28
Copy link
Collaborator

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

Great work!

@timgraham timgraham requested a review from Jibola May 24, 2024 16:38
@timgraham timgraham merged commit 3789b9f into mongodb-labs:main May 24, 2024
3 checks passed
@WaVEV WaVEV deleted the lookup-nodes-supports-mql branch August 26, 2024 15:43
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.

refactor MongoQuery.add_filters()
3 participants