Skip to content

DISTINCT wrap detection on Ss compares CAST-wrapped projection ref against unwrapped ORDER BY render #286

Description

@DJGosnell

Description

After PR #283 (fix for #274), a narrow inconsistency exists in the DISTINCT + ORDER-BY-on-non-projected wrap-detection path on SQL Server.

SqlAssembler.NeedsDistinctOrderByWrap and MayNeedDistinctOrderByWrap compare two SQL-fragment strings via HashSet<string>.Contains:

When an ORDER BY clause references a window-function projection that is in the SELECT list (e.g., Select(o => (o.OrderId, RowNum: Sql.RowNumber(...))).OrderBy(o => Sql.RowNumber(...))), the strings won't match on Ss. The wrap path will trigger even though the ORDER BY column is projected. The output remains functionally correct (the inner SELECT also goes through AppendProjectionColumnSql, so both strings are wrapped in the wrap output), just less efficient than necessary — an unnecessary derived-table wrap fires.

Location

Diagnostics

No automated test currently covers Distinct() + window-function projection + ORDER BY (window function) on Ss. The path is reachable but not exercised by the existing CrossDialectDistinctOrderByTests.

What Has Been Tried

The #274 fix (PR #283) added the cast wrap inside AppendProjectionColumnSql because that's the production SELECT-clause emit path. RenderProjectionColumnRef shares the same helper for byte-identical detection-vs-emission output (per the existing comment at SqlAssembler.cs:1349), which is correct for the inner-emit symmetry but breaks the cross-path symmetry against SqlExprRenderer.Render ORDER BY rendering on Ss only.

Gathered Information

  • SqlExprRenderer.Render is the canonical ORDER BY / WHERE / etc. expression renderer. It produces canonical un-wrapped SQL because the cast metadata lives on ProjectedColumn, not SqlExpr.
  • The wrap is dialect-conditional (Ss only), so PG/My/Lite are unaffected.
  • The only window function that's plausibly worth ordering by is ROW_NUMBER() / RANK() / DENSE_RANK() over the entity's natural order — relatively rare in practice but not pathological.

Suggested Approach

Two options worth weighing:

  1. Strip the CAST when comparing — in NeedsDistinctOrderByWrap / MayNeedDistinctOrderByWrap, render projection-column references via a separate helper that produces the un-wrapped form, used purely for comparison. The actual emit path stays wrapped.

  2. Carry the cast metadata onto the ORDER BY render — propagate RequiresSqlServerIntCast into the SqlExpr representation for column references, so SqlExprRenderer.Render can produce the same wrapped form. More intrusive.

Option (1) is local to the detection code and matches the spirit of the existing "rendered comparison" approach. Option (2) is more "correct" architecturally but expands #274's flag into a new subsystem.

Surfaced By

Code review of #274 fix (#283), finding #5.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions