Skip to content

Optimize: Make Join interceptors noops in PrebuiltDispatch chains - #8

Merged
DJGosnell merged 1 commit into
masterfrom
optimize/prebuilt-join-noop
Mar 15, 2026
Merged

Optimize: Make Join interceptors noops in PrebuiltDispatch chains#8
DJGosnell merged 1 commit into
masterfrom
optimize/prebuilt-join-noop

Conversation

@DJGosnell

Copy link
Copy Markdown
Member

Summary

Reason for Change

In fully analyzed (PrebuiltDispatch) chains, AddJoinClause<T>() performs unnecessary work: creating a JoinClause struct, cloning QueryState via ImmutableArray.Add, and setting FromTableAlias. None of this state is consumed by ExecuteWithPrebuiltSqlAsync, which uses a hardcoded SQL const string. Other chain steps (Where, Select) are already effectively noops in this path; Join was the only step doing unnecessary mutation.

Impact

  • Eliminates per-query allocation of JoinClause structs and ImmutableArray copies in prebuilt join chains
  • Zero behavioral change: prebuilt SQL path never reads JoinClauses or FromTableAlias
  • All 2,826 existing tests pass unchanged

Migration Steps

None — internal API only, no consumer-facing changes.

Performance Considerations

Reduces allocations in the hot path for prebuilt join queries. Each join previously allocated a JoinClause struct and cloned QueryState with ImmutableArray.Add; now it's a single constructor call with state passthrough.

Security Considerations

None.

Breaking Changes

  • Consumer-facing: None
  • Internal: GenerateJoinInterceptor now accepts optional PrebuiltChainInfo? and bool isFirstInChain parameters (source generator internals only)

🤖 Generated with Claude Code

Add AsJoined<T>() methods to QueryBuilder<T>, JoinedQueryBuilder<T1,T2>,
and JoinedQueryBuilder3<T1,T2,T3> that perform type-only conversion
without creating JoinClause structs or cloning QueryState via
ImmutableArray.Add. Update InterceptorCodeGenerator to emit AsJoined<T>()
instead of AddJoinClause<T>() when the chain is PrebuiltDispatch.

Closes #6

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@DJGosnell
DJGosnell force-pushed the optimize/prebuilt-join-noop branch from 746639d to 2856739 Compare March 15, 2026 00:25
@DJGosnell
DJGosnell merged commit 3cc8fdc into master Mar 15, 2026
DJGosnell added a commit that referenced this pull request Mar 20, 2026
…roaches

- Finding #4: Combine GetConstantValue + GetTypeInfo into single flow in
  TranslateCapturedValue, eliminating redundant semantic model query on
  non-constant captured expressions
- Finding #8: Add SymbolDisplayCache using ConditionalWeakTable to cache
  ToDisplayString results across invocations for the same ITypeSymbol,
  avoiding ~3,000 redundant string allocations for ~20 unique entity types
- Finding #10: Deferred update pattern for dictionary iteration in
  TokenizeCollectionParameters, avoiding .ToList() key copy by collecting
  pending updates and applying after iteration

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DJGosnell added a commit that referenced this pull request Mar 20, 2026
…roaches

- Finding #4: Combine GetConstantValue + GetTypeInfo into single flow in
  TranslateCapturedValue, eliminating redundant semantic model query on
  non-constant captured expressions
- Finding #8: Add SymbolDisplayCache using ConditionalWeakTable to cache
  ToDisplayString results across invocations for the same ITypeSymbol,
  avoiding ~3,000 redundant string allocations for ~20 unique entity types
- Finding #10: Deferred update pattern for dictionary iteration in
  TokenizeCollectionParameters, avoiding .ToList() key copy by collecting
  pending updates and applying after iteration

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DJGosnell added a commit that referenced this pull request Mar 20, 2026
* Optimize: Reduce allocations and redundant work in source generator hot paths

- Single-pass partitioning for site classification and scalar/collection param split
- BuilderKind enum replaces scattered string.Contains() checks across 5 files
- Pre-grouped staticFields lookup eliminates per-interceptor linear scans
- StringBuilder for SQL placeholder replacements in loops
- Mutable ExpressionTranslationContext.WithJoinedEntity() eliminates dictionary copies
- Array allocations instead of List for join entity type collections
- Single-pass string escaping in SyntacticClauseTranslator and shared helper in ExpressionSyntaxTranslator

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Optimize: Address remaining performance findings with alternative approaches

- Finding #4: Combine GetConstantValue + GetTypeInfo into single flow in
  TranslateCapturedValue, eliminating redundant semantic model query on
  non-constant captured expressions
- Finding #8: Add SymbolDisplayCache using ConditionalWeakTable to cache
  ToDisplayString results across invocations for the same ITypeSymbol,
  avoiding ~3,000 redundant string allocations for ~20 unique entity types
- Finding #10: Deferred update pattern for dictionary iteration in
  TokenizeCollectionParameters, avoiding .ToList() key copy by collecting
  pending updates and applying after iteration

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@DJGosnell
DJGosnell deleted the optimize/prebuilt-join-noop branch March 20, 2026 03:54
DJGosnell added a commit that referenced this pull request Mar 23, 2026
…lking

README.md:
- Replace InsertMany with InsertBatch API in Modifications section
- Add Batch Insert feature description
- Add variable-stored chain example

llm.md:
- Replace InsertMany with InsertBatch in Modifications examples
- Update builder interface list with IBatchInsertBuilder/IExecutableBatchInsert
- Add Batch Insert Pipeline, Variable-Walking Chain Unification sections
- Document VariableTracer consumers and key invariants

llm-generator.md:
- Add VariableTracer to Stage 1 Discovery table
- Add batch insert InterceptorKinds and BuilderKinds
- Add variable-walking as Key Design Decision #8

llm-usage.md:
- Replace InsertMany with InsertBatch examples

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DJGosnell added a commit that referenced this pull request Mar 23, 2026
…lking

README.md:
- Replace InsertMany with InsertBatch API in Modifications section
- Add Batch Insert feature description
- Add variable-stored chain example

llm.md:
- Replace InsertMany with InsertBatch in Modifications examples
- Update builder interface list with IBatchInsertBuilder/IExecutableBatchInsert
- Add Batch Insert Pipeline, Variable-Walking Chain Unification sections
- Document VariableTracer consumers and key invariants

llm-generator.md:
- Add VariableTracer to Stage 1 Discovery table
- Add batch insert InterceptorKinds and BuilderKinds
- Add variable-walking as Key Design Decision #8

llm-usage.md:
- Replace InsertMany with InsertBatch examples

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DJGosnell added a commit that referenced this pull request Mar 23, 2026
…62)

