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

System.NotImplementedException for Precompiled Query with detailed errors enabled #34393

Open
ChrisJollyAU opened this issue Aug 9, 2024 · 7 comments · May be fixed by #34912
Open

System.NotImplementedException for Precompiled Query with detailed errors enabled #34393

ChrisJollyAU opened this issue Aug 9, 2024 · 7 comments · May be fixed by #34912
Assignees
@roji
Labels
area-aot area-query consider-for-next-release customer-reported type-bug
Milestone
Backlog

Comments

@ChrisJollyAU
Copy link
Contributor

I am busy adding the PrecompiledQuery functionality to EFCore.Jet provider including its test suite and have run into an error I'm not sure about

When running a test (e.g. BinaryExpression) in PrecompiledQueryJetTest (outside of naming and such pretty much similar to PrecompiledQuerySqlServerTest), I end up with the following error

 Precompilation error: System.NotImplementedException: The method or operation is not implemented.
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.VisitTry(TryExpression tryNode)
   at System.Linq.Expressions.TryExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.Translate[T](Expression node)
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.TranslateList(IReadOnlyList`1 list)
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.VisitNewArray(NewArrayExpression newArray)
   at System.Linq.Expressions.NewArrayExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
...... stack trace with a bunch of Visits
at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.TranslateCore(Expression node, IReadOnlyDictionary`2 constantReplacements, ISet`1 collectedNamespaces, ISet`1 unsafeAccessors, Boolean statementContext)
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqToCSharpSyntaxTranslator.TranslateExpression(Expression node, IReadOnlyDictionary`2 constantReplacements, ISet`1 collectedNamespaces, ISet`1 unsafeAccessors)
   at Microsoft.EntityFrameworkCore.Query.Internal.RuntimeModelLinqToCSharpSyntaxTranslator.TranslateExpression(Expression node, IReadOnlyDictionary`2 constantReplacements, IReadOnlyDictionary`2 memberAccessReplacements, ISet`1 collectedNamespaces, ISet`1 unsafeAccessors)
   at Microsoft.EntityFrameworkCore.Query.Internal.PrecompiledQueryCodeGenerator.GenerateQueryExecutor(IndentedStringBuilder code, Int32 queryNum, Expression queryExecutor, HashSet`1 namespaces, HashSet`1 unsafeAccessors)
   at Microsoft.EntityFrameworkCore.Query.Internal.PrecompiledQueryCodeGenerator.ProcessSyntaxTree(SyntaxTree syntaxTree, SemanticModel semanticModel, IReadOnlyList`1 locatedQueries, List`1 precompilationErrors, String suffix, ISet`1 generatedFileNames, CancellationToken cancellationToken)

  Stack Trace: 
PrecompiledQueryTestHelpers.FullSourceTest(String sourceCode, DbContextOptions dbContextOptions, Type dbContextType, Action`1 interceptorCodeAsserter, Action`1 errorAsserter, ITestOutputHelper testOutputHelper, Boolean alwaysPrintGeneratedSources, String callerName)
PrecompiledQueryTestHelpers.FullSourceTest(String sourceCode, DbContextOptions dbContextOptions, Type dbContextType, Action`1 interceptorCodeAsserter, Action`1 errorAsserter, ITestOutputHelper testOutputHelper, Boolean alwaysPrintGeneratedSources, String callerName)
PrecompiledQueryJetTest.BinaryExpression() line 31
--- End of stack trace from previous location ---

It obviously doesn't happen with Sql Server or Sqlite.

Debugging info
This seems to have something to do with the Execution Strategy being used. When it uses the default NonRetryingExecutionStrategy it comes up with the above error. However if I use the retrying execution strategy (JetRetryingExecutionStrategy or TestJetRetryingExecutionStrategy) it works fine as expected.

Further debugging brings me to the CompileQuery function within the ProcessSyntaxTree in this file https://github.com/dotnet/efcore/blob/main/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs#L188

It is further in the ProcessSyntaxTree function when it calls GenerateQueryExecutor that we get the error.

Looking at the debug view of the queryExecutor variable we see the following difference (only relevant part shown - not the whole view)
This is what I get with Sql Server and Sqlite

