Fix to #15873 - Query: Identifying columns in the case of distinct

Added validation step for AddCollectionJoin which checks that if subquery contains Distinct or GroupBy, the projection contains all identifying columns needed to correctly bucket the results during materialization.
Also making sure that identifying columns can be correctly propagated during pushdown and joining - if they are not we mark them as such (by removing identifying columns altogether), so that we can throw exception when these columns are actually needed.

Fixes #15873

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -700,8 +700,8 @@
   <data name="SequenceContainsNoElements" xml:space="preserve">
     <value>Sequence contains no elements.</value>
   </data>
-  <data name="ProjectingCollectionOnKeylessEntityNotSupported" xml:space="preserve">
-    <value>Projecting collection correlated with keyless entity is not supported.</value>
+  <data name="InsufficientInformationToIdentifyOuterElementOfCollectionJoin" xml:space="preserve">
+    <value>Not enough information to uniquely identify outer element in correlated collection scenario. This can happen when trying to correlate on keyless entity or when using 'Distinct' or 'GroupBy' operations without projecting all of the key columns.</value>
   </data>
   <data name="LogBatchExecutorFailedToRollbackToSavepoint" xml:space="preserve">
     <value>An error occurred while the batch executor was rolling back the transaction to a savepoint, after an exception occured.</value>
@@ -735,4 +735,7 @@
   <data name="MissingOrderingInSqlExpression" xml:space="preserve">
     <value>Reverse could not be translated to the server because there is no ordering on the server side.</value>
   </data>
+  <data name="MissingIdentifyingProjectionInDistinctGroupBySubquery" xml:space="preserve">
+    <value>Collection subquery that uses 'Distinct' or 'Group By' operations must project key columns of all of it's tables. Missing column: {column}. Either add column(s) to the projection or rewrite query to not use 'GroupBy'/'Distinct' operation.</value>
+  </data>
 </root>

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

@@ -1070,36 +1070,52 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
 
             var identifiers = _identifier.ToList();
             _identifier.Clear();
-            // TODO: See issue#15873
+
             foreach (var identifier in identifiers)
             {
                 if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
                 {
                     _identifier.Add((outerColumn, identifier.Comparer));
                 }
                 else if (!IsDistinct
-                    && GroupBy.Count == 0)
+                    && GroupBy.Count == 0
+                    || (GroupBy.Contains(identifier.Column)))
                 {
                     outerColumn = subquery.GenerateOuterColumn(identifier.Column);
                     _identifier.Add((outerColumn, identifier.Comparer));
                 }
+                else
+                {
+                    // if we can't propagate any identifier - clear them all instead
+                    // when adding collection join we detect this and throw appropriate exception
+                    _identifier.Clear();
+                    break;
+                }
             }
 
             var childIdentifiers = _childIdentifiers.ToList();
             _childIdentifiers.Clear();
-            // TODO: See issue#15873
+
             foreach (var identifier in childIdentifiers)
             {
                 if (projectionMap.TryGetValue(identifier.Column, out var outerColumn))
                 {
                     _childIdentifiers.Add((outerColumn, identifier.Comparer));
                 }
                 else if (!IsDistinct
-                    && GroupBy.Count == 0)
+                    && GroupBy.Count == 0
+                    || (GroupBy.Contains(identifier.Column)))
                 {
                     outerColumn = subquery.GenerateOuterColumn(identifier.Column);
                     _childIdentifiers.Add((outerColumn, identifier.Comparer));
                 }
+                else
+                {
+                    // if we can't propagate any identifier - clear them all instead
+                    // when adding collection join we detect this and throw appropriate exception
+                    _childIdentifiers.Clear();
+                    break;
+                }
             }
 
             var pendingCollections = _pendingCollections.ToList();
