Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  closed
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:  noop | Triage Stage:
 |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Comment (by Mariusz Felisiak ):

 In [changeset:"a8de04f8db470d1c43f945f28a00fb49b1f2ca80" a8de04f8]:
 {{{#!CommitTicketReference repository=""
 revision="a8de04f8db470d1c43f945f28a00fb49b1f2ca80"
 [5.0.x] Refs #34964 -- Doc'd that Q expression order is preserved.

 Backport of 7714ccfeae969aca52ad46c1d69a13fac4086c08 from main
 }}}
-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018def9c3920-77142833-2a95-4ea2-a487-e7776e8cefc2-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  closed
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:  noop | Triage Stage:
 |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Comment (by Mariusz Felisiak ):

 In [changeset:"7714ccfeae969aca52ad46c1d69a13fac4086c08" 7714ccfe]:
 {{{#!CommitTicketReference repository=""
 revision="7714ccfeae969aca52ad46c1d69a13fac4086c08"
 Refs #34964 -- Doc'd that Q expression order is preserved.
 }}}
-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018def9bbdff-3c5b71a5-3942-4626-9f95-ef65778a9619-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  closed
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:  noop | Triage Stage:
 |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak):

 * status:  assigned => closed
 * resolution:   => wontfix
 * stage:  Accepted => Unreviewed