$materializationContext1 = .New Microsoft.EntityFrameworkCore.Storage.MaterializationContext(
                $emptyValueBuffer,
                $queryContext.Context);
            $instance1 = .Default(Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase+Blog);
            $entry1 = .Call $queryContext.TryGetEntry(
                $key,
                .NewArray System.Object[] {
                    (System.Object).Call $dataReader.GetInt32(0)
                },
                True,
                $hasNullKey1);
            .If (
                !$hasNullKey1

This is what I get on EFCore.Jet

$materializationContext1 = .New Microsoft.EntityFrameworkCore.Storage.MaterializationContext(
                $emptyValueBuffer,
                $queryContext.Context);
            $instance1 = .Default(Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase+Blog);
            $entry1 = .Call $queryContext.TryGetEntry(
                $key,
                .NewArray System.Object[] {
                    .Try {
                        (System.Object).Call $dataReader.GetInt32(0)
                    } .Catch (System.Exception $e) {
                        .Call Microsoft.EntityFrameworkCore.Query.RelationalShapedQueryCompilingExpressionVisitor+ShaperProcessingExpressionVisitor.ThrowReadValueException(
                            $e,
                            .Call $dataReader.GetFieldValue(0),
                            .Constant<System.Type>(System.Object),
                            $'property: Blog.Id (int) Required PK AfterSave:ThrowProperty')
                    }
                },
                True,
                $hasNullKey1);
            .If (
                !$hasNullKey1

Note the part where the data reader call is wrapped in a try...catch. Not sure why it is being generated

As a further note, in the VisitTry function of LinqToCSharpSyntaxTranslator, the _context variable that the switch is done on is ExpressionContext.Expression hence the NotImplementedException

I know its still marked as experimental so maybe I have hit something not finished yet

SDK: .Net 9 preview 6
Visual Studio: 17.11.0 preview 7
EF Core: 9.0.0-rc.1.24374.2 ( a daily build a day or 2 after the last preview 7 so should be close to what the final preview 7 release will be)
Using the preview 6 branch of efcore to build and debug on the SQL Server and Sqlite for comparison

Any thoughts?
@ChrisJollyAU
Copy link
Contributor Author

Bit more info on this

Its actually part of the shaper created in RelationalShapedQueryCompilingExpressionVisitor (under EFCore.Relational\Query)

It is part of the function CreateGetValueExpression

When you have detailed erros enabled you have the following block that executes of which it doesn't seem like the precompile undewrstands it yet

if (_detailedErrorsEnabled
    && !buffering)
{
    var exceptionParameter = Parameter(typeof(Exception), name: "e");

    var catchBlock = Catch(
        exceptionParameter,
        Call(
            ThrowReadValueExceptionMethod.MakeGenericMethod(valueExpression.Type),
            exceptionParameter,
            Call(dbDataReader, GetFieldValueMethod.MakeGenericMethod(typeof(object)), indexExpression),
            Constant(valueExpression.Type.MakeNullable(nullable), typeof(Type)),
            _parentVisitor.Dependencies.LiftableConstantFactory.CreateLiftableConstant(
                property,
                LiftableConstantExpressionHelpers.BuildMemberAccessLambdaForProperty(property),
                property + "Property",
                typeof(IPropertyBase))));

    valueExpression = TryCatch(valueExpression, catchBlock);
}

It is simple to recreate this issue - just add .EnableDetailedErrors() to the builder in AddProviderOptions.
e.g. with Sqlite you will have

public virtual DbContextOptionsBuilder AddProviderOptions(
    DbContextOptionsBuilder builder,
    Action<SqliteDbContextOptionsBuilder>? configureSqlite)
    => builder.UseSqlite(
        Connection, b =>
        {
            b.CommandTimeout(CommandTimeout);
            b.UseQuerySplittingBehavior(QuerySplittingBehavior.SingleQuery);
            configureSqlite?.Invoke(b);
        }).EnableDetailedErrors();

@AndriySvyryd AndriySvyryd changed the title System.NotImplementedException with precompiledquery tests System.NotImplementedException for Precompiled Query with detailed errors enabled Sep 20, 2024
@roji roji added this to the Backlog milestone Oct 11, 2024
@roji
Copy link
Member

roji commented Oct 11, 2024

Thanks @ChrisJollyAU, the new precompiled query support is quite a massive piece of work, and there are definitely quite a few holes in there. Will put this on the consider list for 10, as we make the NativeAOt support more mature and hopefully bring it out of experimental status.

@ChrisJollyAU
Copy link
Contributor Author

Might have a go at this sometime

@roji
Copy link
Member

roji commented Oct 11, 2024

Sure thing! Feel free to submit a PR.

@ChrisJollyAU
Copy link
Contributor Author

Had a look at this. I think this may need a further discussion with design.

Here's the thing, NotImplemented is entirely correct. From what I understand (and I haven't done much with syntax generation and precompiled), the Try..catch can only be a statement and not an expression so you should never hit VistTry when your _context is an Expression

In this case looking at the call stack it is preceded by a VisitNewArray and TranslateList so effectively what we have is something like the following

new object[]
        {
            try {
                dataReader.GetInt32(0);
            }
            catch (Exception e) {
            },
            try
            {
                dataReader.GetInt32(1);
            }
            catch (Exception e)
            {
        };

Note that when detailed errors are enabled, calls to the dataReader to get the field value, are wrapped in a try..catch (code show in a previous comment above)

I don't believe you can have a try...catch in the initializer of an array and it doesn't look like the SyntaxGenerator can do something like that.

Somethings to think about:

  • Support EnableDetailedErrors in NativeAOT/precompiled
  • Maybe just ignore the try catch and just use the plain translatedBody as that would work there (aka detailed errors off)
  • Rethink strategy of where the try...catch goes
    • e.g. instead of doing in the initializer, create the array, then do a for loop over all the field id's you want. Within the for loop you do the above mentioned try...catch to get the value and then add that to the array

@roji
Copy link
Member

roji commented Oct 13, 2024 •
edited
Loading

@ChrisJollyAU LINQ expression trees are indeed different from C# syntax: in LINQ expression trees, various things (e.g. try/catch) can be expressions although in C# they cannot. As a result, the translator needs to deal with these cases by lifting out the incompatible expression, doing something like the following:

int temp;
try
{
    temp = dataReader.GetInt32(0);
}
catch (...) { ...};

return new object[] { temp, ... }

The translator already does this for various constructs - take a look at the various tests; the same would need to be done for try/catch.

ChrisJollyAU added a commit to ChrisJollyAU/efcore that referenced this issue Oct 15, 2024
@ChrisJollyAU
Lift try..catch out of an expression and replace with a temporary var…
29e8218 
…iable

Fixes dotnet#34393
@ChrisJollyAU ChrisJollyAU linked a pull request Oct 15, 2024 that will close this issue
Open
@ChrisJollyAU
Copy link
Contributor Author

Ok, here's my attempt.

PrecompiledQuerySqlServerTest and AdHocPrecompiledQuerySqlServerTest (and same with sqlite) are all working locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@roji roji

Labels
area-aot area-query consider-for-next-release customer-reported type-bug
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Lift try..catch out of an expression and replace with a temporary variable ChrisJollyAU/efcore
4 participants
@ajcvickers @roji @AndriySvyryd @ChrisJollyAU