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

Refs #33482 -- Fixed QuerySet selectin and filtering againts negated Exists() with empty queryset. #15407

Merged
merged 1 commit into from Feb 7, 2022

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Feb 7, 2022

Regression in b7d1da5.

Comment on lines 1225 to 1229
return (
"1=1"
if not features.supports_boolean_expr_in_select_clause
else "",
(),
Copy link
Member

@charettes charettes Feb 7, 2022

Choose a reason for hiding this comment

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

Strange, I thought that sql='' translated to an elided expression.

if sql:
result.append(sql)
result_params.extend(params)

Here's the query I get from expressions.tests.ExistsTests.test_negated_empty_exists from non-Oracle backends

SELECT "expressions_manager"."id", "expressions_manager"."name" FROM "expressions_manager" WHERE "expressions_manager"."id" = 1

Do we need to adapt select_format to special case sql = ""? Probably, but I didn't think that select_format would be involved in this kind of query?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but we have a different select_format on Oracle: "CASE WHEN {} THEN 1 ELSE 0 END".format(sql), so an empty string is translated to CASE WHEN THEN 1 ELSE 0 END.

Copy link
Member

@charettes charettes Feb 7, 2022

Choose a reason for hiding this comment

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

I think we should adapt select_format in this case to handle '' to encapsulate the Oracle logic in one location.

It's not clear why select_format is called here though as we're directly filtering against the expression and not selecting it?

qs = Manager.objects.filter(
~Exists(Manager.objects.none()) & Q(pk=manager.pk)
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, sorry, it's not about select_format() 🤦 Exists() is wrapped with Case() also in the WHERE clause:

for expr in (self.lhs, self.rhs):
if connection.ops.conditional_expression_supported_in_where_clause(expr):
expr = Case(When(expr, then=True), default=False)
wrapped = True
exprs.append(expr)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see, ok let's go with this approach for now. There's likely a good way to reconcile the multiple boolean expression handling we have for Oracle across the ORM but this simple change should go a long way.

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

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

Looking back at ticket-33482 I think that a better way forward would be to have it drop its custom support for negation and have __invert__ simply return ~Q(self) as the latter already has all the proper machinery to deal with negation.

This adjustment should do for now though!

@felixxm
Copy link
Member Author

felixxm commented Feb 7, 2022

@charettes Thanks for checking 👍 I added an extra test for selecting negated Exists().

@felixxm
Copy link
Member Author

felixxm commented Feb 7, 2022

buildbot, test on oracle.

@felixxm felixxm changed the title Refs #33482 -- Fixed QuerySet filtering againts negated Exists() with empty queryset on Oracle. Refs #33482 -- Fixed QuerySet selectin and filtering againts negated Exists() with empty queryset. Feb 7, 2022
@felixxm
Copy link
Member Author

felixxm commented Feb 7, 2022

@charettes It seems that I found a regression in selecting negated Exists() with empty querysets as well 😄

@charettes
Copy link
Member

Ah I assume something along SELECT as empty_exists_negated FROM table 😅 ?

@felixxm
Copy link
Member Author

felixxm commented Feb 7, 2022

Ah I assume something along SELECT as empty_exists_negated FROM table sweat_smile ?

Yes :)

@felixxm felixxm merged commit 6f185a5 into django:main Feb 7, 2022
@felixxm felixxm deleted the refs-33482-oracle branch February 7, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants