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

Change ExecuteUpdate to accept non-expression lambda #35257

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

roji
Copy link
Member

@roji roji commented Dec 2, 2024

This PR changes ExecuteUpdate to accept Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>> instead of Expression<Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>>>. This allows mainly for easy dynamic composition of setters (but has other advantages), and is more in-line with how LINQ operators generally work.

  • ExecuteUpdate needs to add an expression node to the tree just like any other operator, so that that node can get translated properly. Previously we just got an expression tree directly as the argument, but now get a Func; so this Func must be evaluated immediately, and its results (which are a set of property/value selector pairs) integrated into the tree.
  • The funcletizer must also be able to properly process the tree fragment produced by ExecuteUpdate; this means that we can't e.g. simply shove a ConstantExpression into it. Instead, we construct a NewArrayExpression representing all the tuples (the funcletizer will visit into it), and later in translation we "parse" that back into the property/value selectors, to be passed to TranslateExecuteUpdate.
  • Precompilation of this new mode is tricky in various ways...
    • Before, ExecuteUpdate just got a single expression tree argument, so it was the same as all other LINQ operators; but we now have a non-expression argument, which, when evaluated, produces an expression argument. This requires special handling in precompilation, to parse the user's SetProperty() calls and actually evaluate them, just as the ExecuteUpdate operator implementation does for non-precompilation.
    • In effect, precompilation needs to do for ExecuteUpdate exactly what we have been doing for non-precompilation - the code just moves around.

Closes #32018

@roji roji force-pushed the ExecuteUpdateLambda branch from 879c7e5 to f378a22 Compare December 5, 2024 12:20
@roji roji marked this pull request as ready for review December 5, 2024 12:20
@roji roji requested a review from a team as a code owner December 5, 2024 12:20
@roji
Copy link
Member Author

roji commented Dec 5, 2024

@ajcvickers this should be ready if you want to take a look.

@roji
Copy link
Member Author

roji commented Dec 20, 2024

/azp run

Copy link

No pipelines are associated with this pull request.

@roji roji closed this Dec 20, 2024
@roji roji reopened this Dec 20, 2024
@roji
Copy link
Member Author

roji commented Dec 20, 2024

/azp run

Copy link

No pipelines are associated with this pull request.

@roji
Copy link
Member Author

roji commented Dec 21, 2024

@AndriySvyryd any idea why this PR refuses to build (I'm seeing this happening with other open PRs at the moment)?

@AndriySvyryd
Copy link
Member

@AndriySvyryd any idea why this PR refuses to build (I'm seeing this happening with other open PRs at the moment)?

Probably an AZDO outage

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.

Accept Action<SetPropertyCalls> in ExecuteUpdate
2 participants