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

Support ExecuteUpdate when the value selector is a collection #32494

Open
roji opened this issue Dec 2, 2023 · 1 comment
Open

Support ExecuteUpdate when the value selector is a collection #32494

roji opened this issue Dec 2, 2023 · 1 comment

Comments

@roji
Copy link
Member

roji commented Dec 2, 2023

The following query fails:

await db.Test
    .ExecuteUpdateAsync(c => c.SetProperty(x => x.TextParts, x => x.TextParts.Concat(arr)));

The 1st reason for the failure is #32493; this can be worked around by extracting the lambda to a preceding Select:

await ctx.Blogs
    .Select(b => new { Blog = b, NewTextParts = b.TextParts.Concat(arr) })
    .ExecuteUpdateAsync(c => c.SetProperty(x => x.Blog.TextParts, x => x.NewTextParts));

At this point we run against other failures:

  1. If the type of the property being set (Blog.TextParts) and the type coming out of the value lambda is different, we attempt to apply a Convert node (code) which can fail (e.g. array property, List coming out of the selector - at least in PG).
  2. More importantly, the expression translated for NewTextParts in the anonymous object above is a CollectionResultExpression - this is what Select() returns when the thing being selected is an enumerable. We then attempt to (re-)translate that and fail.
    • We should be able to make this work.
    • But more generally, this raises the question of why we attempt to translate twice - once when translating the Select, and another time when translating the ExecuteUpdate's value selector lambda, into which we remap the lambda parameter (inserting a CollectionResultExpression into the lambda).
    • This can be another argument against out general remapping approach. Instead of doing a pre-pass the remap the lambda parameter, and then having to skip re-translating the thing we remapped, if SQL translator simply encountered a parameter it could process it accordingly.

Originally raised by @srasch in npgsql/efcore.pg#3001

@roji
Copy link
Member Author

roji commented Dec 3, 2023

Note: for this to actually work, we'd need to support transforming a relational rowset back to a primitive collection (tracked by #33792). For example, on SQL Server, the result of translating the above selector is a CollectionResultCollectiom, whose ProjectionBindingExpression is a ShapedQueryExpression doing a UNION over the column and the parameter:

Projection Mapping:
    EmptyProjectionMember -> t0.value
SELECT 1
FROM (
    SELECT t.value
    FROM OPENJSON(b.TextParts) WITH (value nvarchar(max) '') AS t
    UNION ALL
    SELECT a.value
    FROM OPENJSON(@__arr_0) AS a
) AS t0

On PostgreSQL we get a nicer, specialized translation:

Projection Mapping:
    EmptyProjectionMember -> t.value
SELECT 1
FROM unnest(b.TextParts + @__arr_0) WITH ORDINALITY AS t(value)

But that's still a rowset, whereas ExecuteUpdate requires the (non-rowset) primitive collection to be assigned to the column.

For SQL Server this would mean using FOR JSON, though I'm not seeing an option to return a simple array of primitives, only an array of objects (confirm):

SELECT * FROM (SELECT 1 UNION SELECT 2 UNION SELECT 3) AS f(v) FOR JSON AUTO;
-- Returns: [{"v":1},{"v":2},{"v":3}], but we want [1, 2, 3]

For PG, for the very specific above example, we'd simply unwrap the unnest. But when other operators are used, we get a real subquery like with SQL Server, so we need to use array_agg to transform that back from a rowset to an array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants