Skip to content

add support for QuerySet.union() #120

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 1 commit into from
Sep 10, 2024
Merged

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented Aug 30, 2024

fixes #72

@WaVEV WaVEV requested a review from timgraham August 30, 2024 04:34
@WaVEV
Copy link
Collaborator Author

WaVEV commented Aug 30, 2024

Once this code is merged, we should change the:
field_name = order.expression.as_mql(self, self.connection).removeprefix("$")
to
field_name = order.as_mql(self, self.connection).removeprefix("$")

@WaVEV WaVEV marked this pull request as ready for review August 30, 2024 21:29
@WaVEV WaVEV force-pushed the support-union branch 3 times, most recently from 5acdf8e to 70c2b20 Compare September 1, 2024 23:11
@timgraham timgraham changed the title Support union add support for QuerySet.union() Sep 3, 2024
if "_id" not in projected_fields:
combinator_pipeline.append({"$unset": "_id"})
else:
raise NotSupportedError(f"Combinator {self.query.combinator} isn't supported.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this already checked by supports_select_{self.query.combinator}?

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, when I wrote this code I had in mind the other combinators. But yes, it is being checked twice.

@@ -166,6 +169,10 @@ def join(self, compiler, connection):
return lookup_pipeline


def orderby(self, compiler, connection):
Copy link
Collaborator

Choose a reason for hiding this comment

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

order_by (follows the pattern of other function names with multiple words)

@@ -391,6 +398,9 @@ def project_field(column):
if hasattr(column, "target"):
# column is a Col.
target = column.target.column
# Handle Order by columns as refs columns.
Copy link
Collaborator

Choose a reason for hiding this comment

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

by -> By

parts.append((compiler_.build_query(columns), compiler_.collection_name))

except EmptyResultSet:
# Omit the empty queryset with UNION and with DIFFERENCE if the
Copy link
Collaborator

Choose a reason for hiding this comment

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

chop "and with DIFFERENCE"

_, exprs = zip(*columns, strict=True)
columns = tuple(zip(main_query_fields, exprs, strict=True))
parts.append((compiler_.build_query(columns), compiler_.collection_name))

Copy link
Collaborator

Choose a reason for hiding this comment

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

chop blank line

@@ -32,6 +32,33 @@ def __init__(self, *args, **kwargs):
# A list of OrderBy objects for this query.
self.order_by_objs = None

def _unfold_column(self, col):
"""
Flattens a field by returning its target or by replacing dots with GROUP_SEPARATOR
Copy link
Collaborator

Choose a reason for hiding this comment

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

PEP 257 verb style: Flatten

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool wow, I didn't know about that.


def _fold_columns(self, unfold_columns):
"""
Converts flat columns into a nested dictionary, grouping fields by table names.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Convert

@@ -512,6 +517,7 @@ def django_test_expected_failures(self):
"delete.tests.DeletionTests.test_only_referenced_fields_selected",
"lookup.tests.LookupTests.test_in_ignore_none",
"lookup.tests.LookupTests.test_textfield_exact_null",
"queries.test_qs_combinators.QuerySetSetOperationTests.test_exists_union",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test seems useful except for the SQL introspection part, so I comment out the unneeded lines in the latest commit on the Django fork.

@WaVEV WaVEV marked this pull request as draft September 5, 2024 04:13
@WaVEV WaVEV force-pushed the support-union branch 2 times, most recently from b68183f to fee311a Compare September 7, 2024 15:49
@WaVEV WaVEV marked this pull request as ready for review September 7, 2024 15:50
@WaVEV
Copy link
Collaborator Author

WaVEV commented Sep 7, 2024

Once this code is merged, we should change the: field_name = order.expression.as_mql(self, self.connection).removeprefix("$") to field_name = order.as_mql(self, self.connection).removeprefix("$")

Done in #123

@timgraham timgraham merged commit df2cddd into mongodb:main Sep 10, 2024
3 checks passed
@WaVEV WaVEV deleted the support-union branch July 2, 2025 02:17
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.

PYTHON-4675: add support for QuerySet.union()
2 participants