GroupBy over a join with an uncorrelated Skip/Take inner silently returns wrong data via the driver-LINQ fallback

XMLWordPrintableJSON

    • Type: Bug
    • Resolution: Done
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • None
    • Dotnet Drivers
    • Not Needed
    • None
    • None
    • None
    • None
    • None
    • None

      Summary

      On the unmerged native-query branch (NativeQueryOngoing, epic EF-322), after the C# driver was bumped 3.9.0 to 3.10.0, a GroupBy over a join whose inner side carries an uncorrelated Skip/Take executes via the driver-LINQ fallback and returns silently wrong data on the default query mode. Before the bump the same query failed translation cleanly.

      This is a regression in user-facing correctness, not merely a test-baseline issue. On main these shapes raise a translation failure; on the branch they return wrong rows.

      Reproduction

      Unable to find source-code formatter for language: csharp. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml
      Orders.Join(Customers.OrderBy(c => c.City).Skip(10).Take(50), ...)
            .GroupBy(...)
            .Select(...)
      // returns 0 rows silently; the correct answer is 49
      

      Root cause

      Two facts compose. Neither alone produces the bug.

      1. The branch opened a doorway. The EF-344 slice changed TranslateGroupBy from returning null to returning a valid shaped query plus a call marking the query non-native. On main every GroupBy query fails EF translation outright and the driver-LINQ bridge is never invoked (measured: the driver is never asked, zero MQL emitted). On the branch, unsupported GroupBy shapes route to the driver-LINQ fallback and execute.
      2. Behind that doorway sits an already-ticketed driver defect, CSHARP-6017: driver 3.10 folds an uncorrelated inner's $sort/$skip/$limit into the correlated $lookup sub-pipeline. Upstream commit 58e05a0 already ticketed this and skipped 6 tests in NorthwindJoinQueryMongoTest for it.

      So this is a pre-existing driver wrong-data bug newly reached through a provider gate the branch opened.

      A competing hypothesis, that a native-stack preprocessor rewrote the tree so the driver would accept it, was measured false: at driver 3.9 the same branch source hands the driver the same tree and gets a clean rejection. The gate is the differentiator, not a tree rewrite.

      Evidence that the pipeline is wrong, not the materialization

      The emitted MQL, executed by the bare driver outside EF against the same seeded data, returns the same wrong row counts. Mechanism isolated by replacing $unwind with a size-counting group stage:

      { _id: 0, n: 100 }   // all 100 outer rows got an EMPTY $lookup
      

      The folded $skip: 10 runs against the per-order matched subset of at most one document, so every lookup yields nothing. Removing only $skip/$limit from the sub-pipeline changes the result from 0 rows to 52.

      Why the existing guard did not catch it

      The provider already has a hard-decline for this class of hazard, MarkGroupByFallbackUnsafe, but it has exactly one call site: the join translation, fired only when a join side is already grouped. It therefore covers group-then-join only. The Query area docs state the premise it rested on:

      > Join-then-group (an aggregate over a join result) falls back correctly and is unaffected.

      That premise is now measured false. The corruption depends on the join inner, not on the ordering of GroupBy relative to the join. That documentation line needs correcting.

      Observed test impact

      12 case-failures (6 methods, each async and sync) in NorthwindGroupByQueryMongoTest, EF10 spec suite, default Native mode. The class went from 509 passed / 0 failed to 497 passed / 12 failed.

      Count Kind
      3 silent wrong data (0 rows vs 133, 0 vs 29, 27 vs 20)
      1 now returns correct data; the test is over-strict
      1 MQL baseline drift
      1 changed exception type (a query object BSON-serialized rather than raising a translation error)

      Control matrix isolating the driver as the only variable. The test file is byte-identical across the rebase, so it is not the variable.

      Config Native stack Driver Result
      upstream main 58e05a0 no 3.10.0 509 pass / 0 fail
      branch pre-rebase yes 3.9.0 509 pass / 0 fail
      branch post-rebase yes 3.10.0 497 pass / 12 fail

      Chosen fix

      Hard-decline the affected shape provider-side, so a query that would route to the known-wrong driver path fails cleanly instead of returning wrong data.

      Chosen over two alternatives. Skipping the tests with a CSHARP-6017 reference would match upstream's precedent but leaves the user-facing wrong data in place. Widening the existing GroupBy guard to be order-insensitive over-declines, reddening currently-green tests whose fallback is correct.

      Under the chosen approach the three wrong-data tests pass as written, because their existing expectation of a translation failure holds again, and no test needs an XUnit skip. Per the project's versioning rubric, changing the exception type of an unsupported operation is not a breaking change.

      The gating risk is predicate precision: the detection must not decline queries that are currently correct. A calibration measurement is part of the design work, and if a sufficiently narrow predicate is not achievable the fallback position is skip-and-ticket.

      Non-goals

      • Fixing the driver defect. CSHARP-6017 already covers it and no new driver ticket is needed for the wrong-data cases.
      • Native join translation. That is separate planned work under EF-322 and would eventually remove the dependency on the driver-LINQ path for these shapes entirely.

      Notes

      Two further driver-side candidates were observed during diagnosis and are recorded for separate assessment, deliberately not filed as part of this ticket.

            Assignee:
            Arthur Vickers
            Reporter:
            Arthur Vickers
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: