Fix conditional clause mask model: honor conditional modifiers, structural cascade grouping (#307) - #322
Merged
Merged
Conversation
…ueId) ChainAnalyzer, AssembledPlan.GetClauseEntries, and the two ManifestEmitter correlators each re-derived site->bit positionally with subtly different skip rules; they agreed only by accident and misassigned bits when a chain sat wholly inside an if with one deeper conditional clause (swapped predicates in SQL variants, mask set by an unconditional clause). ConditionalTerm now carries the owning site's UniqueId and every consumer resolves by identity. Step 1 of #307. Regression test: Select_ChainInsideIf_BitAssignedToDeeperClauseOnly.
…ations Multi-variant dispatch previously indexed _sql[__c.Mask] unchecked: a mask the generator never enumerated produced either IndexOutOfRange or a null! gap entry dispatched as null CommandText into the provider. Both paths (plain _sql[] and the collection _sqlCache/switch path) now throw an actionable InvalidOperationException via Quarry.Internal.ThrowHelper.UnenumeratedMask. Pinned end-to-end: the else-if repro from #307 defect 2 now fails with the actionable guard message (pin is replaced by correct-execution assertions when structural cascade grouping lands). Step 2 of #307.
…gnment form
The carrier Timeout field is TimeSpan? with a DefaultTimeout fallback at the
terminal, so a conditional WithTimeout is already runtime-correct — its mask
bit only doubled the SQL variant table with byte-identical entries. The bit
loop now skips WithTimeout sites.
Also adds WithTimeout to UsageSiteDiscovery.IsKnownBuilderMethod: the
variable-disqualifier walk previously demoted any chain containing
'q = q.WithTimeout(...)' to QRY032 ('assigned from non-Quarry method'),
making the idiomatic reassigning form unusable.
Step 3 of #307.
Conditional .Limit()/.Offset()/.Distinct() sites received mask bits but the bits were dead: SQL rendering ignored them (LIMIT/DISTINCT baked into every variant), the interceptors never set the bit, and pagination params were bound unconditionally — silently truncating results when the branch was not taken, or emitting LIMIT 0 from the default carrier field. Now: PaginationPlan carries LimitBitIndex/OffsetBitIndex and QueryPlan a DistinctBitIndex; SqlAssembler gates LIMIT/OFFSET/DISTINCT (and the distinct-orderby wrap and SQL Server ORDER BY fallback) per mask variant, with batch rendering falling back to per-mask when any modifier is conditional; EmitPagination/EmitDistinct set the carrier mask bit; the terminal binds pagination parameters only when the bit is active; the MySQL bind-order validation expects the placeholder exactly in bit-set variants; ToDiagnostics marks pagination parameters conditional. Step 4 of #307. Cross-dialect coverage: literal/runtime conditional Limit, conditional Offset, conditional Distinct, Where+Limit 2-bit matrix.
Branch groups are now keyed by cascade identity (if/else-if/else chain or ternary head position) instead of condition text, with per-arm mask enumeration: all of an arm's bits enumerate together, mutually exclusive across arms, plus a no-arm mask when the cascade lacks a final else or has arms without chain sites. NestingContext carries GroupKey/ArmIndex/ArmCount/ HasFinalElse; nesting depth counts cascades, so flat else-if chains of any arm count are depth 1 and no longer demote. Ternary reassignment (q = flag ? q.Where(x) : q) — previously baked in unconditionally — is now a proper 2-arm cascade. The no-arm option enumerates first, keeping the base variant as the lead diagnostics/manifest entry.
After enumeration, ChainAnalyzer brute-forces all 2^totalBits masks against per-cascade constraints (empty intersection only when the cascade can take no represented arm, otherwise exactly one arm's complete bit set) and demotes the chain to RuntimeBuild (QRY032) if any reachable mask lacks a variant. Deliberately a separate walk from EnumerateMaskCombinations so one bug cannot hide in both. Core is internal and unit-tested with synthetic cascade shapes, including both historical defect-2 enumerations.
Root llm.md gains a Querying example block listing participating methods, else-if/ternary/multi-clause support, bit-free WithTimeout, and the 8-bit / 2-cascade-level limits; the Constraints bullet is updated to match. Generator llm.md rewrites the Conditional Clause Masking section around the cascade model (structural grouping, per-arm enumeration, SiteUniqueId bit matching, pagination/distinct gating, reachability validator, runtime dispatch guard). docs/articles/querying.md Conditional Branches section updated to cover the newly supported shapes.
F3/F11: cascades nested inside another conditional arm enumerate the no-arm mask (a final else cannot guarantee an arm when the whole cascade is skippable); validator derives ZeroAllowed from the same depth evidence. F4: chain sites inside a non-head (else-if) condition expression demote to QRY032 via NestingContext.UnanalyzablePositionKey unless the terminal shares the position. F6: a same-depth clause in a different arm than the terminal demotes instead of baking in unconditionally. F5: offset-without-LIMIT emits the dialect no-limit idiom (SQLite LIMIT -1, MySQL LIMIT 2^64-1) — fixes both pre-existing offset-only chains and limit-inactive variants manufactured by conditional Limit; DialectTests pins updated. F12: manifest variant labels dedupe by arm identity and mark final-else arms as else(<cond>); multi-line conditions flattened. F13: ClauseDiagnostic pagination params carry isConditional/bit metadata. F7-F10: new generation, cross-dialect execution, ToDiagnostics-consistency, bind-order, and throw-path tests. F2: workflow notes corrected. F15/F16 dismissed (packaging; intentional diagnostics changes).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes both verified critical defects in the conditional clause bitmask model, adds two defense-in-depth layers, and remediates 14 findings from the structured review (1 High among them).
Reason for Change
The conditional bitmask model produced silent wrong results or runtime crashes on documented usage patterns with zero compile-time signal:
.Limit()/.Offset()/.Distinct()were silently applied unconditionally — silent truncation when the branch wasn't taken; a runtime-valued limit defaulted to 0 and returned zero rows;ToDiagnosticsreported the clause inactive while the executed SQL contained it.else ifchains and multi-clause branches produced unenumerated masks — branch groups were keyed by condition text, so reachable mask values dispatchednullSQL into the provider at runtime.Impact
Limit/Offset/Distinctunderifnow render per-mask (LIMIT/OFFSET/DISTINCTgated per variant, including the SQL ServerORDER BY (SELECT NULL)injection and the DISTINCT ORDER-BY wrap), set their mask bit at runtime, bind pagination parameters only when active, and report consistently inToDiagnostics/manifest. MySQL positional?bind-order extraction handles the per-variant pagination slots.WithTimeoutno longer consumes a bit — itsTimeSpan?carrier field withDefaultTimeoutfallback is already runtime-correct, so a bit only doubled the variant table.if/else-if/elsechain or ternary) with per-arm enumeration — all of an arm's bits enumerate together, arms are mutually exclusive, and a no-arm mask is enumerated when the cascade lacks a final else, has arms without chain sites, or is itself nested inside another conditional arm.else ifchains of any arm count, multi-clause branches, and ternary reassignment (q = flag ? q.Where(x) : q) are now fully supported.ConditionalTermcarriesSiteUniqueId; all site→bit correlation is by identity, fixing a latent positional misassignment (chains partially inside anifhad baseline-depth sites stealing bits, producing swapped predicates).InvalidOperationExceptionviaQuarry.Internal.ThrowHelperinstead of a provider null-CommandText error), and a generation-time brute-force validator asserts reachable ⊆ enumerated masks, demoting to QRY032 on violation — deliberately a separate walk from the enumerator.OFFSET n;SqlFormattingnow emits the dialect no-limit idiom (LIMIT -1/LIMIT 18446744073709551615). Covers both plain offset-only chains (pre-existing bug) and the limit-inactive variants manufactured by a conditional Limit.else if (...)condition expression, or a clause in a different arm than its terminal within the same cascade, now demotes to QRY032 instead of silently baking wrong SQL.llm.md(participating methods + example), generatorllm.md(cascade model internals),docs/articles/querying.md.Tests: 3363 + 201 + 146 green across all four dialects (Docker-based MySQL/PostgreSQL/SQL Server included), up from 3281 at baseline — ~80 new tests covering every arm of the new model.
Plan items implemented as specified
All 7 plan steps: (1)
SiteUniqueIdbit identity, (2) runtime dispatch guard on both dispatch paths, (3) WithTimeout bit removal +IsKnownBuilderMethodfix for its reassignment form, (4) full conditional Limit/Offset/Distinct gating, (5) structural cascade grouping with per-arm enumeration, (6) generation-time reachability validator with unit-tested pure core, (7) documentation across all three surfaces.Deviations from plan implemented
RewriteMySqlBindMarkersrather than extendingBuildParamConditionalMapas the plan worded — pagination virtual slots never enter the conditional map. Intent achieved; the plan's missing test shape (conditional runtime-valued Limit + parameterized Where) was added during review remediation (F8).Gaps in original plan implemented
Review pass (16 findings: 1H/5M/10L → classified 7A/7B/2D) drove these beyond the plan:
if/elsenested inside an outer conditional arm enumerated masks{1,2}while runtime mask 0 was reachable (outer branch not taken) — and the validator was blind to it by construction. Fixed by forcing the no-arm option for cascades at relative depth > 1, in both the enumerator and the validator'sZeroAllowedderivation; pinned at generation and execution level including the dangling-else form.ThrowHelperTests), collection-path dispatch guard assertion, andToDiagnosticsconsistency for conditional Offset/Distinct/no-arm/multi-clause shapes.else(<cond>); pagination diagnostic parameters carry conditional-bit metadata insideClauseDiagnostic.Performance Considerations
Enumeration and validation are generation-time only (validator is brute force over ≤256 masks). Generated dispatch gains one bounds+null branch per multi-variant terminal execution; single-variant chains are unchanged. Variant tables shrink for conditional-WithTimeout chains (bit removed).
Security Considerations
Reviewed — no concerns. No user source text flows unescaped into generated code or SQL; the new emitted constructs use only integer bit indices;
GroupKeyis generator-internal.Breaking Changes
Consumer-facing (behavioral, all intended fixes — release notes should call these out)
q = flag ? q.Where(x) : q) previously baked the predicate in unconditionally; query results change whenflagis false. This shape was never an error before.Limit/Offset/Distinctpreviously applied always; untaken-branch executions now return the full/undeduplicated row set (this is defect 1's fix — the old behavior was silent truncation).WithTimeout) and previously-compiling degenerate shapes now demote to QRY032 (chain site inside an else-if condition; clause in a different arm than its terminal).Internal
ToDiagnostics/manifest surfaces:BranchKindis derived structurally (a loneifinside an else block now reportsIndependent); conditionalWithTimeoutclause entries reportIsConditional = false; manifest variant labels changed format. Snapshot-style assertions on these surfaces may need updates.Quarry.Internal.ThrowHelper(new public runtime API; generator ships inside the Quarry package, so versions always match).