On conflict clause / upserts #816

dantownsend · 2023-04-28T09:03:17Z

Resolves #252

Based on #798 by @sinisaos with the following changes:

We're not supporting older SQLite versions, to keep things simpler.
Gives a bit more granular control over which constraints are used, and which values to update.

Remaining tasks:

Update docs
Add more tests
Raise an exception for older SQLite versions

codecov-commenter · 2023-04-28T09:15:47Z

Codecov Report

Merging #816 (8c4a99c) into master (9841730) will decrease coverage by 13.35%.
The diff coverage is 86.73%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           master     #816       +/-   ##
===========================================
- Coverage   91.36%   78.02%   -13.35%     
===========================================
  Files         108      108               
  Lines        7552     7644       +92     
===========================================
- Hits         6900     5964      -936     
- Misses        652     1680     +1028

Impacted Files	Coverage Δ
piccolo/apps/asgi/commands/new.py	`73.43% <ø> (ø)`
piccolo/query/methods/objects.py	`95.30% <0.00%> (-2.68%)`	⬇️
piccolo/query/methods/select.py	`91.83% <50.00%> (-6.13%)`	⬇️
piccolo/query/mixins.py	`87.65% <86.25%> (-5.61%)`	⬇️
piccolo/query/methods/insert.py	`97.87% <100.00%> (+0.65%)`	⬆️

... and 36 files with indirect coverage changes

tests/table/test_insert.py

piccolo/query/mixins.py

sinisaos · 2023-04-28T13:23:48Z

@dantownsend What is the syntax for updating multiple on_conflict columns? I tried a list of tuples like this

values=[(Band.name, "Javas"),(Band.popularity, 1000)]

but it doesn't work. How do we write test like this https://github.com/piccolo-orm/piccolo/pull/798/files#diff-fac5af1d6e85e27669c58e982989dfcf8327f312f8b29a272a1188f86ecb7f39R126-R127 or that's not possible at this time?

dantownsend · 2023-04-28T13:28:46Z

@dantownsend What is the syntax for updating multiple on_conflict columns? I tried a list of tuples like this

@sinisaos My thinking was that the values list can either take a column, or a tuple. If it's a column, it uses the new value. If it's a tuple, it uses the value specified in the tuple.

# If there's a conflict, it sets the popularity to 2000:
await Band.insert(
    Band(name='Pythonistas', popularity=2000)
).on_conflict(
    action='DO UPDATE',
    target=[Band.name],
    values=[Band.popularity]
)

# If there's a conflict, it sets the popularity to something custom:
await Band.insert(
    Band(name='Pythonistas', popularity=2000)
).on_conflict(
    action='DO UPDATE',
    target=[Band.name],
    values=[(Band.popularity, 3000)]
)

I haven't had time to fully test this though - so there may be bugs.

sinisaos · 2023-04-28T14:19:45Z

@dantownsend I understand it for one column but how to do it for multiple columns. With the old API we didn't have to set any columns or values because Piccolo did everything for us. For example if we have rows like this

[
    {'id': 1, 'name': 'Band 1', 'popularity': 1}, 
    {'id': 2, 'name': 'Band 2', 'popularity': 2}, 
    {'id': 3, 'name': 'Band 3', 'popularity': 3}
]

we can set one or more columns to update on_conflict like this for one column (e.g Band.popularity change and Band.name stay the same)

await Band.insert(
    Band(id=1, name="Band 1", popularity=111),
    Band(id=2, name="Band 2", popularity=222),
    Band(id=3, name="Band 3", popularity=333),
    on_conflict=OnConflict.do_update,
).run()
# result
[
    {'id': 1, 'name': 'Band 1', 'popularity': 111}, 
    {'id': 2, 'name': 'Band 2', 'popularity': 222}, 
    {'id': 3, 'name': 'Band 3', 'popularity': 333}
]

or for multiple column (Band.popularity and Band.name both changes)

