Skip to content

Conversation

csirmazbendeguz
Copy link
Contributor

@csirmazbendeguz csirmazbendeguz commented Oct 14, 2024

Trac ticket number

ticket-373

Branch description

Added the supports_tuple_in_subquery database feature.

This enables developers to use TupleIn with subqueries, e.g. WHERE (foo, bar) IN (SELECT foo, bar FROM baz).


This will be used by the feature requested by @LilyFoote (#18056 (comment)) later. The feature is the ability to UPDATE objects with composite primary keys + a subquery filter in one go (without pre-selecting the pks first).

e.g.:

UPDATE table
SET column = 'value'
WHERE (id1, id2) IN (SELECT id1, id2 FROM table)

See SQLUpdateCompiler.pre_sql_setup for more details (https://github.com/django/django/pull/18056/files#diff-f58de2deaccecd2d53199c5ca29e3e1050ec2adb80fb057cdfc0b4e6accdf14fR2128).

While all supported database backends support this operation, we need a feature flag so 3rd party database backends that don't support it can disable it (e.g. SQL Server) and fall back to pre-selecting IDs before update.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@csirmazbendeguz csirmazbendeguz force-pushed the ticket_373_tuple_in_subquery branch 4 times, most recently from 7de4136 to 1c898e2 Compare October 14, 2024 19:56
@sarahboyce sarahboyce force-pushed the ticket_373_tuple_in_subquery branch from 289ad42 to 629266d Compare October 28, 2024 14:34
@sarahboyce
Copy link
Contributor

buildbot, test on oracle.

@sarahboyce
Copy link
Contributor

While all supported database backends support this operation, we need a feature flag so 3rd party database backends that don't support it can disable it (e.g. SQL Server) and fall back to pre-selecting IDs before update.

I've not seen us do this before. Doesn't mean it isn't something we do
If they set supports_tuple_in_subquery = False they get a NotSupportedError right? I'm not sure where the fallback behavior is

@csirmazbendeguz
Copy link
Contributor Author

csirmazbendeguz commented Oct 28, 2024

@sarahboyce , the idea is to

  1. add subquery support to the TupleIn class now
  2. add the UPDATE subquery support in the composite pk patch (as you can see this feature flag is gonna affect if we gotta pre-select ids before update or no)

so yes the feature flag is not utilized for fallback in this patch but it will be in the next patch

@csirmazbendeguz
Copy link
Contributor Author

csirmazbendeguz commented Oct 28, 2024

but if you think this is not the best approach then let me know what solution you would prefer - that's why it's good to have these smaller PRs so we can discuss these smaller things in detail 😊 maybe we don't need the feature flag at all?

@sarahboyce
Copy link
Contributor

maybe we don't need the feature flag at all?

I don't know - maybe @charettes can confirm?

@charettes
Copy link
Member

If no core backends requires it today I'd suggest starting without a feature switch for now. We've seen third-party app maintainers be pretty reactive about requesting ones when needed.

Thoughts on the above @timgraham?

@csirmazbendeguz csirmazbendeguz force-pushed the ticket_373_tuple_in_subquery branch from c3d8b3a to 006ffb3 Compare October 31, 2024 08:54
@csirmazbendeguz
Copy link
Contributor Author

Thanks @charettes - alright, let's not worry about this right now, I suppose it's always easier to add it later than to remove it. @sarahboyce , I removed the feature switch.

@timgraham
Copy link
Member

The backends I maintain (CockroachDB and Snowflake) don't require the feature flag.

@LilyAcorn
Copy link
Contributor

While all supported database backends support this operation, we need a feature flag so 3rd party database backends that don't support it can disable it (e.g. SQL Server)

Given the concern about SQL Server specifically, maybe we should check with the maintainers of mssql-django? @mShan0 @absci do you have any thoughts here?

@csirmazbendeguz csirmazbendeguz force-pushed the ticket_373_tuple_in_subquery branch from 006ffb3 to 8f702cf Compare November 1, 2024 21:24
@absci
Copy link
Contributor

absci commented Nov 1, 2024

While all supported database backends support this operation, we need a feature flag so 3rd party database backends that don't support it can disable it (e.g. SQL Server)

Given the concern about SQL Server specifically, maybe we should check with the maintainers of mssql-django? @mShan0 @absci do you have any thoughts here?

Sorry, we're no longer maintaining mssql-django project, not sure what's the future plan. I don't know who's the new maintainer.

@csirmazbendeguz
Copy link
Contributor Author

I think it's safe to assume it's not maintained by anyone at the moment then?

@sarahboyce , can we proceed without the feature flag? It's easy to add if someone requests it in the future.

@csirmazbendeguz csirmazbendeguz force-pushed the ticket_373_tuple_in_subquery branch from 8f702cf to be3f837 Compare November 2, 2024 09:03
@sarahboyce sarahboyce merged commit f7601ae into django:main Nov 4, 2024
42 checks passed
@csirmazbendeguz
Copy link
Contributor Author

Thank you for the input everybody!

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.

6 participants