* Feat: Batch Insert API Redesign (#61)

Replace Values(T entity) chaining and InsertMany(IEnumerable<T>) with a
column-selector + data-provider pattern: InsertBatch(lambda).Values(collection).

New API separates column declaration (compile-time analyzable) from data
provision (runtime), making all batch insert chains carrier-eligible.

Breaking changes:
- IInsertBuilder<T>.Values(T entity) removed
- IEntityAccessor<T>.InsertMany(IEnumerable<T>) removed
- New: IEntityAccessor<T>.InsertBatch<TColumns>(Func<T, TColumns>)
- New: IBatchInsertBuilder<T> and IExecutableBatchInsert<T> interfaces
- New: BatchInsertCarrierBase<T> carrier base class

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix: Remove dead old-API code, add parameter limits, and fill test gaps

- Remove IsBatchInsert flag from RawCallSite (dead after batch insert redesign)
- Remove DetectBatchInsertInChain and TryExtractPropertyNamesFromInsertManyArgument
- Remove InsertMany branch from ExtractInitializedPropertyNames
- Clean up duplicate batch insert kind remapping in UsageSiteDiscovery
- Update stale doc comments referencing InsertMany
- Add MaxParameterCount (2100) guard to BatchInsertSqlBuilder
- Throw ArgumentException on empty entity collection instead of returning invalid SQL
- Fix BatchInsertSqlBuilder doc comments (removed nonexistent rowTemplate param)
- Replace benchmark TODO with actual InsertBatch benchmark
- Add ToSql cross-dialect tests for batch insert
- Add empty collection and runtime fallback tests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Removed old plans.

* Add VariableTracer: reusable primitives for walking variable declarations

Introduces VariableTracer static class with:
- TryResolveDeclarator: resolves identifier to its VariableDeclaratorSyntax
- GetInitializerExpression: extracts initializer from declarator
- WalkFluentChainRoot: walks fluent chain to deepest non-invocation receiver
- TraceToChainRoot: traces through variable assignments (up to 2 hops)
- IsBuilderType: consolidates builder type name matching
- TraceResult struct with Root, Hops, Traced, FirstVariableName

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Wire VariableTracer into ComputeChainId for chain unification

ComputeChainId now uses VariableTracer.WalkFluentChainRoot and
TraceToChainRoot to trace through variable assignments (up to 2 hops),
producing the same ChainId for all sites in a variable-split chain.
Also updates DetectVariableDisqualifiers to use WalkFluentChainRoot.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix ExtractBatchInsertColumnNamesFromChain to trace through variables

When the syntactic receiver walk doesn't find InsertBatch (because
the chain is split across variables), traces through variable
declarations to find the InsertBatch call in the initializer chain.
Extracts the helper WalkChainForInsertBatch for reuse.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Update AnalyzabilityChecker to use VariableTracer, add IsBuilderTypeName

- HasAnalyzableInitializer now uses VariableTracer.TryResolveDeclarator
  and traces through builder variables (up to 2 hops) to suppress QRY001
  for variable-stored chains that resolve to analyzable sources.
- Standardize builder type name checks: AnalyzabilityChecker line 148
  now uses VariableTracer.IsBuilderTypeName instead of inline pattern.
- Add IsBuilderTypeName to VariableTracer for short-name (INamedTypeSymbol.Name)
  checks, complementing IsBuilderType for display-string checks.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add tests for variable-stored chain unification (WIP)

- VariableStoredChainTests: integration tests for batch insert and query
  chains split across variables (1-hop and 2-hop patterns)
- UsageSiteDiscoveryTests: QRY001 suppression tests for variable-stored
  chains with analyzable initializers

Tests not yet verified against running build due to pre-existing
generator crash (Unsupported projection kind: Unknown).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix: Guard TraceToChainRoot against non-builder variables, simplify tests

TraceToChainRoot now checks that each identifier is a builder type
before hopping through its declaration. This prevents context variables
(like `db`) from being traced, which collapsed independent chains into
one (QRY033 error).

Simplified VariableStoredChainTests to batch-insert-only (removed query
variable tests that trigger Unsupported projection kind: Unknown).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add exception handler to EmitFileInterceptors, fix TraceToChainRoot guard

- EmitFileInterceptors now catches exceptions and reports them as QRY900
  diagnostics with full stack trace instead of crashing the generator.
- TraceToChainRoot checks that each identifier is a builder type before
  tracing through its declaration, preventing context variable collapse.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix chain unification: deepest variable name, context resolution, tests

- VariableTracer.TraceToChainRoot now records the deepest variable name
  (closest to chain origin), not the first. For exec→batch→Lite, this
  produces "batch" matching GetAssignedVariableName on the root statement.