await Band.insert(
    Band(id=1, name="Band 1111", popularity=1111),
    Band(id=2, name="Band 2222", popularity=2222),
    Band(id=3, name="Band 3333", popularity=3333),
    on_conflict=OnConflict.do_update,
).run()
# result
[
    {'id': 1, 'name': 'Band 1111', 'popularity': 1111}, 
    {'id': 2, 'name': 'Band 2222', 'popularity': 2222}, 
    {'id': 3, 'name': 'Band 3333', 'popularity': 3333}
]

How can we do this with the new API, and if not implemented, please do. Sorry for long comment.

dantownsend · 2023-04-28T14:31:12Z

Should be something like this:

await Band.insert(
    Band(id=1, name="Band 1111", popularity=1111),
    Band(id=2, name="Band 2222", popularity=2222),
    Band(id=3, name="Band 3333", popularity=3333)
).on_conflict(
    action='DO UPDATE',
    target=[Band.id],
    values=[Band.name, Band.popularity]
)

If there are loads of columns we pass Band.all_columns() to target, or make that the default behaviour if the user doesn't specify values.

sinisaos · 2023-04-28T15:10:27Z

I'm sorry, but that doesn't work. Both Sqlite and Postgres throw a syntax error (sqlite3.OperationalError: near ""popularity"": syntax error and asyncpg.exceptions.PostgresSyntaxError: syntax error at or near ""popularity""). If we set only one column like this values=[Band.name] everything works. I hope this helps you find what the problem is.

dantownsend · 2023-04-28T15:14:21Z

I'm sorry, but that doesn't work. Both Sqlite and Postgres throw a syntax error (sqlite3.OperationalError: near ""popularity"": syntax error and asyncpg.exceptions.PostgresSyntaxError: syntax error at or near ""popularity""). If we set only one column like this values=[Band.name] everything works. I hope this helps you find what the problem is.

Thanks - sounds like a bug. I'll see what's going on.

sinisaos · 2023-04-28T15:30:40Z

@dantownsend Yes. Missing comma between EXCLUDED columns after UPDATE SET in querystring. This is log
INSERT INTO "band" ("id","name","popularity") VALUES (1,'Band 1',1),(2,'Band 2',2),(3,'Band 3',3) ON CONFLICT ("id") DO UPDATE SET "name"=EXCLUDED."name" "popularity"=EXCLUDED."popularity"

dantownsend · 2023-04-28T16:03:42Z

@sinisaos You're right - that's the problem. I'll refactor it later.

piccolo/query/mixins.py

Co-authored-by: sinisaos <[email protected]>

…o into 252-on-conflict-clause

sinisaos · 2023-04-28T17:03:02Z

@dantownsend I'm sorry if I caused problems because I never used Github suggested changes, but it seemed to me that it was easiest to show what changes I think should be made.

dantownsend · 2023-04-28T17:03:57Z

@dantownsend I'm sorry if I caused problems because I never used Github suggested changes, but it seemed to me that it was easiest to show what changes I think should be made.

It's good - works well. Thanks for suggesting those changes 👍

dantownsend · 2023-04-28T22:14:41Z

I've added some docs. Just need to decide on the names of the arguments.

It's currently:

>>> await Band.insert(
...     Band(name="Pythonistas", popularity=1200)
... ).on_conflict(
...     action="DO UPDATE",
...     target=[Band.name],
...     values=[Band.popularity],
... )

The names action, target and values were inspired by the Postgres docs, but I'm wondering if this is better?

>>> await Band.insert(
...     Band(name="Pythonistas", popularity=1200)
... ).on_conflict(
...     action="DO UPDATE",
...     constraints=[Band.name],
...     update=[Band.popularity],
... )

dantownsend · 2023-04-30T22:11:44Z

@sinisaos I think it's pretty much done now 🤞

The only thing I know of missing is we're letting the user specify the target using a string. Postgres lets you specify the target using the name of a column, or an index expression:

https://www.postgresql.org/docs/current/sql-insert.html#index_expression

I need to read a bit more about how the index expression is meant to work.

sinisaos · 2023-05-01T05:56:52Z

tests/table/test_select.py

