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

Implement ExecuteDelete #2449

Merged
merged 2 commits into from
Aug 5, 2022
Merged

Implement ExecuteDelete #2449

merged 2 commits into from
Aug 5, 2022

Conversation

roji
Copy link
Member

@roji roji commented Jul 26, 2022

Synced against dotnet/efcore#28492.

Nothing much to see here yet, the tests are implemented and all pass (see last commit only). The PostgreSQL-specific USING syntax for joining needs to be implemented, will do that following upstream implementation for SQL Server.

Closes #2450

/cc @smitpatel

@roji roji marked this pull request as draft July 26, 2022 18:29
@roji roji force-pushed the BulkDelete branch 3 times, most recently from 5e877b9 to 08aebff Compare July 27, 2022 09:33
{
table = selectExpression.Tables[0];
}
else if (selectExpression.Tables.All(t => t is TableExpression or InnerJoinExpression { Table: TableExpression }))
Copy link
Member Author

Choose a reason for hiding this comment

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

@smitpatel note this filter to only accept inner joins over tables (since that's what PG USING does/supports).

Copy link

@smitpatel smitpatel Jul 27, 2022

Choose a reason for hiding this comment

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

Little sad that only inner join is supported. I will add test which uses multiple inner joins. What do you think about cross apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree it's sad... Good to add some more tests (multiple inner joins, left/cross...).

I'll take a look at whether USING allows cross apply (lateral join as PG calls it). My gut says no.

Copy link
Member Author

Choose a reason for hiding this comment

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

@smitpatel unfortunately cross apply (lateral join) doesn't work with DELETE's USING:

DELETE FROM "Order Details" AS o
USING (
    SELECT o0."OrderID", o0."CustomerID", o0."EmployeeID", o0."OrderDate"
    FROM "Orders" AS o0
    WHERE o0."OrderID" < o."OrderID"
    ORDER BY o0."OrderID" NULLS FIRST
    LIMIT 100 OFFSET 0
) AS t
WHERE o."OrderID" < 10276

This errors with:

42P01: invalid reference to FROM-clause entry for table "o"

Note that it is apparently possible to do it with UPDATE's FROM, as here:

UPDATE x
SET foo=bar
FROM a, LATERAL (...)

I tried putting LATERAL betwen the subquery and USING in the DELETE above and that didn't work either 😢

Choose a reason for hiding this comment

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

Does it work in id in subquery form?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean the default behavior with the PK comparison? Sure, here's your test which passes. It's just that DELETE"s USING clause is apparently only for inner joins...

@roji roji force-pushed the BulkDelete branch 2 times, most recently from ac0c01c to 280ea15 Compare July 27, 2022 10:01
/// Converts the relational <see cref="NonQueryExpression" /> into a PG-specific <see cref="PostgresDeleteExpression" />, which
/// precisely models a DELETE statement in PostgreSQL. This is done to handle the PG-specific USING syntax for table joining.
/// </summary>
public class NonQueryConvertingExpressionVisitor : ExpressionVisitor
Copy link
Member Author

@roji roji Jul 28, 2022

Choose a reason for hiding this comment

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

@smitpatel here's the conversion to the PG-specific delete expression, as discussed in #2449 (comment). I think it does make it nicer, and query SQL generation is now trivial as it should be. Any thoughts?

src/EFCore.PG/Query/Internal/NpgsqlQuerySqlGenerator.cs Outdated Show resolved Hide resolved
@roji roji changed the title WIP on bulk delete Implement ExecuteDelete Aug 5, 2022
@roji roji marked this pull request as ready for review August 5, 2022 09:11
@roji roji enabled auto-merge (rebase) August 5, 2022 09:11
@roji roji disabled auto-merge August 5, 2022 09:12
@roji roji enabled auto-merge (rebase) August 5, 2022 09:32
@roji roji merged commit a1aa03e into npgsql:main Aug 5, 2022
@roji roji deleted the BulkDelete branch August 5, 2022 09:50
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.

Implement ExecuteUpdate/ExecuteDelete
2 participants