- ResolveContextFromCallSite now uses VariableTracer.WalkFluentChainRoot
  and TraceToChainRoot for multi-hop context resolution, fixing cross-
  context interceptor conflicts (MyDb claiming Lite's call sites).
- Fixed test expectations: ToDiagnostics shows template SQL (1 row),
  use ToSql for multi-row expansion. QRY001 test uses local db variable
  instead of parameter (parameters are not analyzable by design).

All 2788 tests pass, 0 failures.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Treat QuarryContext parameters as analyzable, add query variable tests

- AnalyzabilityChecker: IParameterSymbol now checks IsQuarryContextType,
  matching the existing pattern for ILocalSymbol and IFieldSymbol. Methods
  receiving a QuarryContext parameter no longer emit QRY001.
- QuarryGenerator: skip reader delegate generation for ProjectionKind.Unknown
  instead of throwing. This prevents crashes when projection analysis fails
  (e.g., anonymous types in minimal compilations).
- Added Query_VariableStored_ToDiagnostics test proving query chains split
  across variables work end-to-end.
- QRY001 suppression test now uses parameter pattern (TestDbContext db).

All 2789 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add comprehensive tests for variable-stored chain unification

Integration tests (VariableStoredChainTests):
- Batch insert: 1-hop and 2-hop ExecuteNonQueryAsync (runtime execution)
- Query: single-column, entity, tuple Select projections via variable
- Query: two-hop variable chain (Where → Where → Select)
- Query: conditional Where with variable-stored chain (active/inactive)

Generator-level tests (UsageSiteDiscoveryTests):
- QuarryContext parameter fluent chain: no QRY001
- QuarryContext parameter with variable-stored chain: no QRY001
- Builder parameter (IQueryBuilder<T>): still emits QRY001

All 2798 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Docs: Update README and LLM references for batch insert + variable-walking

README.md:
- Replace InsertMany with InsertBatch API in Modifications section
- Add Batch Insert feature description
- Add variable-stored chain example

llm.md:
- Replace InsertMany with InsertBatch in Modifications examples
- Update builder interface list with IBatchInsertBuilder/IExecutableBatchInsert
- Add Batch Insert Pipeline, Variable-Walking Chain Unification sections
- Document VariableTracer consumers and key invariants

llm-generator.md:
- Add VariableTracer to Stage 1 Discovery table
- Add batch insert InterceptorKinds and BuilderKinds
- Add variable-walking as Key Design Decision #8

llm-usage.md:
- Replace InsertMany with InsertBatch examples

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Addresses review.md classifications from session 2:
- A/#4 Correctness: extend HasQuarryContextAttributeSyntactic with an
  AliasQualifiedNameSyntax branch so [global::QuarryContextAttribute]
  and extern-alias attribute forms pass the syntactic pre-filter.
- B/#1 Plan: drop RawSqlTypeInfo.FullyQualifiedResultTypeName — written
  but never read; ResultTypeName already carries the final display form
  (FQN for nested, short for not).
- B/#7 Tests: add alias-qualified attribute QRY044 coverage.
- B/#8 Tests: add struct-with-init-only-property QRY043 coverage.
- B/#9 Tests: add mixed RawSqlScalarAsync<int> + failing RawSqlAsync<T>
  test exercising the scalar branch in PipelineOrchestrator + FileEmitter.

3259 tests pass (+3 new).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Address review findings from _sessions/256-fix-sql-raw-select-projection/review.md:

- Fail-loud fallback (#3): AnalyzeProjectedExpression returns null for a
  methodName=="Raw" whose GetAggregateInfo returned null, preventing the
  generic type-info fallback from producing an empty-column ProjectedColumn
  that would regress the original #256 bug. AnalyzeInvocation emits a
  specific CreateFailed message for the single-column path.

- Captured-var typing (#4, #8, #16): RenderRawArgToCanonical tries
  ResolveScalarArgSql first for simple scalar args (identifiers, literals,
  captured members). This consults the semantic model for authoritative CLR
  types rather than the SqlExprParser default of "object", so a captured
  DateTime landed in ParameterInfo as "object" becomes "DateTime".

- Dialect-aware booleans (#5): FormatLiteralForProjection emits canonical
  {@boolt}/{@boolf} placeholders rather than per-dialect bool text.
  QuoteSqlExpression resolves the placeholder to TRUE/FALSE on PostgreSQL
  and 1/0 elsewhere. Because projection analysis runs at a fixed discovery
  dialect, emitting dialect-rendered text at analysis time would embed the
  wrong value in the cached SqlExpression.

- String-concat guard (#6): RenderRawArgNode's Add walker bails to null if
  either operand is a string/char typed literal or captured; the projection
  fails loudly rather than emitting "a + b" which is invalid on MySQL/SqlServer.
  Users who want string concat should write it in the template text.

- Validation refactor (#7): Replaced the throwaway RawCallExpr shell with a
  dedicated IsRawTemplateValid helper. Transient shell is now isolated to
  one helper with clear scope.

- T="object" guard (#21): TryExtractSqlRawTypeArg returns null rather than
  "object" when T is unresolvable. BuildSqlRawInfo then fails the projection
  loudly instead of letting ChainAnalyzer.TryResolveAggregateTypeFromSql
  misinfer a CLR type from SUM/MIN/MAX substrings in the user's template.

- Tests: Add Select_SqlRaw_BooleanLiteralArg_DialectAware (#5, #13) and
  Select_SqlRaw_CapturedVariable_TypeInferredFromSemanticModel (#4, #12, #14).

Review classifications + Action Taken recorded in
_sessions/256-fix-sql-raw-select-projection/review.md §Classifications.
All 3253 tests pass (3242 baseline + 11 Sql.Raw projection tests).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Review session 2 findings #4, #8.

The projection-arg walker used to emit `SqlRawExpr.SqlText` verbatim
when it reached a node the parser could not translate. Since
`SqlExprParser` falls through to `new SqlRawExpr(expression.ToString())`
for C# ternaries, unknown invocations, postfix unary (other than `!`),
array-creation without initializer, element access, and interpolated
strings, any of those inside `Sql.Raw<T>(template, arg)` would leak the
C# source text — e.g. `/* unsupported: C# ternary expression */` or
`u.Foo(x)` — into the generated SQL. This reproduces the exact class of
silent-wrong-SQL bug this PR fixes. The walker now returns null for
`SqlRawExpr`, forcing the caller to bail out.

The string-concat Add guard now also checks `ColumnRefExpr` operands
via a new `isStringColumn` delegate threaded alongside the existing
column resolver. Previously only direct string literals and captured
string locals were caught, so `Sql.Raw<string>("{0}", u.FirstName + u.LastName)`
would still emit `("FirstName" + "LastName")` — invalid on MySQL and
SqlServer. The guard now bails on any string-typed operand: literal,
captured, or column.

Both `RenderRawArgToCanonical` and `RenderRawArgToCanonicalJoined`
receive an `isStringColumn` delegate that closes over the column
lookup for the current context; `IsStringColumnRef` and
`IsJoinedStringColumnRef` implement the two contexts.
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Addresses review.md classifications from session 2:
- A/#4 Correctness: extend HasQuarryContextAttributeSyntactic with an
  AliasQualifiedNameSyntax branch so [global::QuarryContextAttribute]
  and extern-alias attribute forms pass the syntactic pre-filter.
- B/#1 Plan: drop RawSqlTypeInfo.FullyQualifiedResultTypeName — written
  but never read; ResultTypeName already carries the final display form
  (FQN for nested, short for not).
- B/#7 Tests: add alias-qualified attribute QRY044 coverage.
- B/#8 Tests: add struct-with-init-only-property QRY043 coverage.
- B/#9 Tests: add mixed RawSqlScalarAsync<int> + failing RawSqlAsync<T>
  test exercising the scalar branch in PipelineOrchestrator + FileEmitter.

3259 tests pass (+3 new).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
* Support Sql.Raw<T> in single-entity Select projections (#256)

Previously, Sql.Raw<T> used inside a Select tuple/DTO/object-init projection
silently rendered as an empty string literal in the generated SQL
(e.g., SELECT "OrderId", "" FROM "orders") because ProjectionAnalyzer fell
through to the generic fallback where columnName="" and the raw C# source
was placed in SqlExpression then later stripped.

Add a Raw case to GetAggregateInfo that parses each template argument via
SqlExprParser, walks the resulting SqlExpr tree with a projection-aware
renderer, and substitutes {0}/{1}/... template placeholders with canonical
{ColumnName} identifier placeholders (dialect-resolved later), @__proj{N}
parameter placeholders for captured runtime vars, and inline literals for
compile-time constants. Binary ops, unary ops, function calls, IS NULL, IN,
and LIKE expressions render inline so args like u.Price * 2 are supported.

The generic T type argument on Sql.Raw<T> determines the column CLR type;
template/arg count mismatches fail via RawCallExpr.Validate().

Covers tuple, DTO, object-initializer, and single-column projection forms
for single-entity Select (joined projections follow in a separate commit).

Task #1 of 3 in #256 workflow.

* Support Sql.Raw<T> in joined and single-column Select projections (#256)

Add a Raw case to GetJoinedAggregateInfo that resolves column references
to canonical {alias}.{ColumnName} placeholders using the per-parameter
lookup. The shared BuildSqlRawInfo helper and the SqlExpr tree walker from
Phase 1 are reused via a column-resolver delegate — only the column
resolution differs between single-entity and joined contexts.

Single-column projections (e.g., .Select(u => Sql.Raw<string>(...)))
already route through GetAggregateInfo via AnalyzeInvocation, so no
additional change is needed; joined single-column does the same via
AnalyzeJoinedInvocation → ResolveJoinedAggregate → GetJoinedAggregateInfo.

Add sanity tests for joined-tuple and single-column forms; the existing
column-reference sanity test from Phase 1 covers single-entity tuple form.

Task #2 of 3 in #256 workflow.

* Cross-dialect test coverage for Sql.Raw<T> in Select projections (#256)

Add 6 tests to complement the sanity tests from Phases 1+2, covering the
full surface of the fix:

- Multiple column references — Sql.Raw<string>("coalesce({0}, {1})", ...)
- Captured variable — verifies @__proj{N} local placeholders are remapped
  to dialect-specific parameter placeholders (@p0 / $1 / ?)
- Literal parameter — compile-time constant inlines as SQL literal
- No placeholders — template with no {N} substitutions passes through
- DTO initializer — new UserSummaryDto { ... = Sql.Raw<string>(...) }
- Binary op arg — u.UserId * 10 exercises the IR-based arg tree walker

All 9 tests (3 from Phases 1+2 + 6 added here) assert SQL output across
SQLite, PostgreSQL, MySQL, and SqlServer.

Task #3 of 3 in #256 workflow.

* Remediate review findings for Sql.Raw<T> Select projection (#256)

Address review findings from _sessions/256-fix-sql-raw-select-projection/review.md:

- Fail-loud fallback (#3): AnalyzeProjectedExpression returns null for a
  methodName=="Raw" whose GetAggregateInfo returned null, preventing the
  generic type-info fallback from producing an empty-column ProjectedColumn
  that would regress the original #256 bug. AnalyzeInvocation emits a
  specific CreateFailed message for the single-column path.

- Captured-var typing (#4, #8, #16): RenderRawArgToCanonical tries
  ResolveScalarArgSql first for simple scalar args (identifiers, literals,
  captured members). This consults the semantic model for authoritative CLR
  types rather than the SqlExprParser default of "object", so a captured
  DateTime landed in ParameterInfo as "object" becomes "DateTime".

- Dialect-aware booleans (#5): FormatLiteralForProjection emits canonical
  {@boolt}/{@boolf} placeholders rather than per-dialect bool text.
  QuoteSqlExpression resolves the placeholder to TRUE/FALSE on PostgreSQL
  and 1/0 elsewhere. Because projection analysis runs at a fixed discovery
  dialect, emitting dialect-rendered text at analysis time would embed the
  wrong value in the cached SqlExpression.

- String-concat guard (#6): RenderRawArgNode's Add walker bails to null if
  either operand is a string/char typed literal or captured; the projection
  fails loudly rather than emitting "a + b" which is invalid on MySQL/SqlServer.
  Users who want string concat should write it in the template text.

- Validation refactor (#7): Replaced the throwaway RawCallExpr shell with a
  dedicated IsRawTemplateValid helper. Transient shell is now isolated to
  one helper with clear scope.

- T="object" guard (#21): TryExtractSqlRawTypeArg returns null rather than
  "object" when T is unresolvable. BuildSqlRawInfo then fails the projection
  loudly instead of letting ChainAnalyzer.TryResolveAggregateTypeFromSql
  misinfer a CLR type from SUM/MIN/MAX substrings in the user's template.

- Tests: Add Select_SqlRaw_BooleanLiteralArg_DialectAware (#5, #13) and
  Select_SqlRaw_CapturedVariable_TypeInferredFromSemanticModel (#4, #12, #14).

Review classifications + Action Taken recorded in
_sessions/256-fix-sql-raw-select-projection/review.md §Classifications.
All 3253 tests pass (3242 baseline + 11 Sql.Raw projection tests).

* Record PR #262 in workflow state (#256)

* Back-step REMEDIATE → REVIEW for session 2 re-analysis (#256)

User declined to finalize PR #262 and requested a fresh full
review. Archived prior review.md to review-session1.md; workflow
phase reset to REVIEW.

* Make Sql.Raw projection walker fail loudly on unsupported args (#256)

Review session 2 findings #4, #8.

The projection-arg walker used to emit `SqlRawExpr.SqlText` verbatim
when it reached a node the parser could not translate. Since
`SqlExprParser` falls through to `new SqlRawExpr(expression.ToString())`
for C# ternaries, unknown invocations, postfix unary (other than `!`),
array-creation without initializer, element access, and interpolated
strings, any of those inside `Sql.Raw<T>(template, arg)` would leak the
C# source text — e.g. `/* unsupported: C# ternary expression */` or
`u.Foo(x)` — into the generated SQL. This reproduces the exact class of
silent-wrong-SQL bug this PR fixes. The walker now returns null for
`SqlRawExpr`, forcing the caller to bail out.

The string-concat Add guard now also checks `ColumnRefExpr` operands
via a new `isStringColumn` delegate threaded alongside the existing
column resolver. Previously only direct string literals and captured
string locals were caught, so `Sql.Raw<string>("{0}", u.FirstName + u.LastName)`
would still emit `("FirstName" + "LastName")` — invalid on MySQL and
SqlServer. The guard now bails on any string-typed operand: literal,
captured, or column.

Both `RenderRawArgToCanonical` and `RenderRawArgToCanonicalJoined`
receive an `isStringColumn` delegate that closes over the column
lookup for the current context; `IsStringColumnRef` and
`IsJoinedStringColumnRef` implement the two contexts.

* Propagate IsStaticField and ExpressionPath for Sql.Raw captured vars (#256)

Review session 2 findings #2, #9, #17.

`AddCapturedAsProjectionParameter` now mirrors the ParameterInfo
construction in `SqlExprClauseTranslator.ExtractParametersCore`
(SqlExprClauseTranslator.cs:88-92) so captured-variable metadata
propagates consistently across the Where-path and Select-projection
paths:

- `ExpressionPath = captured.ExpressionPath` — needed by the parameter
  binder to emit direct-path navigation code for deep captures like
  `obj.Inner.Field`.
- `IsStaticCapture = captured.IsStaticField` — controls whether
  UnsafeAccessor uses StaticField kind (null target) vs Field kind
  (func.Target).

Latent today: the walker path is only reached for captured variables
that are NOT direct identifiers or parameter-prefixed member accesses
(the fast-path in RenderRawArgToCanonical delegates those to
ResolveScalarArgSql). Remaining walker captures are rare (function-call
args, operator arms) and SqlExprParser never sets IsStaticField=true in
the current code. The fix aligns the projection path with the canonical
pattern so future parser changes or more complex Raw arg shapes don't
silently drop capture metadata.

* Tighten bool-literal detection and operator-table fallback (#256)

Review session 2 findings #6, #7.

`FormatLiteralForProjection` previously treated `"TRUE"`, `"true"`, and
`"1"` as truthy, but `SqlExprParser.ParseLiteral` only ever emits
`"TRUE"` or `"FALSE"` for bool literals (SqlExprParser.cs:270-274). The
extra checks were dead and added cognitive load — now only `"TRUE"`
maps to `{@boolt}`, everything else to `{@boolf}`.

`GetRawBinaryOperator` used to return `"?"` for any unmapped
`SqlBinaryOperator`, which is the MySQL parameter placeholder and
would parse into unrelated runtime behavior — the exact silent-
wrong-SQL failure mode this PR fixes. It now throws
`ArgumentOutOfRangeException`, so any future operator addition that
forgets to update the table will fail the build rather than emit
garbage SQL.

* Emit QRY029 for Sql.Raw template errors in Select projections (#256)

Review session 2 finding #1.

Where-path `Sql.Raw` calls surface template errors (placeholder/
argument count mismatch) as QRY029 compile-time errors via
`PipelineOrchestrator.CollectTranslatedDiagnostics`, because the raw
site's `Expression` is a `RawCallExpr` that runs `Validate()`. Select-
projection `Sql.Raw` calls were silent — `ProjectionAnalyzer.
BuildSqlRawInfo` validated via a transient `RawCallExpr` shell and, on
failure, discarded both the shell and the error, returning `(null, null)`.
The projection then failed analysis and the chain degraded to runtime
build, leaving no user-visible diagnostic.

`GetRawTemplateValidationError` now returns the `Validate()` message
(the same string the Where path shows). `BuildSqlRawInfo` records the
message via a thread-static accumulator modeled on `PipelineErrorBag`.
Public entry points in `ProjectionAnalyzer` drain the accumulator and
attach the messages to the returned `ProjectionInfo` via a new init-
only `SqlRawValidationErrors` property. `PipelineOrchestrator` emits
QRY029 per entry, scoped to the Select call's location.

Scope note: QRY029 is attached to the Select call location rather
than the individual `Sql.Raw` call site — less precise than the
Where path, but sufficient to locate the bad template in the lambda.

Added `ProjectionFailureReason.SqlRawValidationError` for future callers
that want to distinguish this degradation class; current pipeline
reads from `SqlRawValidationErrors` directly.

* Test remediation coverage for Sql.Raw Select projection (#256)

Review session 2 findings #11, #12, #14.

UsageSiteDiscoveryTests.cs (#11):
Three new QRY029 tests for projection-path Sql.Raw:
- Too many arguments
- Too few arguments
- Non-sequential placeholders ({0}, {2} skipping {1})
All assert QRY029 fires and the message matches the RawCallExpr.Validate
text, mirroring the existing Where-path tests.

CrossDialectMiscTests.cs (#12, #14):
- Select_SqlRaw_CapturedVariable_TypeInferredFromSemanticModel now also
  asserts Name, Value, IsCollection, IsEnum, and executes the query at
  runtime to verify the captured DateTime round-trips through the
  parameter binder (shallow-test remediation).
- Select_SqlRaw_Joined_MultipleArgs — joined projection with three
  args from a literal, u.UserName, and o.Status. Exercises
  ResolveJoinedColumnRefToPlaceholder for two distinct lambda params.
- Select_SqlRaw_Joined_WithCapturedVariable — joined projection with a
  captured int threshold in the Raw arg. Exercises
  IsScalarArgCandidateJoined and the fast-path delegation in the
  joined context.

Manifest regeneration reflects the three new cross-dialect queries.

* Share binary-operator table between SqlExprRenderer and Sql.Raw walker (#256)

Review session 2 finding #16 (scoped).

The Sql.Raw projection walker used to carry its own copy of the
SqlBinaryOperator → text table (`GetRawBinaryOperator`). Any future
operator addition would require updating both it and
`SqlExprRenderer.GetSqlOperator`, and any dialect-specific fix in the
renderer (e.g., the string-concat TODO at SqlExprRenderer.cs:290)
would bypass the walker. `GetSqlOperator` is now `internal` and
called directly from the walker.

The renderer's fallback was also tightened to throw
`ArgumentOutOfRangeException` rather than emit `"?"` — matching the
walker's stricter contract and preventing an unmapped operator from
silently rendering as the MySQL parameter placeholder.

Full consolidation of the remaining shape-level duplication
(IN/LIKE/IS NULL/function-call/unary emission) is out of scope: those
render paths in SqlExprRenderer invoke dialect-sensitive recursion
that the canonical walker must not enter. Tracked as a follow-up in
the walker's docstring: to share the rest, SqlExprRenderer would need
a canonical-projection output mode with delegate hooks for column
resolution, captured-value accumulation, and literal rendering.

* Record Action Taken for session 2 review findings (#256)

Populates the review.md Classifications table with a concise description
of how each A/B finding was addressed (or why D findings were left
alone). Session 2 remediation complete — ready to finalize.

* Save PR #262 body draft alongside session artifacts (#256)

* Update workflow.md for session 2 REMEDIATE completion (#256)

* chore: remove session artifacts before merge
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Addresses review.md classifications from session 2:
- A/#4 Correctness: extend HasQuarryContextAttributeSyntactic with an
  AliasQualifiedNameSyntax branch so [global::QuarryContextAttribute]
  and extern-alias attribute forms pass the syntactic pre-filter.
- B/#1 Plan: drop RawSqlTypeInfo.FullyQualifiedResultTypeName — written
  but never read; ResultTypeName already carries the final display form
  (FQN for nested, short for not).
- B/#7 Tests: add alias-qualified attribute QRY044 coverage.
- B/#8 Tests: add struct-with-init-only-property QRY043 coverage.
- B/#9 Tests: add mixed RawSqlScalarAsync<int> + failing RawSqlAsync<T>
  test exercising the scalar branch in PipelineOrchestrator + FileEmitter.

3259 tests pass (+3 new).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Addresses review.md classifications from session 2:
- A/#4 Correctness: extend HasQuarryContextAttributeSyntactic with an
  AliasQualifiedNameSyntax branch so [global::QuarryContextAttribute]
  and extern-alias attribute forms pass the syntactic pre-filter.
- B/#1 Plan: drop RawSqlTypeInfo.FullyQualifiedResultTypeName — written
  but never read; ResultTypeName already carries the final display form
  (FQN for nested, short for not).
- B/#7 Tests: add alias-qualified attribute QRY044 coverage.
- B/#8 Tests: add struct-with-init-only-property QRY043 coverage.
- B/#9 Tests: add mixed RawSqlScalarAsync<int> + failing RawSqlAsync<T>
  test exercising the scalar branch in PipelineOrchestrator + FileEmitter.

3259 tests pass (+3 new).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
* feat(generator): QRY043 diagnostic for un-materializable RawSqlAsync row types

Surface the real reason RawSqlAsync<T> row types fail to compile when T is a
positional record or has init-only properties. Previously authors saw cryptic
CS7036/CS8852 errors against generated code; now they see QRY043 naming their
type and the specific shape violation.

Detection runs in DisplayClassEnricher where the ITypeSymbol is already resolved
via the supplemental compilation. Emission is suppressed for affected sites so
QRY043 is the only error reported.

* feat(generator): support nested row-entity types in RawSqlAsync interceptors

Nested row types (row record declared inside an enclosing class) previously broke
the generator: it emitted `using <EnclosingType>;` which the compiler rejects
with CS0138. Fix tracks the nesting state on RawSqlTypeInfo and emits the
`global::`-prefixed fully qualified type name in generated bodies for nested
types so references resolve without a using directive. Namespace-level row
types still use their short name + using (unchanged codegen).

* feat(packaging): ship Quarry.targets auto-registering Quarry.Generated

Adds build/Quarry.targets to the Quarry NuGet package. It appends
Quarry.Generated to <InterceptorsNamespaces> so consumers no longer hit CS9137
for the Quarry-internal namespace they can't reasonably discover. Consumers
still add their own QuarryContext namespace — Phase 4's QRY044 analyzer
surfaces that gap at authoring time.

Also exposes InterceptorsNamespaces as a CompilerVisibleProperty so that
analyzer can read it from AnalyzerConfigOptions.

* feat(analyzers): QRY044 warns when QuarryContext namespace is missing from InterceptorsNamespaces

Surfaces the CS9137 project-setup gap at authoring time with the exact
<InterceptorsNamespaces> line to paste into the .csproj. Warning severity:
the build would fail with CS9137 anyway, so this is an earlier signal, not a
new error. Context classes in the global namespace are ignored because
Quarry.Generated (auto-registered by the shipped targets file) already
covers that path.

Descriptive diagnostic only — no CodeFixProvider, since the fix target is
the .csproj rather than a source document and standard Roslyn code fixes
can't reliably modify project files.

* docs: document row-entity shape requirements and interceptor opt-in behavior

Updates llm.md and Quarry.Generator/{README.md,llm.md} with:
- Revised InterceptorsNamespaces guidance: Quarry.Generated is now
  auto-registered by the shipped targets file, so the doc only prompts for
  the consumer's context namespace.
- New Row entity shape note under Raw SQL describing the parameterless ctor
  + public get/set property requirements, with QRY043 and the chain-query
  workaround for immutable shapes.
- Nested row types are explicitly called out as supported.
- QRY043 and QRY044 added to the diagnostic inventory tables.

* fix(generator): remediate review findings for #259

Review items addressed inline:
- #3 (B, Correctness): `CheckRowEntityMaterializability` now also rejects
  abstract classes and interfaces. CS0144 would otherwise fire against the
  generated `new T()` with no indication of which row type was at fault.
- #5 (B, Tests): adds a nested-row test driving the struct-reader fallback
  by using an expression SELECT list, exercising `SanitizeForIdentifier`
  and the FQN-in-`IRowReader<T>` path.
- #6 (B, Tests): strengthens the namespace-level-row regression to assert
  that `using TestApp.Rows;` IS emitted.
- #7 (B, Tests): adds a QRY044 test where `build_property.InterceptorsNamespaces`
  is entirely absent (null), confirming the diagnostic still fires.
- Plus QRY043 tests for abstract and interface row types.

Test totals: 3256 passing (+4 from Phase 5 green baseline).

* chore: record PR #260 in workflow.md + add pr-body.md session artifact

* docs: include abstract class and interface rejection in QRY043 docs

The REMEDIATE-phase extension of CheckRowEntityMaterializability added two
additional rejection cases (abstract classes and interfaces) that weren't
reflected in llm.md, src/Quarry.Generator/README.md, or
src/Quarry.Generator/llm.md. Brings the guide text and diagnostic inventory
tables in line with the actual QRY043 behavior.

* chore(session): record session 2 resume and populate review Action Taken

Resume bookkeeping only — no code changes. Closes the review classification
loop by filling Action Taken for B items #3/#5/#6/#7 with references to
commit be224dd; adds session log entry covering the resume.

* chore(session): back-step REMEDIATE -> REVIEW for re-analysis

* fix: remediate second-round review findings (A/#4, B/#1/#7/#8/#9)

Addresses review.md classifications from session 2:
- A/#4 Correctness: extend HasQuarryContextAttributeSyntactic with an
  AliasQualifiedNameSyntax branch so [global::QuarryContextAttribute]
  and extern-alias attribute forms pass the syntactic pre-filter.
- B/#1 Plan: drop RawSqlTypeInfo.FullyQualifiedResultTypeName — written
  but never read; ResultTypeName already carries the final display form
  (FQN for nested, short for not).
- B/#7 Tests: add alias-qualified attribute QRY044 coverage.
- B/#8 Tests: add struct-with-init-only-property QRY043 coverage.
- B/#9 Tests: add mixed RawSqlScalarAsync<int> + failing RawSqlAsync<T>
  test exercising the scalar branch in PipelineOrchestrator + FileEmitter.

3259 tests pass (+3 new).

* docs(pr): update PR body for second remediation round (A/#4, B/#1/#7/#8/#9)

* chore(session): log rebase on origin/master (4daf62d)

* chore(session): log second rebase on origin/master (e4354a4)

* chore(session): log third rebase on origin/master (95c4bbb + d2a6b1e)

* chore: remove session artifacts before merge
DJGosnell added a commit that referenced this pull request Apr 24, 2026
…tries (#265)

* fix(analyzers): route QRY044 input through a pipe-delimited alt property (#264)

Roslyn's editorconfig key-value regex treats `;` (and `#`) inside a value as
an inline-comment marker, truncating the captured value at the first such
character. Reading `build_property.InterceptorsNamespaces` directly therefore
under-reads any multi-entry list — including the normal case where
`Quarry.targets` auto-appends `Quarry.Generated` — and QRY044 fires for
namespaces that are in fact opted in.

Fix: expose a sibling MSBuild property `QuarryInterceptorsNamespaces` whose
value is the same list with `;` replaced by `|`. `|` is never a legal C#
namespace character, so the substitution is lossless and survives Roslyn's
parser intact. Computation is done in a target that runs before
`GenerateMSBuildEditorConfigFileCore` so the replacement sees the final
`InterceptorsNamespaces` value after every upstream .props / csproj /
.targets contribution has folded in. The analyzer prefers the pipe property
and falls back to the raw semicolon property only when the pipe one is
absent (consumers pinned to older Quarry versions).

Test coverage:
- Five pipe-path scenarios (covering leading `|`, target-not-last,
  duplicates, target absent).
- Two legacy-fallback scenarios.
- One precedence test (both properties set — pipe wins).

Part 1 of 3 for #264. Quarry.targets mirror (so consumers without
Quarry.Generator still get the alt property) and doc touch-ups follow in
separate commits.

* fix(Quarry.targets): also expose QuarryInterceptorsNamespaces (#264)

Mirror the QuarryInterceptorsNamespaces computation from Quarry.Generator.props
into Quarry.targets so every Quarry consumer picks up the alt property the
QRY044 analyzer needs — not just consumers who have Quarry.Generator in their
package graph. Quarry.Generator is usually pulled in transitively, but a
consumer who references only Quarry (e.g. with the generator and analyzer
shipped via other means, or pinned via PrivateAssets configurations) should
still get the working property.

Both the props-side and targets-side targets run BeforeTargets=
"GenerateMSBuildEditorConfigFileCore" and set the same property to the same
value, so having both imported is a no-op duplicate rather than a conflict.

Part 2 of 3 for #264.

* chore: remediate review findings for #264 (B/#8, B/#13, B/#22)

Review classifications addressed inline:
- B/#8 (Correctness): added comments in Quarry.Generator.props and
  Quarry.targets documenting reliance on the Roslyn-internal
  GenerateMSBuildEditorConfigFileCore target name and the silent-
  fallback-to-legacy failure mode if it's renamed in a future SDK.
- B/#13 (Tests): added EmptyPipeValueFallsBackToLegacyProperty test
  covering the case where QuarryInterceptorsNamespaces is exposed but
  empty-string and the analyzer must fall through to the semicolon
  property. 117 passing (+1 from 116).
- B/#22 (Consistency): moved the long XML comment out of the
  <ItemGroup> (where it sat adjacent to but didn't describe a
  CompilerVisibleProperty item) to above the <Target> it actually
  describes.

No behavior changes; all code-path coverage stays the same.

* chore(session): record PR #265 + add pr-body.md artifact

* chore: remove session artifacts before merge
DJGosnell added a commit that referenced this pull request Apr 29, 2026
- Finding #2: Delete the registry-driven joined-projection island
  (Analyze, AnalyzeJoined, AnalyzeJoinedExpression, AnalyzeJoinedSingleColumn,
  AnalyzeJoinedInitializer, AnalyzeJoinedTuple, ResolveJoinedProjectedExpression,
  ResolveJoinedColumn) and the orphaned BuildColumnLookup. Production discovery
  only reaches AnalyzeJoinedSyntaxOnly, so the deleted methods carried zero traffic
  yet duplicated the same legacy strict-Kind FK .Id branch the placeholder path
  was already fixed for. Inlined InferResultTypeFromSyntax (live callers remain).

- Finding #3: Clear IsRefKeyAccess=false before falling through to generic
  enrichment, so the generic block doesn't observe a stale flag if it matches
  a non-FK column for some other reason.

- Finding #8: Pin the wrap-suppression contract in FkKeyProjection_DiagnosticsShape
  with IsForeignKey == false and ForeignKeyEntityName == null assertions.

- Finding #11: Align IsRefKeyAccess enrichment with generic enrichment by carrying
  CustomTypeMapping and IsEnum from the FK column.

Finding #7 (test for the FK-not-found fall-through) is documented as a known
test-infrastructure follow-up — reproducing the path produces broken interceptor
code, so a runnable test requires source-generator unit-test scaffolding that
doesn't exist in this repo yet.
DJGosnell added a commit that referenced this pull request Apr 29, 2026
…st + production-path test (#274)

Post-review remediation for review findings:

#2 (A) — Gate refinement: tighten the flag-set criterion in ProjectionAnalyzer
to exclude LAG/LEAD/FIRST_VALUE/LAST_VALUE explicitly. Previously the gate
was 'HasOverClauseLambda && clrType == "int"', which would wrap a user-
written 'Sql.Lag(o.IntCol, ...)' on Ss despite the documented exclusion.
LAG/LEAD/FirstValue/LastValue inherit the source column's type, so on Ss
they return INT (not BIGINT) when the column is int — the wrap was a
defensive no-op functionally, but the manifest/emit would diverge from
other dialects which the spec promised would not happen. New helper
'IsIntReturningWindowFunction' centralises the rule used at all 4
ProjectedColumn construction sites.

#1 (B) — workflow.md Decisions updated to record that the production wrap
lives in SqlAssembler.AppendProjectionColumnSql, not the originally-planned
ReaderCodeGenerator.GenerateColumnList (the latter is unreferenced inside
the generator).

#3+#10 (B) — Added cross-dialect 'WindowFunction_Lag_IntColumn_NoCast'
regression test verifying no CAST is emitted on any dialect when the
window function is LAG over an int column.

#8 (B) — Added two production-path integration tests in
SqlServerWindowIntCastTests that drive SqlAssembler end-to-end via
QueryTestHarness + Prepare().ToDiagnostics(). Class-level XML doc updated
to document the two-layer test split (helpers + production path).

Refs #274.
DJGosnell added a commit that referenced this pull request Apr 29, 2026
…ption (#287)

* feat(generator): add RequiresSqlServerIntCast flag on ProjectedColumn (#274)

Pure model change. Default false; no upstream sets the flag yet.
Subsequent commits wire the flag into the column-list emitter and
into the projection analyzer at window-function emit sites.

Refs #274.

* feat(generator): emit CAST(... AS INT) on Ss when column flagged (#274)

GenerateColumnList and GenerateColumnNamesArray wrap the rendered SQL
expression with CAST(... AS INT) when ProjectedColumn.RequiresSqlServerIntCast
is set and the dialect is SqlServer. Other dialects emit unchanged SQL.

No upstream call site sets the flag yet, so behaviour is unchanged in this
commit; the next commit wires ProjectionAnalyzer to set the flag at
window-function emit sites.

Adds nine SqlServerWindowIntCastTests verifying the dialect-conditional
wrap and the flag-off no-op on every dialect.

Refs #274.

* feat(generator): wrap int-typed window projections with CAST(... AS INT) on Ss (#274)

ProjectionAnalyzer sets RequiresSqlServerIntCast on every ProjectedColumn
that is built from a Sql.* invocation with an OVER lambda whose resolved
ClrType is 'int'. Covered functions: ROW_NUMBER, RANK, DENSE_RANK, NTILE,
COUNT/SUM/AVG/MIN/MAX OVER (when target type is int). LAG, LEAD,
FirstValue, LastValue inherit the source column's type and are not flagged.

The wrap itself is applied in SqlAssembler.AppendProjectionColumnSql -
the production SELECT-clause emit path. Only fires when dialect ==
SqlServer; other dialects emit identical SQL to before.

The corresponding wrap in ReaderCodeGenerator.GenerateColumnList /
GenerateColumnNamesArray (added in the previous commit) covers the
unit-test entry points and any external consumers; both helpers already
appeared to be unreferenced inside the generator but are public API.

Existing CrossDialectWindowFunctionTests / CrossDialectSetOperationTests
will fail their Ss SQL-string assertions until the next commit updates
the expected strings; manifest snapshot is regenerated in the same step.

Refs #274.

* test: update Ss SQL assertions and regenerate Ss manifest for CAST(... AS INT) wrap (#274)

The generator now emits CAST(... AS INT) around int-typed window-function
projections on SQL Server. This commit syncs the affected expectations:

- 16 ss: assertions in CrossDialectWindowFunctionTests.cs
  (ROW_NUMBER, RANK, DENSE_RANK, NTILE, COUNT(*) OVER projections)
- 2 ss: assertions in CrossDialectSetOperationTests.cs
  (UnionAll_WithVariableWindowFunctionArg* — NTILE)
- src/Quarry.Tests/ManifestOutput/quarry-manifest.sqlserver.md
  (auto-regenerated by the build)

Other dialect manifests (postgresql/mysql/sqlite) are byte-identical;
only Ss SQL changes. Sum/Avg/Min/Max OVER on decimal columns are not
flagged and emit unchanged on every dialect.

Refs #274.

* test: un-skip Ss execution for nine window-function tests after #274 fix

With the CAST(... AS INT) wrap in place, SqlDataReader.GetInt32 succeeds
for ROW_NUMBER/DENSE_RANK/NTILE projections on SQL Server. Removes the
nine 'ss execution skipped — see #274' comments and adds matching
ssResults assertions mirroring the existing Pg/My execution paths.

Verified end-to-end against Testcontainers MsSql; full suite (3,373
tests across Quarry.Tests / Quarry.Analyzers.Tests / Quarry.Migration.Tests)
green.

Closes #274.

* fix(generator): tighten window-function-int-cast gate; add LAG-int test + production-path test (#274)

Post-review remediation for review findings:

#2 (A) — Gate refinement: tighten the flag-set criterion in ProjectionAnalyzer
to exclude LAG/LEAD/FIRST_VALUE/LAST_VALUE explicitly. Previously the gate
was 'HasOverClauseLambda && clrType == "int"', which would wrap a user-
written 'Sql.Lag(o.IntCol, ...)' on Ss despite the documented exclusion.
LAG/LEAD/FirstValue/LastValue inherit the source column's type, so on Ss
they return INT (not BIGINT) when the column is int — the wrap was a
defensive no-op functionally, but the manifest/emit would diverge from
other dialects which the spec promised would not happen. New helper
'IsIntReturningWindowFunction' centralises the rule used at all 4
ProjectedColumn construction sites.

#1 (B) — workflow.md Decisions updated to record that the production wrap
lives in SqlAssembler.AppendProjectionColumnSql, not the originally-planned
ReaderCodeGenerator.GenerateColumnList (the latter is unreferenced inside
the generator).

#3+#10 (B) — Added cross-dialect 'WindowFunction_Lag_IntColumn_NoCast'
regression test verifying no CAST is emitted on any dialect when the
window function is LAG over an int column.

#8 (B) — Added two production-path integration tests in
SqlServerWindowIntCastTests that drive SqlAssembler end-to-end via
QueryTestHarness + Prepare().ToDiagnostics(). Class-level XML doc updated
to document the two-layer test split (helpers + production path).

Refs #274.

* docs(session): record issue #286 in review.md classifications

* docs(session): record PR #287 in workflow.md state

* test: regenerate manifests post-rebase for #274 + LAG-int regression test

Rebase against origin/master (#284 landed) caused conflicts in all four
dialect manifests because both sides added entries. Resolved by taking
master's version then rebuilding to regenerate; result includes both
master's additions (from #284) and this branch's #274 + LAG-int
additions.

Refs #274.

* chore: remove session artifacts before merge
DJGosnell added a commit that referenced this pull request Apr 29, 2026
- Finding #2: Delete the registry-driven joined-projection island
  (Analyze, AnalyzeJoined, AnalyzeJoinedExpression, AnalyzeJoinedSingleColumn,
  AnalyzeJoinedInitializer, AnalyzeJoinedTuple, ResolveJoinedProjectedExpression,
  ResolveJoinedColumn) and the orphaned BuildColumnLookup. Production discovery
  only reaches AnalyzeJoinedSyntaxOnly, so the deleted methods carried zero traffic
  yet duplicated the same legacy strict-Kind FK .Id branch the placeholder path
  was already fixed for. Inlined InferResultTypeFromSyntax (live callers remain).

- Finding #3: Clear IsRefKeyAccess=false before falling through to generic
  enrichment, so the generic block doesn't observe a stale flag if it matches
  a non-FK column for some other reason.

- Finding #8: Pin the wrap-suppression contract in FkKeyProjection_DiagnosticsShape
  with IsForeignKey == false and ForeignKeyEntityName == null assertions.

- Finding #11: Align IsRefKeyAccess enrichment with generic enrichment by carrying
  CustomTypeMapping and IsEnum from the FK column.

Finding #7 (test for the FK-not-found fall-through) is documented as a known
test-infrastructure follow-up — reproducing the path produces broken interceptor
code, so a runnable test requires source-generator unit-test scaffolding that
doesn't exist in this repo yet.
DJGosnell added a commit that referenced this pull request May 6, 2026
…ves)

Addresses the four A/B findings from the REVIEW pass:

- Finding #1 (B): adds `Select_ProjectionMixedNestingDepths_OrderTotalAndItemTotal`
  to `CrossDialectNestedSubqueryTests` — closes the deep-projection-side gap
  that the sibling 1-level test left uncovered. The plan's 3-level
  Sum/Sum/Count variant exposed a generator projection-type resolver bug
  (nested int aggregates resolve as decimal, CS9144); tracked in #294 and
  worked around by keeping the new test on decimal-typed Sums.
- Findings #3 + #9 (B): adds `QRY075_UpdateSetAction_AssignToComputedColumn_Reports`
  and `QRY075_UpdateSetAction_AssignToWritableColumn_DoesNotReport` to
  `ComputedColumnDiagnosticTests` — provides the positive firing assertion
  that was missing after 1abdd63 trimmed the dead typed-lambda tests.
- Finding #7 (A): renames `ToAsyncEnumerable_BreakEarly_StopsAfterFirstRow`
  to `ToAsyncEnumerable_BreakAfterFirst_YieldsOrderedFirstRow` and rewrites
  the comment to admit the assertion is behavioral, not a streaming-vs-
  buffering proof.
- Finding #8 (A): tightens the `MultiContextPerFileTests` class docstring
  to state the test only proves file separation; carrier-name non-collision
  is attributed to C# `file`-scoped accessibility (language guarantee, not
  a generator behavior the test observes).

Manifest snapshots regenerate to pick up the new projection query shape.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize: Make Join interceptors noops in fully analyzed (PrebuiltDispatch) chains

1 participant