[release/6.0] Query: Match joined tables properly when lifting for group by aggregate

[smitpatel]

Earlier we only matched alias and skipped if alias matched. This could happen if the table name starts with same character.
Even if we do deeper match unwinding joins, we cannot match if the join is to subquery (which can be generated when target of navigation has query filter).

Solution:

When applying group by on SelectExpression, remember the original table count. Once Groupby has been applied we cannot add more joins to SelectExpression other than group by aggregate term lifting.

During lifting:

• If aliases of original tables in parent and subquery doesn't match then we assume subquery is non-grouping element rooted and we don't lift.
• If parent have lifted additional joins, one of them being a subquery join, then we abort lifting if the subquery contains a join to lift which is a subquery.
• If we are allowed to join after first 2 checks then:
  • We copy over owned entity in initial tables
  • We try to match additional joins after initial if they are table joins and joining to same table, in which case we don't need to join them again.
  • We copy over all other joins.

Resolves #27163

Description

After group by operator, when there are multiple aggregate terms in projection containing navigation expansion, when alias of joined tables match, then we generate invalid SQL if column referenced doesn't exist on other table which we didn't lift or incorrect result if column reference exists.

Customer impact

Customers will have their query throwing invalid SQL error or giving incorrect results.

How found

Customer reported on 6.0.1

Regression

No. The feature was introduced in 6.0 release only.

Testing

Added testing for user scenario and complex scenario in similar pattern.

Risk

Low risk. Logic added for lifting is pretty conservative. Also added quirk to revert to previous behavior.

[smitpatel]

Just realized, branch may not be open.

[AndriySvyryd]

AFAIK no new builds will be produced for 6.0.2

@dougbu Should we revert this just in case?

[dougbu]

@mmitche @wtgodbe @vseanreesermsft if builds have made it from build 20220120.1 to anywhere important, can that be undone❔

@smitpatel and @AndriySvyryd, I defer to the three above on whether reverting the change is needed. It's already correctly marked as going into 6.0.3 😃

[wtgodbe]

There was an open ingestion PR in aspnetcore that I just closed, we should be good as long as the subscription doesn't get triggered again (which would only happen if there was another build of EFCore).