_sqlAliasManager unexpectedly null in SelectExpression.PushdownIntoSubquery() call after previous call of SelectExpression.Update()

[lauxjpn]

An existing SelectExpression is "updated" in SqlNullabilityProcessor.Visit(SelectExpression selectExpression, bool visitProjection) via a call to SelectExpression.Update(...).

On that resulting SelectExpression, we later call SelectExpression.PushdownIntoSubquery().

Because SelectExpression.PushdownIntoSubquery() uses _sqlAliasManager internally, and SelectExpression.Update(...) does not retain the SqlAliasManager instance of the original SelectExpression, _sqlAliasManager is null in the SelectExpression.PushdownIntoSubquery() call and an unexpected/implicit NRE is thrown.

Include provider and version information

EF Core version: 9.0.0
Database provider: Pomelo.EntityFrameworkCore.MySql

[roji]

Hey @lauxjpn 👋

Are you saying you call PushdownIntoSubquery at some point after in SqlNullabilityProcessor or afterwards, in your provider? That's not something we currently do elsewhere - can you provide a bit more context on where/why you need to do that?

To give more context, SelectExpression (currently) has two modes: mutable and immutable; a SelectExpression is only mutable during the translation phase, as more operators get processed and the SelectExpression get mutated; right after translation it is made mutable. This is a design that I think is problematic in several ways, and I hope to be able to change in 10, so that our SQL tree is fully immutable. In any case, the SQL alias manager is currently only available in the mutable phase.

Very concretely: rather than doing PushdownIntoSubquery(), which is meant to be used during the mutable phase, maybe it's possible to simply construct the new SelectExpression on top of the existing one (perform a "manual pushdown")? If not, I'll look into finding a solution for you.

Unfortunately, this entire area is quite messy at the moment, and in a transitional phase... But I'm slowly trying to move us in a more sane direction.

[lauxjpn]

Yeah, we're back baby!

Original post

Yes, we have a couple of expression visitors that we need to run as late as possible. I have now moved the one in question (the one that is doing the pushdown) into the post translation handling. The debugger shows though, that IsMutable is already false.

Since at this point, no call to SelectExpression.Update() seems to have happened yet, the SelectExpression still contains a valid _sqlAliasManager object and we can still successfully perform a pushdown, even though IsMutable is already false (SelectExpression.PushdownIntoSubquery() does not check IsMutable, it just tries to use the _sqlAliasManager object whenever called).

Unfortunately, we can't be 100% sure that nobody has already called SelectExpression.Update() (there is no official way to check), so we are now calling the following extension method before the pushdown, to ensure that there is a valid _sqlAliasManager object available in the SelectExpression:

public static SelectExpression UpdateWithSqlAliasManager(
    this SelectExpression selectExpression,
    SqlAliasManager sqlAliasManager)
    => new(
        selectExpression.Alias,
        selectExpression.Tables.ToList(),
        selectExpression.Predicate,
        selectExpression.GroupBy.ToList(),
        selectExpression.Having,
        selectExpression.Projection.ToList(),
        selectExpression.IsDistinct,
        selectExpression.Orderings.ToList(),
        selectExpression.Offset,
        selectExpression.Limit,
        selectExpression.Tags,
        selectExpression.GetAnnotations().ToDictionary(a => a.Name, a => a),
        sqlAliasManager,
        false);

We are still able to call this method in MySqlQueryTranslationPostprocessor.Process(), because we still have access to a QueryCompilationContext:

// In  MySqlQueryTranslationPostprocessor.Process():
query = new MySqlHavingExpressionVisitor(
    _sqlExpressionFactory,
    ((RelationalQueryCompilationContext)QueryCompilationContext).SqlAliasManager)
    .Visit(query);

// In MySqlHavingExpressionVisitor.VisitSelect:
selectExpression = selectExpression.UpdateWithSqlAliasManager(_sqlAliasManager);
selectExpression.PushdownIntoSubquery();

The question is: Is it a bug that we are able to do this successfully in MySqlQueryTranslationPostprocessor.Process() (and it could break in the future, so we should use a different way to accomplish what we want), or is this still supposed to work there, even though IsMutable is already false (and we can continue using it for now)?

If we should not do this, we will either move the expression visitor to the end of the translation phase (but before MySqlQueryTranslationPostprocessor) or do the pushdown manually (as you suggested).

(For context: The expression visitor in question has to do some weird syntax stuff for queries with HAVING clauses, if the SELECT statement does not contain an aggregate function. In such cases (and only in those), the GROUP BY clause needs to reference aliases (from the projection) for the expressions it uses (it cannot use the expressions themselves). We need to run this late in the pipeline, so that we can be sure that nobody changes or alters the query in a significant way).

I will reevaluate what we are doing with our HAVING expression visitor and come back to this, once I know in which direction we want to go with this.

[roji]

Thanks for all the information.

Yeah, let me know where you end up. The current state of things really is transitional - I hope to be able to clean up a lot of this stuff in 10 and have a much better-factored, fully immutable SelectExpression and query pipeline... In the meantime, if you need a hack such as UpdateWithSqlAliasManager and don't mind having to react again for 10/11, then by all means go for it (I can see how something like that might be needed right now). And if you do get stuck, definitely don't hesitate to ping me again for assistance.

I'll go ahead and close this in the meantime (nothing immediately actionable), but post back and I'll reopen as needed.