Comment:

 It looks like changing the order is not an option, at least not for
 `CheckConstraint`s.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bd7822dfb-6263226f-b528-481e-b19d-cef927387c09-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by David Sanders):

 Advice from #postgresql IRC channel is that an expression's operand order
 _does_ matter, but only for check constraints.

 > in most contexts ANDed and ORed terms are sliced-and-diced as the
 planner sees fit. but there is one case that might matter:
 > the check constraint obviously gets evaluated to check inserted rows,
 and I don't believe (but would have to check) that it gets reordered there
 > in which case  CHECK (foo OR slowfunc(bar))  might be faster than  CHECK
 (slowfunc(bar) OR foo)
 >
 > let me check the code. sec.
 > looks like check expressions are not reordered by the planner for
 performance
 > inside queries, lists of ANDed quals evaluated at a single node are
 reordered when possible to put expensive ones last (subject to security
 restrictions)
 > but that doesn't apply to checking constraints on insert/update
 > so I would expect CHECK (foo OR slowfunc(bar))  to be noticably faster
 than CHECK (slowfunc(bar) OR foo) in the case where "foo" is usually true
 >
 > tested it and it looks like i am correct

 Advice courtesy of RhodiumToad 

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bd1548baf-8c70f467-14dd-44c6-a8ea-acbccfc54437-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak):

 I have a gut feeling that it's not worth doing, as there is a risk of
 potential regressions. If you modify `check` of `condition` you should be
 ready to deal with consequences of recreating the index, even if they
 theoretically have the same meaning 路 I'm not strongly against this
 patch, this is just a friendly warning ;)

 > ... with a table rebuild. That's something you might miss.

 I know how migrations work on SQLite ;)

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bccc31af1-7b9bf239-db89-4061-af5d-f5477a3d2d31-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Jacob Walls):

 Thanks for the extra context around those edge cases. I'd love to know
 more about them so I can look for them in my own projects.

 I notice we've been sorting the arguments to the Q constructor since
 #29125. So if someone did need to change the order to work around a
 database edge case, they'd ''want'' a change from `Q(A, B)` to `Q(A) &
 Q(B)` to generate a migration, since if B sorts before A, it can only be
 expressed in Django as the latter, and this PR would take away the
 migration to make it.

 The argument in favor of this change (which is very slight, I know) is
 that refactoring the code for consistency/readability with the surrounding
 constraints might generate a migration with a table rebuild. That's
 something you might miss. In other words, you need to know that it's not
 worth paying that cost when you're editing/reviewing the code. Until I
 knew about these edge cases, I thought that was a potential DX win to
 reduce cognitive burden.

 Finally, I guess if we still wanted this change we might ought to block it
 on a resolution of #25245. Should we Someday/Maybe this ticket, then?

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc9007dd9-d21600f1-671b-4c72-a539-6fa428595e51-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak):

 I have doubts, in my experience database optimizers are sometimes very
 picky and the order of statements may be significant even for commutative
 operators.

 Is it worth changing?

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc8ba79d3-8d2b34c5-df2d-496a-ba78-f4b00f32bb92-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by David Wobrock):

 * cc: David Wobrock (added)


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc4de736a-269f3068-e1c4-497f-bda9-203e8c75fe52-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Jacob Walls):

 Thanks Simon. I just checked more carefully and found that I wasn't able
 to come up with a test case that creates a migration for an existing
 project. (Sorry!)

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc4c6f6a6-51738e1e-76c7-4858-a52c-0cadc77d50e6-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Simon Charette):

 The creation of new migrations could be avoided entirely by performing the
 sorting only during `__eq__`

 {{{#!diff
 diff --git a/django/db/models/constraints.py
 b/django/db/models/constraints.py
 index 56d547e6b0..e221bebd8b 100644
 --- a/django/db/models/constraints.py
 +++ b/django/db/models/constraints.py
 @@ -152,14 +152,22 @@ def __repr__(self):
  )

  def __eq__(self, other):
 -if isinstance(other, CheckConstraint):
 -return (
 -self.name == other.name
 -and self.check == other.check
 -and self.violation_error_code ==
 other.violation_error_code
 -and self.violation_error_message ==
 other.violation_error_message
 -)
 -return super().__eq__(other)
 +if not isinstance(other, CheckConstraint):
 +return super().__eq__(other)
 +# Relax check equality to equivalence comparisons to allow
 +# re-ordering of components.
 +if isinstance(self.check, Q):
 +if not isinstance(other.check, Q) or not
 self.check.equivalent_to(
 +other.check
 +):
 +return False
 +elif self.check != other.check:
 +return False
 +return (
 +self.name == other.name
 +and self.violation_error_code == other.violation_error_code
 +and self.violation_error_message ==
 other.violation_error_message
 +)

  def deconstruct(self):
  path, args, kwargs = super().deconstruct()
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc47bee52-04705b86-8aca-44c7-9043-e481b866e539-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Jacob Walls):

 Simon's suggestion on the PR was to sort Q objects in the constructor of
 `CheckConstraint` so that we wouldn't even get into a situation where the
 autodetector would be remaining quiet even with SQL changes. I see that
 this would come at the cost of causing migrations to be emitted for
 existing projects, though, so we might want a release note.

 The case that originally prompted me to open a ticket was for a migration
 dropping and recreating a constraint with the exact same SQL, e.g.
 changing `Q(A) & Q(B)` to `Q(A, B)` . I just added a regression test for
 this case.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc45cbc40-b2022723-8fb0-405e-b982-616bf6231cd3-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by David Sanders):

 * cc: David Sanders (added)


Comment:

 To extend Simon's concerns; I think this needs more investigation as
 commutative doesn't always mean that the system will behave in an
 equivalent way 樂

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bc258ae9a-4abeecc4-0dec-46a3-86cd-0215969bb0ba-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Simon Charette):

 * stage:  Unreviewed => Accepted


Comment:

 The only thing that slightly worries me is that I've seen Postgres be very
 finicky about the order of components in complex constraint when dealing
 with conditional of functional indices and in these rare cases the order
 matters even they are representing the same condition.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bbf10d29e-fa9b45fa-4dd3-40ea-bce3-84fc4e69a5c9-00%40eu-central-1.amazonses.com.

Re: [Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
 Reporter:  Jacob Walls  |Owner:  Jacob
 Type:   |  Walls
  Cleanup/optimization   |   Status:  assigned
Component:  Migrations   |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:  noop | Triage Stage:
 |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Jacob Walls):

 * has_patch:  0 => 1


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bbf08116c-073a2505-efa7-4b7e-b9f2-af331dc41295-00%40eu-central-1.amazonses.com.

[Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration

[Django]

#34964: Reversing the order of Q objects in a CheckConstraint generates a 
migration
-+-
   Reporter:  Jacob  |  Owner:  Jacob Walls
  Walls  |
   Type: | Status:  assigned
  Cleanup/optimization   |
  Component: |Version:  dev
  Migrations |
   Severity:  Normal |   Keywords:  noop
   Triage Stage: |  Has patch:  0
  Unreviewed |
Needs documentation:  0  |Needs tests:  0
Patch needs improvement:  0  |  Easy pickings:  0
  UI/UX:  0  |
-+-
 Changing `Q(A) & Q(B)` to `Q(B) & Q(A)` (or `Q(B, A)`) generates a
 migration that drops and recreates the exact same constraint.

 Suggesting to build on the work in #34744 to adjust the identity property
 for Q objects to use a deterministic sort, given that `^`, `&`, and `|`
 operations are commutative.

 [https://github.com/django/django/pull/17466 PR]

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018bbf0519ff-4cdd224a-5fda-448e-9d68-bee0a8d7919a-00%40eu-central-1.amazonses.com.