Introduce liftable constants to shaper to prepare for precompilation by maumar · Pull Request #33351 · dotnet/efcore

maumar · 2024-03-19T01:44:48Z

json reader writer proper fix

maumar · 2024-03-19T01:56:22Z

@roji created a separate PR to preserve the original code changes until we are happy with what we have. It's draft so far but should be ok to take a look. Only real outstanding issue is NTS, their value converter require SqlServerBytesReader/SqlServerBytesWriter, which in turn require NtsGeometryServices so it's a bit tricky to smuggle it in. Still a lot of small fixes, comments cleanups etc spoke too soon, still have lots of failures outside query

src/EFCore/Storage/ValueConversion/ValueConverter`.cs

test/EFCore.Specification.Tests/StoreGeneratedTestBase.cs

...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs

src/EFCore.Relational/Query/Internal/RelationalLiftableConstantExpressionDependencies.cs

src/EFCore/Metadata/FactoryMethodBinding.cs

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

maumar · 2024-03-28T09:02:46Z

@ajcvickers specifically for changes in the EntityMaterializerSource and the area around (lazy loading, injectible service, parameter binding etc)

src/EFCore/Query/LiftableConstantExpressionHelpers.cs

roji

Really nice to see this coming together! See first review comments.

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

src/EFCore/Query/StructuralTypeShaperExpression.cs

src/EFCore/Storage/ValueConversion/BoolToStringConverter.cs

src/EFCore/Storage/ValueConversion/ValueConverter.cs

src/Shared/ExpressionExtensions.cs

test/EFCore.Specification.Tests/Query/SpatialQueryTestBase.cs

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

src/EFCore.Relational/Query/RelationalQueryCompilationContext.cs

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

roji · 2024-04-07T18:02:56Z

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

+                    // NOTE: this is unreliable way to get type mapping. Only doing this as last resort, hoping to "guess" the right one
+                    typeMappingExpression = liftableConstantFactory.CreateLiftableConstant(
+                        typeMapping,
+                        c => (RelationalTypeMapping)c.Dependencies.TypeMappingSource.FindMapping(type, c.Dependencies.Model, null)!,


Doing the same lookup for the regular/inline case would at least show us if there's a failing test where this logic isn't correct (this is in general why I prefer we avoid having different paths for regular vs. precompiled as much as possible).

src/EFCore.SqlServer.HierarchyId/Storage/Json/SqlServerJsonHierarchyIdReaderWriter.cs

src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs

ajcvickers

Changes to EntityMaterializerSource seem okay.

maumar · 2024-04-09T09:01:10Z

@roji ready for the final peek, I removed all the changes not related to vanilla queries

roji

This looks almost ready - see remaining comments, I don't think they're blocking if they're problematic (let's discuss if so).

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

src/EFCore/Query/LiftableConstantExpressionHelpers.cs

src/EFCore/Query/QueryCompilationContext.cs

src/EFCore/Storage/Json/JsonBoolReaderWriter.cs

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

maumar · 2024-04-12T00:16:42Z

@roji updated with the latest feedback - all changes in separate commit for easier review

roji

LGTM, thanks @maumar!

Take a look at the remaining unresolved comments for final minor cleanup, but otherwise good to go!

src/EFCore/Query/LiftableConstantProcessor.cs

@roji

Fix materializer code to replace non-primitive constants with liftable constants. Precompilation is opt-in in QueryCompilationContext, switched on for relational, off for everything else. Architecture, design and initial implementation done by @roji, polish done by @maumar Part of #25009

roji · 2024-04-13T07:23:46Z

@maumar after rebasing the precompiled query PR (#33297) on top of this merged on, that PR build fails because of unsupported constant nodes (specifically ParameterBindingInfo, it seems). Can you please take a look?

maumar requested a review from roji March 19, 2024 01:44

maumar commented Mar 19, 2024

View reviewed changes

src/EFCore/Storage/ValueConversion/ValueConverter`.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch from 3378012 to 05cf1e9 Compare March 19, 2024 02:14