Good luck! Looking forward to seeing the 9.0 release of the provider!

[ChrisJollyAU]

I'll add my 2 cents as I had the same issue (@roji see #16038 (comment))

I did end up going a very similar route but with a couple of extras

private SelectExpression AddAliasManager(SelectExpression selectExpression)
{
    //get private IsMutable property
    var ismutable = selectExpression.GetType().GetProperty("IsMutable", BindingFlags.NonPublic | BindingFlags.Instance);
    //get value
    var ismut = (bool)ismutable?.GetValue(selectExpression)!;

    //create new selectexp from selectexpression with aliasmanager
    var newselect = new SelectExpression(selectExpression.Alias,
        [.. selectExpression.Tables],
        selectExpression.Predicate, [.. selectExpression.GroupBy], selectExpression.Having,
        [.. selectExpression.Projection], selectExpression.IsDistinct,
        [.. selectExpression.Orderings], selectExpression.Offset, selectExpression.Limit,
        selectExpression.Tags, new Dictionary<string, IAnnotation>(), sqlAliasManager, ismut);

    //do private stuff
    //_projectionMapping = newProjectionMappings,
    //_clientProjections = newClientProjections,
    var clientProj = selectExpression.GetType().GetField("_clientProjections", BindingFlags.NonPublic | BindingFlags.Instance);
    var projMap = selectExpression.GetType().GetField("_projectionMapping", BindingFlags.NonPublic | BindingFlags.Instance);
    clientProj?.SetValue(newselect, clientProj.GetValue(selectExpression));
    projMap?.SetValue(newselect, projMap.GetValue(selectExpression));
    return newselect;
}

The key part is I also needed to copy the private _clientProjections. I was ending up hitting issues later on (not on everything though), where it was expecting something to be in _clinetProjections but it was empty

@lauxjpn Keep that in mind that you might run into that (I can't remember off hand what the scenarios were that triggered that problem). And good to see you're back

[roji]

@lauxjpn @ChrisJollyAU thanks for reporting on all this. I'll do my best to improve the overall query pipeline architecture so that hacks like these aren't needed. Thanks for being flexible/creative here in the meantime!

[lauxjpn]

@ChrisJollyAU Yeah, we hit the same problems along the way for Pomelo.

Our goal is to make a pushdown of a query into a subquery and then change that subquery.

We first tried to accomplish this in the part of the query pipeline, where the SelectExpressions are generally still mutable (though some subqueries might already be not), so before their projections got applied in RelationalQueryTranslationPostprocessor.Process(). There were two issues with that approach: First, we could not safely figure out, if the original SelectExpression was mutable or not, without using reflection to access the internal property IsMutable (which we did not want to do). The second issue was that we could not figure out, how to copy over the projection mappings or client projections (if any) from the original query to the new outer query. This became necessary because we could not alter the HAVING clause and the GROUP BY clause of the subquery of the pushdown directly, and had to create new SelectExpressions with our changes (but based on the work done by SelectExpressions.PushdownIntoSubquery()). We thus abandoned this approach.

We then tried to do all the work after RelationalQueryTranslationPostprocessor.Process(), where we can rewrite the expression tree in the good old and simple ExpressionVisitor way by calling .Update() methods. Unfortunately, we cannot make proper use of SelectExpressions.PushdownIntoSubquery() anymore and would have to completely reimplement it. That seemed like a maintenance nightmare to me, so we abandoned that approach as well.

I deliberately left our non-working solutions in the codebase for now, so that @roji and the team can take a look at were we failed to continue with our approaches. You guys can take a look the MySqlNonWorkingHavingExpressionVisitor in our Upgrade to EF Core 9.0.0 PR, which showcases both efforts and contains a couple of comments.

In the end, we split-up the work into a mutable tree part and an immutable tree part. In the mutable tree part, we just do the pushdown. In the immutable tree part, we then update the query. This is the cleanest approach that we came up with that does not use any hacks and is still maintainable. It is simple enough and contains the best of both worlds. The implementation is in MySqlHavingExpressionVisitor and we call it in MySqlQueryTranslationPostprocessor.Process():

var mySqlHavingExpressionVisitor = new MySqlHavingExpressionVisitor(_sqlExpressionFactory);

query = mySqlHavingExpressionVisitor.Process(query, usePrePostprocessorMode: true);

// Changes `SelectExpression.IsMutable` from `true` to `false`.
query = base.Process(query);

query = mySqlHavingExpressionVisitor.Process(query, usePrePostprocessorMode: false);

[ChrisJollyAU]

We first tried to accomplish this in the part of the query pipeline, where the SelectExpressions are generally still mutable (though some subqueries might already be not), so before their projections got applied in RelationalQueryTranslationPostprocessor.Process(). There were two issues with that approach: First, we could not safely figure out, if the original SelectExpression was mutable or not, without using reflection to access the internal property IsMutable (which we did not want to do). The second issue was that we could not figure out, how to copy over the projection mappings or client projections (if any) from the original query to the new outer query. This became necessary because we could not alter the HAVING clause and the GROUP BY clause of the subquery of the pushdown directly, and had to create new SelectExpressions with our changes (but based on the work done by SelectExpressions.PushdownIntoSubquery()). We thus abandoned this approach.

Yeah, I continued on with that approach and used reflection for all the internal stuff.

Now I just do

selectExpression = AddAliasManager(selectExpression);
selectExpression.PushdownIntoSubquery();