@@ -1328,10 +1344,16 @@ public Expression ApplyCollectionJoin(
             var innerSelectExpression = _pendingCollections[collectionIndex];
             _pendingCollections[collectionIndex] = null;
 
+            if (_identifier.Count == 0)
+            {
+                throw new InvalidOperationException(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin);
+            }
+
             if (splitQuery)
             {
                 var parentIdentifier = GetIdentifierAccessor(_identifier).Item1;
                 innerSelectExpression.ApplyProjection();
+                ValidateIdentifyingProjection(innerSelectExpression);
 
                 for (var i = 0; i < _identifier.Count; i++)
                 {
@@ -1421,16 +1443,14 @@ public Expression ApplyCollectionJoin(
             else
             {
                 var parentIdentifierList = _identifier.Except(_childIdentifiers).ToList();
-                if (parentIdentifierList.Count == 0)
-                {
-                    throw new InvalidOperationException(RelationalStrings.ProjectingCollectionOnKeylessEntityNotSupported);
-                }
 
                 var (parentIdentifier, parentIdentifierValueComparers) = GetIdentifierAccessor(parentIdentifierList);
                 var (outerIdentifier, outerIdentifierValueComparers) = GetIdentifierAccessor(_identifier);
                 var innerClientEval = innerSelectExpression.Projection.Count > 0;
                 innerSelectExpression.ApplyProjection();
 
+                ValidateIdentifyingProjection(innerSelectExpression);
+
                 if (collectionIndex == 0)
                 {
                     foreach (var identifier in parentIdentifierList)
@@ -1547,6 +1567,24 @@ public Expression ApplyCollectionJoin(
 
                 return result;
             }
+
+            static void ValidateIdentifyingProjection(SelectExpression selectExpression)
+            {
+                if (selectExpression.IsDistinct
+                    || selectExpression.GroupBy.Count > 0)
+                {
+                    var innerSelectProjectionExpressions = selectExpression._projection.Select(p => p.Expression).ToList();
+                    foreach (var innerSelectIdentifier in selectExpression._identifier)
+                    {
+                        if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column)
+                            && (selectExpression.GroupBy.Count == 0
+                                || !selectExpression.GroupBy.Contains(innerSelectIdentifier.Column)))
+
+                            throw new InvalidOperationException(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery(
+                                innerSelectIdentifier.Column.Table.Alias + "." + innerSelectIdentifier.Column.Name));
+                    }
+                }
+            }
         }
 
         private sealed class EntityShaperNullableMarkingExpressionVisitor : ExpressionVisitor
@@ -2173,14 +2211,24 @@ private void AddJoin(
                     .Remap(joinPredicate);
             }
 
-            if (joinType == JoinType.LeftJoin
-                || joinType == JoinType.OuterApply)
+            // if the subquery that is joined to can't be uniquely identified
+            // then the entire join should also not be marked as non-identifiable
+            if (innerSelectExpression._identifier.Count == 0
+                || _identifier.Count == 0)
             {
-                _identifier.AddRange(innerSelectExpression._identifier.Select(e => (e.Column.MakeNullable(), e.Comparer)));
+                _identifier.Clear();
             }
             else
             {
-                _identifier.AddRange(innerSelectExpression._identifier);
+                if (joinType == JoinType.LeftJoin
+                    || joinType == JoinType.OuterApply)
+                {
+                    _identifier.AddRange(innerSelectExpression._identifier.Select(e => (e.Column.MakeNullable(), e.Comparer)));
+                }
+                else
+                {
+                    _identifier.AddRange(innerSelectExpression._identifier);
+                }
             }
 
             var innerTable = innerSelectExpression.Tables.Single();

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs

@@ -1139,12 +1139,6 @@ public override Task Projecting_multiple_collection_with_same_constant_works(boo
             return base.Projecting_multiple_collection_with_same_constant_works(async);
         }
 
-        [ConditionalTheory(Skip = "Issue#17246")]
-        public override Task Projecting_after_navigation_and_distinct_works_correctly(bool async)
-        {
-            return base.Projecting_after_navigation_and_distinct_works_correctly(async);
-        }
-
         public override Task Reverse_without_explicit_ordering_throws(bool async)
         {
             return AssertTranslationFailedWithDetails(
@@ -1157,6 +1151,14 @@ public override Task Custom_projection_reference_navigation_PK_to_FK_optimizatio
             return base.Custom_projection_reference_navigation_PK_to_FK_optimization(async);
         }
 
+        [ConditionalTheory(Skip = "issue #17246")]
+        public override async Task Projecting_after_navigation_and_distinct_throws(bool isAsync)
+        {
+            await base.Projecting_after_navigation_and_distinct_throws(isAsync);
+
+            AssertSql(" ");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs

@@ -474,6 +474,14 @@ public override async Task Ordering_by_identifying_projection(bool async)
             AssertSql(" ");
         }
 
+        [ConditionalTheory(Skip = "issue #17246")]
+        public override async Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool isAsync)
+        {
+            await base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(isAsync);
+
+            AssertSql(" ");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.InMemory.FunctionalTests/Query/OwnedQueryInMemoryTest.cs

@@ -1,7 +1,9 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System.Threading.Tasks;
 using Microsoft.EntityFrameworkCore.TestUtilities;
+using Xunit;
 using Xunit.Abstractions;
 
 namespace Microsoft.EntityFrameworkCore.Query
@@ -14,6 +16,12 @@ public OwnedQueryInMemoryTest(OwnedQueryInMemoryFixture fixture, ITestOutputHelp
             //TestLoggerFactory.TestOutputHelper = testOutputHelper;
         }
 
+        [ConditionalTheory(Skip = "issue #19742")] 
+        public override Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool async)
+        {
+            return base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(async);
+        }
+
         public class OwnedQueryInMemoryFixture : OwnedQueryFixtureBase
         {
             protected override ITestStoreFactory TestStoreFactory => InMemoryTestStoreFactory.Instance;

test/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalTestBase.cs

@@ -1,7 +1,13 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
+using System.Linq;
+using System.Threading.Tasks;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel;
 using Microsoft.EntityFrameworkCore.TestUtilities;
+using Xunit;
 
 namespace Microsoft.EntityFrameworkCore.Query
 {
@@ -13,6 +19,41 @@ protected GearsOfWarQueryRelationalTestBase(TFixture fixture)
         {
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => AssertQuery(
+                async,
+                ss => ss.Set<Gear>()
+                    .OrderBy(g => g.Nickname)
+                    .Select(g => g.Weapons.SelectMany(x => x.Owner.AssignedCity.BornGears)
+                    .Select(x => (bool?)x.HasSoulPatch).Distinct().ToList())))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message);
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async)
+        {
+            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => AssertQuery(
+                async,
+                ss => ss.Set<Mission>()
+                    .Select(m => new
+                    {
+                        m.Id,
+                        grouping = m.ParticipatingSquads
+                            .Select(ps => ps.SquadId)
+                            .GroupBy(s => s)
+                            .Select(g => new { g.Key, Count = g.Count() })
+                    })))).Message;
+
+            Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message);
+        }
+
         protected virtual bool CanExecuteQueryString => false;
 
         protected override QueryAsserter CreateQueryAsserter(TFixture fixture)