maumar commented Mar 19, 2024

View reviewed changes

test/EFCore.Specification.Tests/StoreGeneratedTestBase.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch 3 times, most recently from 5bd1bd8 to 3a7faf7 Compare March 24, 2024 11:35

maumar mentioned this pull request Mar 25, 2024

Sunday cleanup project: replace internal usage analyzer with [Experimental] #33385

Closed

maumar force-pushed the aot_materializer branch from 3a7faf7 to 61aa234 Compare March 25, 2024 18:26

roji reviewed Mar 26, 2024

View reviewed changes

...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch 6 times, most recently from 3548892 to fed9a36 Compare March 27, 2024 22:45

maumar commented Mar 28, 2024

View reviewed changes

src/EFCore.Relational/Query/Internal/RelationalLiftableConstantExpressionDependencies.cs Show resolved Hide resolved

maumar force-pushed the aot_materializer branch 2 times, most recently from d708d97 to 339b9af Compare March 28, 2024 06:49

maumar marked this pull request as ready for review March 28, 2024 06:49

maumar commented Mar 28, 2024

View reviewed changes

src/EFCore/Metadata/FactoryMethodBinding.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch 2 times, most recently from dd1cf50 to 487225a Compare March 28, 2024 08:11

maumar commented Mar 28, 2024

View reviewed changes

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch from 487225a to 0f61a77 Compare March 28, 2024 08:56

maumar requested a review from ajcvickers March 28, 2024 08:59

maumar force-pushed the aot_materializer branch from 0f61a77 to a424b3e Compare March 28, 2024 09:11

maumar commented Mar 28, 2024

View reviewed changes

src/EFCore/Query/LiftableConstantExpressionHelpers.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch from 1d415ae to c8a5950 Compare March 29, 2024 08:22

roji requested changes Mar 31, 2024

View reviewed changes

maumar force-pushed the aot_materializer branch 6 times, most recently from 226a7c9 to 8c3691b Compare April 4, 2024 08:11

roji reviewed Apr 7, 2024

View reviewed changes

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved

roji mentioned this pull request Apr 7, 2024

Implement precompiled queries #33297

Merged

roji reviewed Apr 7, 2024

View reviewed changes

maumar force-pushed the aot_materializer branch from 35fe741 to 49b9e76 Compare April 8, 2024 04:09

maumar commented Apr 8, 2024

View reviewed changes

src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs Outdated Show resolved Hide resolved

ajcvickers reviewed Apr 8, 2024

View reviewed changes

maumar force-pushed the aot_materializer branch 4 times, most recently from 828dbf0 to 36bb5a7 Compare April 9, 2024 06:04

maumar force-pushed the aot_materializer branch from 36bb5a7 to 805fdef Compare April 9, 2024 09:30

roji reviewed Apr 11, 2024

View reviewed changes

maumar force-pushed the aot_materializer branch from f47c305 to 3dcb48c Compare April 12, 2024 02:01

roji approved these changes Apr 12, 2024

View reviewed changes

src/EFCore/Query/LiftableConstantProcessor.cs Outdated Show resolved Hide resolved

maumar force-pushed the aot_materializer branch from 3dcb48c to 7880f1c Compare April 12, 2024 21:35

maumar merged commit f0e88d4 into main Apr 12, 2024

maumar deleted the aot_materializer branch April 12, 2024 23:13

roji mentioned this pull request Apr 13, 2024

Introduce liftable constants to shaper to prepare for precompilation #32721

Closed

Conversation

maumar commented Mar 19, 2024

Uh oh!

maumar commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maumar commented Mar 28, 2024

Uh oh!

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roji Apr 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajcvickers left a comment

Choose a reason for hiding this comment

Uh oh!

maumar commented Apr 9, 2024

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maumar commented Apr 12, 2024

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roji commented Apr 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maumar commented Mar 19, 2024 •

edited

Loading