@@ -1364,12 +1364,12 @@ def test_distinct_on_sqlite(self):
        SQLite doesn't support ``DISTINCT ON``, so a ``ValueError`` should be


I think it should be NotImplementedError instead ValueError.

sinisaos · 2023-05-01T06:07:52Z

This works and looks great. Can I suggest two more tests that will cover tuple value as columns and tuple value as columns name in values argument.

def test_do_update_tuple_values(self):
    """
    We can use tuple values in ``values``.
    """
    Band = self.Band

    new_popularity = self.band.popularity + 1000
    new_name = "Rustaceans"

    Band.insert(
        Band(
            id=self.band.id,
            name=new_name,
            popularity=new_popularity,
        )
    ).on_conflict(
        action="DO UPDATE",
        targets=[Band.id],
        values=[
            (Band.name, new_name),
            (Band.popularity, new_popularity + 2000),
        ],
    ).run_sync()

    self.assertListEqual(
        Band.select().run_sync(),
        [
            {
                "id": self.band.id,
                "name": new_name,
                "popularity": new_popularity + 2000,
            }
        ],
    )

def test_do_update_tuple_values_string_column_name(self):
    """
    We can use string column name in tuple values.
    """
    Band = self.Band

    new_popularity = self.band.popularity + 1000
    new_name = "Rustaceans"

    Band.insert(
        Band(
            id=self.band.id,
            name=new_name,
            popularity=new_popularity,
        )
    ).on_conflict(
        action="DO UPDATE",
        targets=[Band.id],
        values=[
            ("name", new_name),
            ("popularity", new_popularity + 2000),
        ],
    ).run_sync()

    self.assertListEqual(
        Band.select().run_sync(),
        [
            {
                "id": self.band.id,
                "name": new_name,
                "popularity": new_popularity + 2000,
            }
        ],
    )

dantownsend · 2023-05-02T20:36:58Z

Can I suggest two more tests that will cover tuple value as columns and tuple value as columns name in values argument.

Good point - I'll add those.

I realised I made a small mistake. When specifying the target, you can specify multiple columns. If you specify column_a and column_b, it means it's targeting a composite unique constraint for column_a and column_b. I realised when I was writing a test - need to update the docs to reflect this.

dantownsend · 2023-05-02T22:18:57Z

I added a where clause, but it's failing on cockroach.

I also need to move it. The first one is just useful for conditional constraints, but the second one is more likely to be used:

sinisaos · 2023-05-03T06:16:12Z

I added a where clause, but it's failing on cockroach.

I also need to move it. The first one is just useful for conditional constraints, but the second one is more likely to be used:

I agree with you and think you should move self.where below self.actions to get correct querystring like this

if self.action:
    query += " {}"
    values.append(self.action_string)

if self.where:
    query += " WHERE {}"
    values.append(self.where.querystring)

because Cockroach ON CONFLICT with where clause syntax is like this (from docs)

You can also use a WHERE clause to apply the DO UPDATE SET expression conditionally:

INSERT INTO accounts (id, balance)
    VALUES (8, 700)
    ON CONFLICT (id)
    DO UPDATE SET balance = excluded.balance
    WHERE excluded.balance > accounts.balance;

After that you should also update the test for where

query = Band.insert(
    Band(name=self.band.name, popularity=new_popularity)
).on_conflict(
    target=Band.name,
    where=Band.popularity < new_popularity,
    action="DO UPDATE",
    values=[Band.popularity],
)

self.assertIn(
    f'WHERE "band"."popularity" < {new_popularity}',
    query.__str__(),
)

I hope that makes sense.

dantownsend · 2023-05-03T15:41:33Z

@sinisaos Yes, you're right - thanks. I've made those changes.

Once the tests pass, I'll release this. There are some minor things missing, but I think this is fine for 99% of use cases.

dantownsend · 2023-05-03T16:05:21Z

Follow up actions in the future:

Maybe expose some of this via save.
Allow conditional constraints (using where)
Use on_conflict='DO NOTHING' with Table.add_m2m

