-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Fixed #30200 -- Add support for using indexes in update() for ArrayFields. #19166
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
base: main
Are you sure you want to change the base?
Conversation
Hi, I was tweaking and testing some ideas and this is what I came up with. I would like to hear from you, if this is heading in right direction. |
3242899
to
9ed970e
Compare
9ed970e
to
11c132f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! I have initial comments. I think we will want more input on whether ArrayField
should implement as_sql
as discussed below.
As you may already know, this will also need a release note in 6.1.txt.
You may be interested to see the related PR for JSONField
(#18758). Because it is function-based, via JSON_SET
, it only has to deal with right-hand sides, thus avoiding touching SQLUpdateCompiler
and allowing assigning expressions targeting paths to model instances.
If we preferred, we could pursue a right-hand-side-only approach with concatenation via ||
, e.g. before || new_value || after
. I'd like to hear more views on this.
def test_update_on_db_value(self): | ||
instance = NullableIntegerArrayModel(field=[1, 2], order=3) | ||
instance.save() | ||
NullableIntegerArrayModel.objects.update(field__0=F("order")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a test where the update expression indexes an array like F("field__0") + 1
or such.
instance.refresh_from_db() | ||
self.assertEqual(instance.field, [1, 2, 3, 4]) | ||
|
||
@unittest.expectedFailure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be waiting for feedback before polishing this, but when returning to this, you can remove these decorators and instead assert that the appropriate python error is raised. (At least one of these is failing at the database layer.)
instance.refresh_from_db() | ||
self.assertEqual(instance.field, [0, 2, 3, 4]) | ||
|
||
def test_update_last(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also test slicing with negative indexes.
IntegerArrayModel.objects.update( | ||
field__0_2=[-1, -2], | ||
field__2_4=[-3, -4], | ||
field__4_6=[-5, -6], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing fails when I provide an array with more than two elements here.
elif isinstance(array_slice, (list, tuple)): | ||
return SliceTransformFactory(*array_slice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will want to observe self.size
somewhere to make it harder to evade size
by passing a slice that creates additional elements.
if hasattr(self.base_field, "from_db_value"): | ||
self.from_db_value = self._from_db_value | ||
super().__init__(**kwargs) | ||
self.allow_lookups_on_update = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can just rely on hasattr(field, "as_sql")
instead of using this new flag.
obj_copy.lookups = [self._prep_array_slice(lookup) for lookup in lookups] | ||
return obj_copy | ||
|
||
def as_sql(self, compiler, connection): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially surprised to see a Field
become a compilable.
Q1.) If it's going to be a compilable, I think it needs to implement get_source_expressions
, see above link.
Q2.) Its compilability only makes sense for updates. Is there a way to reuse the compilables we already have, like IndexTransform
? Then we could have a method here that's a sibling to get_placeholder
like get_transforms_for_update
or similar, and call that instead of as_sql
.
/cc @charettes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simon commented on the previous attempt with some implementation hints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like get_transforms_for_update or similar, and call that instead of as_sql.
The linked review mentions an as_update_sql()
, which sounds good.
def test_error_raised_on_not_supported_range_slice(self): | ||
IntegerArrayModel.objects.update(field__1_1_1="2") | ||
|
||
@unittest.expectedFailure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also test invalid indices like \n
that will be swallowed by int()
.
E.g. this should raise a nice python error instead of failing at the database layer:
IntegerArrayModel.objects.update(**{"field__\n": None})
if isinstance(array_slice, int): | ||
lhs += "[%s]" | ||
params.append(array_slice) | ||
|
||
elif isinstance(array_slice, (list, tuple)): | ||
if array_slice[1] is None: | ||
lhs += "[%s:]" | ||
params.append(array_slice[0]) | ||
else: | ||
lhs += "[%s:%s]" | ||
params.extend(array_slice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following Q2 above, my impression is that IndexTransform
and SliceTransform
already do this; can we delegate to them?
if not isinstance(value, (list, tuple)): | ||
return "%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can do a naked type-check on value
, because we might have a Value
, e.g. let's add a test for
IntegerArrayModel.objects.update(field=Value([4, 5, 6]))
On main I get:
SET "field" = '{4,5,6}'::int2[]::bigint[];
On the PR I get:
SET "field" = '{4,5,6}'::int2[];
Perhaps one of the other test models provides a field where this would truly matter (and easier to assert against). We could add this test coverage in a separate commit.
Trac ticket number
ticket-30200
Branch description
Add support for using indexes in update() for ArrayFields.
Checklist
main
branch.