Introduce liftable constants to shaper to prepare for precompilation #32721
Conversation
| var resolver = resolverExpression.Compile(preferInterpretation: true); | ||
| var value = resolver(_relationalMaterializerLiftableConstantContext); |
There was a problem hiding this comment.
It would be better to also store the constant value in LiftableConstant and only use Compile to assert it's the same value in Debug
That gives the C# generator a chance to determine whether it can just generate the constant instead of the resolver without having to compile
There was a problem hiding this comment.
Yeah, that's something I debated with myself when writing this. I think you're right, and I like the DEBUG-only check (though I sometimes wish we ran tests in CI in DEBUG as well...)
There was a problem hiding this comment.
@maumar if you're interested in doing this, it would be an opportunity to put hands inside the infra as well - let me know.
There was a problem hiding this comment.
BTW there could be some issues with asserting "same value" here - I'm not sure whether in all cases reference equality works (i.e. if the resolver instantiates a new instance that's equivalent but not reference-equal, this starts to possibly get complicated). Anyway, we can think about this together once the other work in the PR is done.
| // IReadOnlyList<ValueComparer> parentIdentifierValueComparers, | ||
| // IReadOnlyList<ValueComparer> outerIdentifierValueComparers, | ||
| // IReadOnlyList<ValueComparer> selfIdentifierValueComparers, |
There was a problem hiding this comment.
@AndriySvyryd yeah, this code is very much what I hacked together to make basic things work during the prototype. This absolutely needs to be cleaned up (and support extended for all cases).
|
Replaced by #33351 |
This PR is one piece of the precompiled query puzzle (#25009) - making sure that our generated shaper code is compatible with outputting to C#. This is something that materializer enthusiasts can work on independently, so we can start parallelizing the work. Of course, the actual infra introduced here also needs to be reviewed.
The problem is mainly removing Constant expression nodes which don't refer to literals (int, string), and therefore can't be outputted to C#. The fix is the "LiftableConstantExpression" mechanism; instead of integrating constants directly into the tree, materializer generation code can use a factory to generate these nodes, providing them with an expression that determines how to resolve the "constant", given starting points (e.g. the model). So instead of referencing an IEntityType as a constant, you create a liftable constant node that resolves that entity type given the model. In normal mode a visitor simply resolves these nodes, replacing them with the correct constant before compiling the expression; but in precompilation we'll instead generate code out of these resolvers.
This PR makes us fail if any non-liftable constant is in the shaper, so making all the tests green here should in principle mean that all our shaper generation is precompilation-compatible. Note also that the materializer changes you see in this PR were done in prototype/hacky mode a year ago, and need to be cleaned up/reviewed (but the concepts/infra should be OK).
In addition, this introduces a DEBUG-only ShaperPublicMethodVerifier, which makes sure that all method/constructor calls in the shaper reference fully public members (otherwise they can't be referenced in generated C# code).
Categories of test errors:
[EntityFrameworkInternal]!) - but also its declaring type, if it's on a nested type. But it might be an occasion to think a bit more widely about factoring etc., and maybe extract the method.