sinisaos · 2023-05-03T16:13:06Z

Use on_conflict='DO NOTHING' with Table.add_m2m

When you release the new Piccolo version, if you want, I can open a PR to do it as we discussed.

dantownsend · 2023-05-03T16:14:30Z

When you release the new Piccolo version, if you want, I can open a PR to do it as we discussed.

@sinisaos Thanks. My only concern is we might need composite unique constraints to make it work. So the M2M table has two foreign keys, and they're unique together.

sinisaos · 2023-05-03T16:43:18Z

@dantownsend Ok. You're probably right and you know better about these things than I do, so I'll leave it up to you.

dantownsend · 2023-05-03T16:50:13Z

@sinisaos Composite unique constraints is definitely something we need to add soon. I just haven't figured out a great API for it, because most of Piccolo works with column references instead of strings.

Could be something like this (using strings):

class Band(Table, constraints=[UniqueTogether('name', 'manager')]):
    name = Varchar()
    manager = ForeignKey()

Or this:

class Band(Table):
    name = Varchar()
    manager = ForeignKey()
    constraints = [UniqueTogether('name', 'manager')])

This might work ... I'm not sure though:

class Band(Table):
    name = Varchar()
    manager = ForeignKey()
    constraints = [UniqueTogether(name, manager)])

I considered something like this, but it's hard to track for migrations:

class Band(Table):
    name = Varchar()
    manager = ForeignKey()

    @classmethod
    def constraints(cls):
        return [UniqueTogether(cls.name, cls.manager)]

If you have any ideas / preference let me know.

sinisaos · 2023-05-03T16:57:13Z

@dantownsend If I may ask, why not use this PR #582?

dantownsend · 2023-05-03T17:01:06Z

@sinisaos I think that PR is good. Just considering all the options, as once it's in, the API is very hard to change.

sinisaos · 2023-05-03T18:14:54Z

@dantownsend These are all valid choices and I don't know how difficult it is to implement them, but I like this example the most.

class Band(Table):
    name = Varchar()
    manager = ForeignKey()
    constraints = [UniqueTogether('name', 'manager')])

dantownsend · 2023-05-03T21:09:39Z

@sinisaos Yeah, something like that probably makes more sense than passing the args in like class Band(Table, constraints=...).

Constraints and schema support are the main things to add next. Will try and go through that PR soon.

sinisaos · 2023-05-04T18:09:35Z

@dantownsend As usual you were right 😄. Unique constraints in joining_table will solve the problem of duplicates. I just tried adding a constraints with raw sql like this (just an example)

class TrackToPlaylist(Table, db=DB):
    tracks = ForeignKey(Track)
    playlists = ForeignKey(Playlist)

    async def constraints():
        constraint = "CREATE UNIQUE INDEX unique_tracks_playlists ON track_to_playlist(tracks, playlists)"
        await TrackToPlaylist.raw(constraint)

and then I just change the insert methods to use on_conflict in M2MAddRelated like this

async def _run(self):
    ...
    if unsaved:
            await rows[0].__class__.insert(*unsaved).on_conflict(
                action="DO NOTHING"
            ).run()
    ...
    return (
            await joining_table.insert(*joining_table_rows)
            .on_conflict(action="DO NOTHING")
            .run()
        )

and everything works and there are no duplicates in the joining_table. When you implement UniqueTogether it should also solve the problem of duplicates in m2m. This is just info and I hope this is useful for you.

dantownsend · 2023-05-04T21:33:09Z

@sinisaos Thanks for checking that - we know it's the right approach then.

Do you want to create a PR, adding on_conflict to that insert query? It might be worth merging in straight away, as at least the user can manually add the composite unique constraint.

dantownsend added 2 commits April 28, 2023 00:16

initial commit

6533ee4

fix tests

0ee5c89

dantownsend added the enhancement New feature or request label Apr 28, 2023

dantownsend added 2 commits April 28, 2023 10:16

version pin litestar

c58dbbc

use typing extensions for Literal

ccc1538

sinisaos reviewed Apr 28, 2023

