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

feat(deleteMany): add limit option to deleteMany #5105

Merged
merged 6 commits into from
Jan 8, 2025

Conversation

FGoessler
Copy link
Contributor

@FGoessler FGoessler commented Jan 7, 2025

This PR aims to implement a limit param on deleteMany operations as requested in prisma/prisma#6957.

For now it uses subqueries for all SQL databases to achieve this although some databases (e.g. MySQL) support a more native LIMIT statement as well.

I decided to go with the limit naming for this option instead of the existing take naming used for select queries. For me this is semantically more fitting.

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

codspeed-hq bot commented Jan 7, 2025

CodSpeed Performance Report

Merging #5105 will not alter performance

Comparing feat/limit-delete-many (4917ea7) with main (4123509)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Jan 7, 2025

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.097MiB 2.096MiB 910.000B
Postgres (gzip) 841.797KiB 841.363KiB 445.000B
Mysql 2.058MiB 2.057MiB 918.000B
Mysql (gzip) 828.071KiB 827.871KiB 205.000B
Sqlite 1.973MiB 1.972MiB 918.000B
Sqlite (gzip) 792.801KiB 792.450KiB 360.000B

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example update of a snapshot:

7006327 |             {
7006328 |               "name": "deleteManywebsite_redirect",
7006329 |               "args": [
7006330 |                 {
7006331 |                   "name": "where",
7006332 |                   "isRequired": false,
7006333 |                   "isNullable": false,
7006334 |                   "inputTypes": [
7006335 |                     {
7006336 |                       "type": "website_redirectWhereInput",
7006337 |                       "namespace": "prisma",
7006338 |                       "location": "inputObjectTypes",
7006339 |                       "isList": false
7006340 |                     }
7006341 |                   ]
7006342 |+                },
7006343 |+                {
7006344 |+                  "name": "limit",
7006345 |+                  "isRequired": false,
7006346 |+                  "isNullable": false,
7006347 |+                  "inputTypes": [
7006348 |+                    {
7006349 |+                      "type": "Int",
7006350 |+                      "location": "scalar",
7006351 |+                      "isList": false
7006352 |+                    }
7006353 |+                  ]
7006354 |                 }
7006355 |               ],
7006356 |               "isNullable": false,
7006357 |               "outputType": {
7006358 |                 "type": "AffectedRowsOutput",
7006359 |                 "namespace": "prisma",
7006360 |                 "location": "outputObjectTypes",
7006361 |                 "isList": false
7006362 |               }
7006363 |             },

@FGoessler FGoessler force-pushed the feat/limit-delete-many branch from 4d10b18 to 90b30b7 Compare January 7, 2025 12:51
@FGoessler FGoessler marked this pull request as ready for review January 8, 2025 08:51
@FGoessler FGoessler requested a review from a team as a code owner January 8, 2025 08:51
@FGoessler FGoessler requested review from jkomyno and removed request for a team January 8, 2025 08:51
Comment on lines 322 to 324
builder.limit = limit;

let builder = builder.with_model_projection(id_field)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to the first builder once that variable is aliased in line 324? Is the .limit field preserved? If that's the case, shouldn't these two lines be swapped to make the resulting code semantics more intuitive and apparent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is preserved. I am not sure how moving the limit statement down improves the semantics. Esp. considering the builder.joins etc assignments in the lines above.
Technically line 324 can also be simply builder = builder.with_model_projection(id_field)?; (no let).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how moving the limit statement down improves the semantics.
It doesn't, but it makes it clearer to the human reader that the .limit field is preserved for the second builder shadowed variable

Technically line 324 can also be simply builder = builder.with_model_projection(id_field)?; (no let).
I'm sure there are reasons for that declaration that rescopes the second builder. Let's keep it that way unless we have a compelling argument to not do so :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Moved it but also had to make the second builder var now mutable.

Copy link
Member

@aqrln aqrln Jan 8, 2025

Choose a reason for hiding this comment

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

Late to the party here but I'd personally prefer to shadow the previous binding rather than make it mutable in cases like these. In this specific case a new immutable binding to replace the old mutable one looks intentional to limit the scope of mut only to the few lines where we need to assign some fields.

@aqrln aqrln added this to the 6.3.0 milestone Jan 8, 2025
@FGoessler FGoessler merged commit 66ff515 into main Jan 8, 2025
367 checks passed
@FGoessler FGoessler deleted the feat/limit-delete-many branch January 8, 2025 15:05
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.

4 participants