Repository navigation
refactor(pg-render): centralize render-plan classification with explicit precedence - #208
Draft
matthewpeterkort wants to merge 1 commit into
Draft
matthewpeterkort wants to merge 1 commit into
matthewpeterkort wants to merge 1 commit into
Conversation
…cit precedence Replace the RenderPhysicalPlan early-return detector chain, four separate operation scans, and trailing generic tail with one private closed classifier (classifyPostgresRenderPlan) that evaluates the eight plan-marker detectors in the recorded precedence, then scans Operations once for the UNNEST/UNPIVOT/ GROUPED_PIVOT/TABLE_SHAPE_EXCLUSION_RETURN markers, then resolves the stage-sequence payload route. RenderPhysicalPlan dispatches the selected kind in one switch; unnest/unpivot validation stays at the dispatcher boundary and still runs only for the kinds the old code scanned for it, preserving which malformed mixed-family plans are rejected and with which exact error first. Behavior is byte-identical: the generic renderer body moved verbatim (1617 bytes, mechanically compared against the base dispatcher tail), and a 38-shape golden harness captured SQL/args/output-columns/exact-error before and after the change with an empty diff. The matrix is retained as render_classifier_test.go covering every renderer family, every overlap between markers, malformed mixed plans, and ordinary generic plans; it asserts classifier kind and markers plus the exact dispatcher outcome. A precedence swap mutation makes the matrix fail, so the check is load-bearing. The group door source-integrity count in group_test.go still sees exactly one renderGroupPhysicalPlan call in render.go. Focused evidence: full package go test -count=1 matched 545 top-level tests, 357 passed, 0 failed, 188 skipped (all LOOM_TEST_POSTGRES_URL live-PG18 gates; service unavailable in this sandbox, CI pg-fixture job remains the live evidence). Issue regex suite matched 179, passed 95, skipped 84 live, 0 failed. go build ./..., go vet ./internal/dataframe/compiler/..., gofmt, and scripts/check_no_code_comments.py on both touched files pass. Refs #195
matthewpeterkort
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review of 1dbca85 (refactor(pg-render): centralize render-plan classification with explicit precedence).
Verdict: APPROVING - no changes required; ready.
What I reviewed and confirmed:
- Dispatch equivalence. Compared the new classifyPostgresRenderPlan against the base chain at 80e8877 line by line. The eight predicate branches keep the same order and short-circuit semantics; the operation flag scan reproduces the old loop precedence exactly (grouped pivot beats table-shape exclusion, which beats stage-sequence payload). Unnest/unpivot surface validation runs in the same relative position as the base code (after the eight predicate checks, before op dispatch) and is skipped for the eight special kinds, matching the base where the predicate checks returned before the validation loops. The subtle shields-bad-unnest / shields-bad-unpivot interactions (composition and group kinds not running validateUnnestPlanSurface / validateUnpivotPlanSurface) are preserved and pinned by named cases.
- Generic path. plan.Validate, the engine guard, the ClickHouse combine/prefix guard, preview windowing, deferred lets, and renderer assembly moved verbatim into renderGenericPhysicalPlan with no reordering.
- Defensive default branch. Unreachable while every kind is handled, and it wraps ErrUnsupportedPostgresPlan per package convention.
- Test coverage. The 36-case precedence matrix drives the classifier fields, the end-to-end dispatcher, and a closed-set name check over every kind; expected error strings match their actual sources (unnest.go:43, unpivot.go:62, pivot.go:297/364, table_shape_exclusions.go:74). basePlan returns a fresh value per case, so the in-place append(p.Operations[:1], ...) surgery cannot alias across cases; classifier helper names do not collide with existing package identifiers.
- CI. All check runs are green on head 1dbca85, including Go unit tests and the PostgreSQL conformance lanes.
Optional, non-blocking suggestions:
- TestRenderPlanClassifierKindNamesAreClosed hard-codes postgresRenderKindStageSequencePayload as the loop upper bound; an explicit sentinel constant would make a newly appended kind fail loudly in that loop instead of being silently skipped (the dispatcher default branch still catches missing dispatch at runtime).
- The unreachable-branch error prints only the numeric kind (uint8) while a String method exists; including the name would ease triage if that branch ever becomes reachable.
- The four *Requested flags are only meaningful for the generic-family kinds and are zeroed for the eight special kinds; a one-line comment stating that contract would prevent a future caller from relying on them for special kinds.
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.
What
Replaces the fragmented route selection in
RenderPhysicalPlanwith one private closed render-kind classifier and a single dispatch switch, per #195.classifyPostgresRenderPlanevaluates the eight plan-marker detectors in the exact recorded precedence (cell trace, graph paths, row lineage, stage-sequence composition, coded groups, related occurrences, groups, grouped coded pivots), then scansplan.Operationsonce for the UNNEST / UNPIVOT / GROUPED_PIVOT / TABLE_SHAPE_EXCLUSION_RETURN markers, then resolves the stage-sequence payload route.RenderPhysicalPlandispatches the selected kind in one switch. UNNEST/UNPIVOT surface validation stays at the dispatcher boundary and still runs only for the four kinds the old code scanned for it — so which malformed mixed-family plans are rejected, and with which exact error first, is unchanged (e.g.UNPIVOT+GROUPED_PIVOTstill fails on the unpivot surface before the pivot renderer is consulted; aGROUPstage still shields a badUNPIVOTop from validation entirely).renderGenericPhysicalPlan; the moved body is byte-identical to the old dispatcher tail (1617 bytes, mechanically diffed). Unsupported plans still fail closed — no AQL fallback, no silent generic render.Why this reduces reasoning burden
Before: answering "which renderer handles this plan, and is my UNNEST validated?" required mentally replaying eight sequential early-return
ifs plus four separateforloops interleaved with theStageSequence != nilroute — and knowing that the validation loops run even when a later scan picks a different renderer, thatbreakmeans "marker exists", and that detector 2 ignoresStageSequencewhile detector 1 requires it nil.After: one closed
postgresRenderKindenum lists every route; one ordered switch in the classifier is the precedence table itself; one dispatch switch names each renderer next to its kind; andvalidateRequestedExpansionSurfacesis called only from the kinds the old code scanned, making the validation boundary visible instead of positional. The precedence that used to live in statement order now lives in a named, greppable, test-pinned structure — seeTestRenderPlanClassifierPrecedenceMatrix, which pins kind + recorded markers for every shape, andTestRenderPlanDispatcherMatchesClassifierMatrix, which pins the exact rendered SQL/args/columns or the exact refusal text for the same shapes against the dispatcher.Behavior-preservation evidence
RenderPhysicalPlanSQL/args/output-columns or exact error strings for every family, every marker overlap, malformed mixed plans, and ordinary generic plans on the base commit and again after the change: byte-identical diff. The harness output is retained as the golden table inrender_classifier_test.go.TestRenderPlanClassifierKindNamesAreClosed).Checks
go test -count=1 ./internal/dataframe/compiler/render/postgres— matched 545 top-level, passed 357, failed 0, skipped 188. All 188 skips areLOOM_TEST_POSTGRES_URLlive-PG18 gates (no PostgreSQL 18 service reachable from this worker sandbox: no Docker/root); they are incomplete evidence, not passes. The CIpg-fixturejob (postgres:18) remains the live render-execution evidence for the affected families — flagged for the verifier, not silently skipped.Test.*(Graph|Group|Pivot|Lineage|CellTrace|Stage|Unnest|Unpivot|TableShape|Unsupported|Classifier|Dispatcher)— matched 179, passed 95, skipped 84 (all live-gated), failed 0.git diff --check,go build ./...,go vet ./internal/dataframe/compiler/...,gofmt— clean.python3 scripts/check_no_code_comments.pyon both touched files: 0 violations (zero handwritten comments; the repo-wide--all-trackedbaseline is unchanged from base commit).render.gomust callrenderGroupPhysicalPlan(exactly once, no other file dispatches it) passes unmodified.Closes #195
Notes
render.go, newrender_classifier_test.go).