View reviewed changes

tests/table/test_insert.py Outdated Show resolved Hide resolved

sinisaos reviewed Apr 28, 2023

View reviewed changes

piccolo/query/mixins.py Show resolved Hide resolved

sinisaos reviewed Apr 28, 2023

View reviewed changes

piccolo/query/mixins.py Outdated Show resolved Hide resolved

sinisaos reviewed Apr 28, 2023

View reviewed changes

piccolo/query/mixins.py Outdated Show resolved Hide resolved

dantownsend and others added 6 commits April 28, 2023 17:49

make suggested change

03287bc

undo

87afd15

Update piccolo/query/mixins.py

745630a

Co-authored-by: sinisaos <[email protected]>

Update piccolo/query/mixins.py

d32b89b

Co-authored-by: sinisaos <[email protected]>

add one extra comma

85ee5a0

Merge branch '252-on-conflict-clause' of github.com:telerytech/piccol…

5623bc8

…o into 252-on-conflict-clause

first attempt at docs

e171a1a

dantownsend added the new feature label Apr 28, 2023

dantownsend added 3 commits April 30, 2023 22:36

add test for using all_columns

11de6f6

add test for using an enum to specify the action

ba0e8e2

add a test to make sure DO UPDATE with no values raises an exception

0d2ce6d

dantownsend marked this pull request as ready for review April 30, 2023 21:43

sinisaos reviewed May 1, 2023

View reviewed changes

sinisaos mentioned this pull request May 1, 2023

Api for upserting #798

Closed

dantownsend added 2 commits May 2, 2023 23:12

rename targets back to target

bbe4c6f

integrate @sinisaos tests

2f3881a

dantownsend added 2 commits May 3, 2023 16:35

move on_conflict to its own page

54c02da

refactor the where clause

8c4a99c

dantownsend merged commit 63af9c4 into master May 3, 2023

Drapersniper added a commit to PyLav/PyLav that referenced this pull request May 3, 2023

Update ahead of Picollo release with piccolo-orm/piccolo#816

4ccc17f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On conflict clause / upserts #816

On conflict clause / upserts #816

dantownsend commented Apr 28, 2023 •

edited

Loading

codecov-commenter commented Apr 28, 2023 •

edited

Loading

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023 •

edited

Loading

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

dantownsend commented Apr 28, 2023

dantownsend commented Apr 30, 2023

sinisaos May 1, 2023

sinisaos commented May 1, 2023

dantownsend commented May 2, 2023

dantownsend commented May 2, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023

dantownsend commented May 3, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023 •

edited

Loading

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023 •

edited

Loading

sinisaos commented May 4, 2023

dantownsend commented May 4, 2023

		@@ -1364,12 +1364,12 @@ def test_distinct_on_sqlite(self):
		SQLite doesn't support ``DISTINCT ON``, so a ``ValueError`` should be

On conflict clause / upserts #816

On conflict clause / upserts #816

Conversation

dantownsend commented Apr 28, 2023 • edited Loading

codecov-commenter commented Apr 28, 2023 • edited Loading

Codecov Report

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023 • edited Loading

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

sinisaos commented Apr 28, 2023

dantownsend commented Apr 28, 2023

dantownsend commented Apr 28, 2023

dantownsend commented Apr 30, 2023

sinisaos May 1, 2023

Choose a reason for hiding this comment

sinisaos commented May 1, 2023

dantownsend commented May 2, 2023

dantownsend commented May 2, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023

dantownsend commented May 3, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023 • edited Loading

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023

sinisaos commented May 3, 2023

dantownsend commented May 3, 2023 • edited Loading

sinisaos commented May 4, 2023

dantownsend commented May 4, 2023

dantownsend commented Apr 28, 2023 •

edited

Loading

codecov-commenter commented Apr 28, 2023 •

edited

Loading

sinisaos commented Apr 28, 2023 •

edited

Loading

dantownsend commented May 3, 2023 •

edited

Loading

dantownsend commented May 3, 2023 •

edited

Loading