Skip to content

refactor(pg-render): centralize render-plan classification with explicit precedence - #208

Draft
matthewpeterkort wants to merge 1 commit into
feature/postgres-backendfrom
ao/repo-simplify/195-pg-render-classifier
Draft

matthewpeterkort wants to merge 1 commit into
feature/postgres-backendfrom
ao/repo-simplify/195-pg-render-classifier

Conversation

@matthewpeterkort

Copy link
Copy Markdown
Collaborator

What

Replaces the fragmented route selection in RenderPhysicalPlan with one private closed render-kind classifier and a single dispatch switch, per #195.

  • classifyPostgresRenderPlan evaluates 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 scans plan.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 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_PIVOT still fails on the unpivot surface before the pivot renderer is consulted; a GROUP stage still shields a bad UNPIVOT op from validation entirely).
  • The generic path (plan validation, engine/ClickHouse/preview-window/deferred-let refusals, renderer construction) moved verbatim into 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.
  • Specialized renderers and their validators are untouched; each still owns its closed IR subset. No renderer was combined; support is never inferred from operation count.

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 separate for loops interleaved with the StageSequence != nil route — and knowing that the validation loops run even when a later scan picks a different renderer, that break means "marker exists", and that detector 2 ignores StageSequence while detector 1 requires it nil.

After: one closed postgresRenderKind enum lists every route; one ordered switch in the classifier is the precedence table itself; one dispatch switch names each renderer next to its kind; and validateRequestedExpansionSurfaces is 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 — see TestRenderPlanClassifierPrecedenceMatrix, which pins kind + recorded markers for every shape, and TestRenderPlanDispatcherMatchesClassifierMatrix, which pins the exact rendered SQL/args/columns or the exact refusal text for the same shapes against the dispatcher.

Behavior-preservation evidence

  • A 38-shape golden harness captured RenderPhysicalPlan SQL/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 in render_classifier_test.go.
  • Negative control: swapping the composition/coded-group detector precedence in the classifier makes the matrix fail — the check is load-bearing, and it also corrected two of my own wrong precedence expectations before commit.
  • All 12 classifier kinds are exercised by the matrix (TestRenderPlanClassifierKindNamesAreClosed).
  • SQL is unchanged by construction (zero edits to renderers/validators; SQL output including COLLATE and COUNT-DISTINCT shapes from feat(pg-render): register post-window child routes and SUM/COUNT/MIN/MAX set reducers #190 is untouched); the exact-error goldens in the matrix pin refusal text and first-failure precedence.

Checks

  • go test -count=1 ./internal/dataframe/compiler/render/postgres — matched 545 top-level, passed 357, failed 0, skipped 188. All 188 skips are LOOM_TEST_POSTGRES_URL live-PG18 gates (no PostgreSQL 18 service reachable from this worker sandbox: no Docker/root); they are incomplete evidence, not passes. The CI pg-fixture job (postgres:18) remains the live render-execution evidence for the affected families — flagged for the verifier, not silently skipped.
  • Issue focused regex 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.py on both touched files: 0 violations (zero handwritten comments; the repo-wide --all-tracked baseline is unchanged from base commit).
  • The group-13 source-integrity check (render.go must call renderGroupPhysicalPlan( exactly once, no other file dispatches it) passes unmodified.

Closes #195

Notes

  • 500 touched lines (627, of which 486 are the new evidence matrix): the rerunnable tooling is the golden capture/compare harness described above, and the moved-body correspondence for the generic tail is machine-verified byte equality.

  • Existing behavior tests: none modified. Only owned files touched (render.go, new render_classifier_test.go).
  • Native adversarial review intentionally not triggered here (coordinator-owned for this batch).

…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 matthewpeterkort left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

1 participant