Report a guided error for temporal operators in compiled queries - #38852
Open
buvinghausen wants to merge 1 commit into
Open
Report a guided error for temporal operators in compiled queries#38852buvinghausen wants to merge 1 commit into
buvinghausen wants to merge 1 commit into
Conversation
A temporal operator whose point in time is a compiled-query parameter threw ArgumentException out of ExpressionTreeFuncletizer, complaining that IQueryable<T> did not fit a DbSet<T> parameter. - ExpressionTreeFuncletizer.VisitMethodCall: when a rebuilt argument is no longer assignable to a parameter declared DbSet<T>, keep the original argument. Evaluating a DbSet inlines its query root, which is typed IQueryable<T>, and that cannot be rebuilt into a DbSet<T> parameter. A query root needs no inlining -- it already is the root. VisitMember already declines to inline DbSet-typed members for this reason, citing FromSql, but that guard is bypassed when the parent processes the member as an evaluatable root. Scoped to DbSet<> parameters so nothing else changes behaviour. - SqlServerQueryableMethodTranslatingExpressionVisitor.VisitMethodCall: a SqlServerDbSetExtensions call that survives to translation was never executed, which means a compiled query. Report which operator, and why, instead of falling through to a generic "could not be translated". - The scenario cannot be supported as-is: SqlServerQuerySqlGenerator writes the point in time into the SQL as a literal, and a compiled query reuses one cached SQL string, so a per-invocation value has nowhere to go. Constant and captured-variable point-in-times, and TemporalAll, are unaffected and keep working. Fixes dotnet#38851
Comment on lines
+8
to
+9
| public class TemporalCompiledQuerySqlServerTest(TemporalCompiledQuerySqlServerFixture fixture) | ||
| : IClassFixture<TemporalCompiledQuerySqlServerFixture> |
Member
There was a problem hiding this comment.
Add this coverage to TemporalGearsOfWarQuerySqlServerTest instead of creating a new fixture
There was a problem hiding this comment.
Pull request overview
This PR improves the SQL Server temporal query experience under EF.CompileQuery / EF.CompileAsyncQuery by preventing an internal ArgumentException during funcletization and instead surfacing a guided, provider-specific error when a temporal point-in-time/range varies per invocation (which can’t be represented in cached compiled-query SQL).
Changes:
- Adjust
ExpressionTreeFuncletizer.VisitMethodCallto avoid inliningDbSet<T>-typed arguments intoIQueryable<T>when that would makeMethodCallExpression.Update(...)fail forDbSet<T>parameters. - Add a SQL Server translation-time guard that throws a guided
InvalidOperationExceptionfor temporal operators that reach translation (compiled-query expression-tree scenario). - Add new functional tests + fixture/store to validate the guided error and ensure supported shapes still work.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/Query/TemporalCompiledQuerySqlServerTest.cs | New functional tests covering guided error behavior for temporal operators under compiled queries. |
| test/EFCore.SqlServer.FunctionalTests/Query/TemporalCompiledQuerySqlServerFixture.cs | New dedicated fixture/context/entity/store setup for the compiled temporal tests. |
| src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs | Prevents DbSet<T> argument inlining from breaking method call rebuild for APIs declared with DbSet<T> parameters. |
| src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs | Throws a guided SQL Server error when a temporal DbSet extension call reaches translation (compiled query scenario). |
| src/EFCore.SqlServer/Properties/SqlServerStrings.resx | Adds a new user-facing error message resource for the compiled-query temporal limitation. |
| src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs | Updates the generated accessor for the new resource string. |
Files not reviewed (1)
- src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs: Generated file
Suppressed comments (1)
test/EFCore.SqlServer.FunctionalTests/Query/TemporalCompiledQuerySqlServerTest.cs:70
- This sync CompileQuery test also uses a substring match on the exception message. Since the feature change is specifically about producing a guided error message, the test should assert the full resource string for stability and to ensure the correct message is produced.
var exception = Assert.Throws<InvalidOperationException>(() => compiled(context, PointInTime).ToList());
Assert.Contains("compiled query", exception.Message);
}
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+34
to
+57
| [Theory] | ||
| [InlineData("FromTo")] | ||
| [InlineData("Between")] | ||
| [InlineData("ContainedIn")] | ||
| public async Task Temporal_range_operators_with_query_parameters_report_a_guided_error(string op) | ||
| { | ||
| using var context = fixture.CreateContext(); | ||
|
|
||
| Func<TemporalCompiledQueryContext, DateTime, DateTime, IAsyncEnumerable<string?>> compiled = op switch | ||
| { | ||
| "FromTo" => EF.CompileAsyncQuery( | ||
| (TemporalCompiledQueryContext c, DateTime a, DateTime b) => c.Customers.TemporalFromTo(a, b).Select(x => x.Name)), | ||
| "Between" => EF.CompileAsyncQuery( | ||
| (TemporalCompiledQueryContext c, DateTime a, DateTime b) => c.Customers.TemporalBetween(a, b).Select(x => x.Name)), | ||
| "ContainedIn" => EF.CompileAsyncQuery( | ||
| (TemporalCompiledQueryContext c, DateTime a, DateTime b) => c.Customers.TemporalContainedIn(a, b).Select(x => x.Name)), | ||
| _ => throw new ArgumentOutOfRangeException(nameof(op)), | ||
| }; | ||
|
|
||
| var exception = await Assert.ThrowsAsync<InvalidOperationException>( | ||
| async () => await compiled(context, PointInTime, LaterPointInTime).ToListAsync()); | ||
|
|
||
| Assert.Contains("compiled query", exception.Message); | ||
| } |
Comment on lines
+385
to
+387
| <data name="TemporalOperatorRequiresConstantArgumentInCompiledQuery" xml:space="preserve"> | ||
| <value>The temporal operator '{operatorName}' cannot be used in a compiled query with an argument that varies per invocation. The point in time is written into the SQL as a literal, and a compiled query reuses a single SQL string, so the argument must be a constant. Either use a constant point in time, or execute the query without EF.CompileQuery.</value> | ||
| </data> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On the second box: I filed #38851 with the full analysis and offered to contribute this, but
have not waited for team approval before opening the PR. Happy to close this and discuss the
shape on the issue first if you'd prefer — the analysis there stands on its own either way.
Fixes #38851
The bug
A temporal operator whose point in time is a compiled-query parameter throws out of parameter
extraction, before anything provider-specific runs:
Affects all four value-taking temporal operators (
TemporalAsOf,TemporalFromTo,TemporalBetween,TemporalContainedIn) under bothEF.CompileQueryandEF.CompileAsyncQuery.TemporalAll()and constant/captured point-in-times are unaffected.Why this reports an error rather than making it work
SqlServerQuerySqlGeneratorwrites the point in time into the SQL as a literal viaGenerateSqlLiteral. A compiled query caches one SQL string and reuses it across invocations,so a value that varies per invocation has nowhere to go. Supporting that would mean emitting
FOR SYSTEM_TIMEagainst a SQL parameter — a feature, and a separate discussion.What is clearly a bug is that an unsupported scenario surfaces as an internal
ArgumentExceptionabout expression types instead of a guided EF error. That is what this fixes.
The change
ExpressionTreeFuncletizer.VisitMethodCall— when a rebuilt argument is no longer assignableto a parameter declared
DbSet<T>, keep the original argument. Evaluating aDbSetinlines itsquery root, which is typed
IQueryable<T>; that is right forQueryable.Count(IQueryable<T>)butcannot be rebuilt into a
DbSet<T>parameter. A query root needs no inlining — it already is theroot.
VisitMemberalready declines to inlineDbSet-typed members for exactly this reason(its comment cites
FromSql), but that guard is bypassed when the parent processes the member asan evaluatable root. Scoped to
DbSet<>parameters so nothing else changes behaviour.SqlServerQueryableMethodTranslatingExpressionVisitor.VisitMethodCall— aSqlServerDbSetExtensionscall that survives to translation was never executed, which means acompiled query. Report which operator and why.
Both parts are needed: the funcletizer throws before the preprocessor runs, so a
QueryRootProcessorcannot substitute for the first, and without the second the query fallsthrough to a generic "could not be translated".
Tests
TemporalCompiledQuerySqlServerTest, 9 tests on a dedicated store:TemporalAll(), and non-temporal compiled queriesall still work
Verified locally against SQL Server 2025:
EFCore.Tests6985/0,EFCore.SqlServer.FunctionalTests50,862/0 failed.🤖 Generated with Claude Code