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.

Sign up for GitHub

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

Jump to bottom

New EF.Constant handling #34297

Merged
merged 2 commits into from
Aug 11, 2024
Merged

New EF.Constant handling #34297

merged 2 commits into from
Aug 11, 2024

Conversation

cincuranet
Copy link
Contributor

Fixes #33674

@cincuranet cincuranet requested a review from roji July 26, 2024 17:55
@cincuranet cincuranet mentioned this pull request Jul 31, 2024
Open
@cincuranet cincuranet changed the title [WIP] New EF.Constant handling New EF.Constant handling Jul 31, 2024
@cincuranet cincuranet marked this pull request as ready for review July 31, 2024 16:00
@cincuranet cincuranet requested a review from roji July 31, 2024 16:00
@cincuranet cincuranet force-pushed the ef-constant branch 2 times, most recently from 8c9275a to bdddc2b Compare August 1, 2024 07:40
roji
roji requested changes Aug 2, 2024
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Really good first attempt @cincuranet, this covers many parts of the query pipeline and it's great to see all this work. See below for all my comments.

Note that it would be good to add some tests which cover the bugs in the current implementations (e.g. inline collection with a single scalar parameter, which shouldn't get treated as a "values parameter").

src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs Outdated Show resolved Hide resolved
src/EFCore.Cosmos/Properties/CosmosStrings.resx Outdated Show resolved Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Query/IRelationalParameterBasedSqlProcessorFactory.cs Outdated Show resolved Hide resolved
src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs Outdated Show resolved Hide resolved
src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs Outdated Show resolved Hide resolved
src/EFCore/Query/QueryRootProcessor.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQueryOldSqlServerTest.cs Outdated Show resolved Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs Outdated Show resolved Hide resolved
@cincuranet
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Pull request contains merge conflicts.

@cincuranet cincuranet force-pushed the ef-constant branch 9 times, most recently from 322700a to 0eb0c37 Compare August 8, 2024 09:27
@cincuranet cincuranet requested a review from roji August 8, 2024 10:00
@cincuranet
Copy link
Contributor Author

@roji This is ready for another review pass.

roji
roji approved these changes Aug 8, 2024
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Almost LGTM, see only nits below 🎉

Can fix the nits, but please don't merge yet though - I want to take a look at the precompiled query code there.

src/EFCore.Relational/Query/IRelationalParameterBasedSqlProcessorFactory.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore/Properties/CoreStrings.Designer.cs Outdated Show resolved Hide resolved
src/EFCore/Properties/CoreStrings.resx Outdated Show resolved Hide resolved
src/EFCore/Query/QueryCompilationContext.cs Outdated Show resolved Hide resolved
test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs Outdated Show resolved Hide resolved
test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs Outdated Show resolved Hide resolved
@cincuranet cincuranet force-pushed the ef-constant branch 2 times, most recently from 24e1821 to 027557c Compare August 9, 2024 10:25
@cincuranet cincuranet enabled auto-merge (squash) August 11, 2024 12:40
@cincuranet cincuranet merged commit ab30db3 into dotnet:main Aug 11, 2024
7 checks passed
@cincuranet cincuranet deleted the ef-constant branch August 11, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roji roji roji approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reimplement EF.Constant without introducing constant nodes in the funcletizer
2 participants
@cincuranet @roji