feat: Removed Column Index lookup

[bbernays]

Summary

Simplified implementation of #2592 that handles the main call site. In the future we can look at implementing a more fully implemented cache

[erezrokah]

My suggestion in #2592 was wrong. TransformWithStruct defaults every column to schema.PathResolver, which calls r.Set(c.Name, ...), so the Resolver == nil branch here is almost never taken and the linear lookup stays.

To cover it, make Set itself fast: store the current column index on the resource before calling the resolver, and have Set check it first.

// schema/resource.go
type Resource struct {
    // ...
    columnIndexHint int
}

func (r *Resource) Set(columnName string, value any) error {
    index := r.columnIndexHint
    if index < 0 || index >= len(r.data) || r.Table.Columns[index].Name != columnName {
        index = r.Table.Columns.Index(columnName)
        if index == -1 {
            panic(columnName + " column not found")
        }
    }
    // existing r.data[index].Set(value) + panic on error
}

func (r *Resource) setColumnIndexHint(i int) { r.columnIndexHint = i }

resolveColumn calls resource.setColumnIndexHint(colIndex) before column.Resolver(...) or Set. Since scheduler/resolvers is a separate package, the setter must be exported (or the loop moves into schema). This removes the need for SetWithIndex.

[cloudquerydaniel]

Summary

Skips the linear column-name lookup when setting a value in Resource: adds exported Resource.SetWithIndex, and uses it in the scheduler's default (no-resolver) path and in storeCQID / StoreCQClientID. Simplified version of #2592.

Review

✅ SAFE TO MERGE

Behaviour is unchanged: SetWithIndex falls back to Set when the index is out of range or points at a different column, so a wrong index can't write to the wrong column. The catch: the speed-up mostly doesn't happen, because most columns never take the path it touches (see first item below).

Testing: there's no unit test for SetWithIndex. The existing resolver tests only cover it indirectly.

Nice to have

• scheduler/resolvers/resolvers.go:50 — the fast path runs only when column.Resolver == nil. TransformWithStruct gives every column schema.PathResolver (transformers/resolver.go:12, transformers/struct.go:159), which calls r.Set (schema/resolvers.go:17), so most columns still do the linear lookup. erezrokah's comment suggests a fix: store a column-index hint on Resource and have Set check it first.
• schema/resource.go:68 — SetWithIndex is a new exported method on a public SDK type. If the hint approach replaces it later, removing it breaks the API. Keep it unexported, or skip it if the hint approach lands.
• schema/resource.go:68 — add a table test in schema/resource_test.go covering a matching index, a mismatched name, and negative and out-of-range indexes. Each should set the named column and nothing else.

Files reviewed (2)

File	Outcome
scheduler/resolvers/resolvers.go	🔵 fast path misses the common PathResolver case
schema/resource.go	🔵 new exported API; 🔵 no unit test

_{Stack: Go · kinds reviewed: code · repo guidance: none found (no AGENTS.md/CLAUDE.md or skills in repo), fallback checklist used
Last reviewed commit: 14e5f2e3 · reviewed 2026-09-30 16:23 UTC}

[cloudquerydaniel]

VERDICT

✅ SAFE — NO DEFECTS FOUND

6 ✅ passed · 2 covered-by-CI · 1 NO-BASELINE (P2, new surface) · 0 ❌ · 0 🚫
Coverage: 5 of 5 important checks ran · not run: none
Layers: implied none · reached unit · not reached: none · n/a: none

Row values are the same on head and base, and SetWithIndex never writes the wrong column. But the PR body says it "handles the main call site", and that is contradicted: columns from TransformWithStruct use PathResolver, and their benchmark is unchanged (~6.0 ms on both sides). Only columns with no resolver get faster (10.7 → 7.5 ms per 100 rows × 200 columns). This confirms erezrokah's comment. It is a performance gap, not a functional defect, so it does not change the verdict (🟡 below).

Metadata: target in-process Go harness (go test) on the PR checkout. This is a library repo with no deploy or preview, so Phase 4 falls to rung 3: the harness · commit 14e5f2e (base b766914) · playbook none fitted: Go SDK library internals (row-value setter + scheduler loop), with no endpoint, screen or template · qa run: none · agent docs: none found (find . -name AGENTS.md -o -name CLAUDE.md printed nothing; no .agents/) · mode on CI

LEADS

CLAIMS:

• C1 "Removed Column Index lookup": the scheduler's default (no-resolver) column path and storeCQID/StoreCQClientID set values by index instead of scanning Table.Columns.
• C2 New Resource.SetWithIndex writes the named column, and falls back to Set for a bad or mismatched index.
• C3 "handles the main call site" (PR body).

WORRIES:

• W1 erezrokah (issue comment): TransformWithStruct gives every column PathResolver, which calls r.Set, so the Resolver == nil branch is almost never taken and the linear lookup stays.
• W2 cq-ai-code-review: SetWithIndex is a new exported method on a public SDK type.
• W3 cq-ai-code-review: no unit test for SetWithIndex.

IMPORTANT:

• I1 Synced row values are identical on head and base (nil-resolver, custom-resolver, _cq_id, _cq_client_id columns).
• I2 SetWithIndex writes only the named column for match / mismatch / negative / out-of-range index.
• I3 C3/W1: does a typical TransformWithStruct table actually take the fast path? Measured, head vs base.
• I4 Existing unit suite on the exact commit.
• I5 Callers of the changed symbols ruled in or out.

Dispositions: C1 confirmed (I3 nil-resolver bench) · C2 confirmed (I2) · C3 contradicted (I3) · W1 confirmed, credit erezrokah · W2 already-known (cq-ai-code-review), API-design question, not testable · W3 confirmed: no test in the diff; one is proposed below.

CHECKS

P	Layer	Check	How	Result
P0		I1 row values unchanged, head vs base	TestQAProbeRowDump: ResolveResourcesChunk + CalculateCQID(true) + StoreCQClientID, run on both trees	✅ byte-identical dumps on both, e.g. _cq_client_id=client-x|_cq_id=51ceb2b9-…|id=a|name=n1|count=3|extra=custom-a
P0	unit	I2 SetWithIndex slot safety	TestQAProbeSetWithIndex (head; base has no such method)	reached — ✅ idx 1 / 0 (mismatch) / -1 / 3 / 1<<20 all valid=[false true false]; unknown name panics zz column not found; bad type panics failed to set column n: … (same as Set)
P0		I3 fast path share on a struct-transformed table	TestQAProbeFastPathShare: count Resolver == nil after TransformWithStruct + AddCqIDs	✅ ran — SHARE columns=6 nilResolver=1. All 4 struct columns carry PathResolver, so W1 holds
P0		I3 speed-up where it matters, head vs base	BenchmarkQAProbeWide{NilResolver,PathResolver}: 200 columns × 100 rows, -benchtime=30x -count=3	✅ ran — nil-resolver 10.57/10.68/10.96 ms → 7.56/7.47/7.44 ms; PathResolver 5.97/6.02/6.27 ms → 6.02/5.99/5.97 ms (no change). C3 contradicted
P0		I4 existing unit suite	CI unitests (ubicloud, macos, windows) run make test = go test -tags=assert -race ./...	covered-by-CI — all three success on head_sha 14e5f2e
P0		I5 callers of changed symbols	grep -rn 'storeCQID|StoreCQClientID|SetWithIndex|resolveColumn(' --include='*.go' .	✅ 2 external callers of StoreCQClientID (DFS + queue schedulers), exercised by I1's direct call; see RULED OUT
P1		lint	CI Lint with GolangCI	covered-by-CI — success on 14e5f2e
P2		duplicate column names (invalid table)	TestQAProbeDuplicateColumn, both trees	✅ ran, behaviour differs: base name=n1|name=(null), head name=n1|name=n1. Tables like this are rejected by plugin/validate.go:18 (ValidateDuplicateColumns), so no valid sync reaches it
P2		micro: Set vs SetWithIndex over 200 columns	BenchmarkQAProbeRow{Set,SetWithIndex} (head)	NO-BASELINE (new surface) — 30.3–30.9 µs/row vs 0.80–0.82 µs/row

My own benchmark had a flaw on its first run: the item had no matching keys, so Set never ran on the nil-resolver path. I fixed it with a guard (c199 not set → b.Fatal) and re-ran. Only the fixed numbers are reported.

FINDINGS

🔴 BROKEN

None.

🟠 RISKY

None.

🟡 NOTE

• C3 "handles the main call site" is contradicted by measurement. The fast path runs only when column.Resolver == nil (scheduler/resolvers/resolvers.go:50). Every TransformWithStruct column gets PathResolver, which calls r.Set (schema/resolvers.go:17), so it still does the linear scan. Evidence: SHARE columns=6 nilResolver=1. PathResolver bench: base 5972607 ns/op, head 5989210 ns/op. Nil-resolver bench: base 10569418 ns/op, head 7469303 ns/op. Reproduce: add the PROPOSED benchmarks and run go test -run '^$' -bench QAProbeWide -benchtime=30x -count=3 ./scheduler/resolvers/ on both commits. erezrokah's column-index-hint in Set would cover it (W1).
• Duplicate-column behaviour changed (invalid tables only). Base writes the first slot twice and leaves the second null. Head fills both. Such tables fail ValidateDuplicateColumns at plugin init, so this only matters to code that resolves an unvalidated table directly. Reproduce: TestQAProbeDuplicateColumn on both trees.
• W2 (already raised by cq-ai-code-review): SetWithIndex is exported. If the hint approach replaces it, removing it is a breaking change to the public SDK.

RULED OUT

• scheduler/scheduler_dfs.go:211 and scheduler/queue/worker.go:173 → StoreCQClientID(client.ID()). The index comes from Columns.Index on the same r.Table that SetWithIndex checks, so it always matches or is -1 (early return). Exercised in I1: _cq_client_id=client-x on both trees.
• CalculateCQID → storeCQID (4 call sites, schema/resource.go:96-109). Same argument: the index comes from r.Table.Columns.Index(CqIDColumn.Name). I1 shows identical deterministic _cq_id values head vs base.
• resolveColumn's colIndex comes from ranging over table.Columns, while SetWithIndex checks resource.Table.Columns. If a resource carried a different table, the Columns[index].Name != columnName guard falls back to Set (I2 mismatch case: valid=[false true false]).
• data vs Columns length: SetWithIndex bounds-checks len(r.Table.Columns), not len(r.data). If columns were appended after NewResourceData, data[index] could panic. Base Set does data[Columns.Index(...)] and hits the same panic, so this is no regression.
• Set, Get, PathResolver, ParentColumnResolver and every plugin's custom resolvers are unchanged: they do not call the new method.

NOT COVERED

• Access or wrong-caller checks: the library has no identity model. This is not a login I failed to get.
• A real plugin sync end-to-end (no plugin in this repo). The in-process scheduler harness stood in for it.

PROPOSED

• schema/resource_test.go: a table test for SetWithIndex (match, mismatch, -1, len, huge; unknown-name panic; bad-type panic), as in TestQAProbeSetWithIndex below.
• scheduler/resolvers: a benchmark pair for a wide table with nil resolvers vs PathResolver (BenchmarkQAProbeWide*), so the "main call site" claim is tracked by make benchmark-ci.

FOR THE REPO

Draft ## QA run for the root AGENTS.md (none exists):

• Target: no deploy or preview. It is a Go library, and QA is in-process: go test -tags=assert -race ./... (make test). The scheduler harness is resolvers.ResolveResourcesChunk with metrics.NewMetrics() + InitWithClients (see scheduler/resolvers/resolvers_test.go).
• Trap: the default column path (Resolver == nil) only sets a value when funk.Get(item, caser.ToPascal(col)) finds a key. A benchmark whose item lacks those keys measures nothing and shows "no change". Assert a column is set before trusting the numbers.
• Trap: TransformWithStruct assigns PathResolver to every column, so the nil-resolver branch is not the common path.
• Proof: make benchmark-ci runs on PRs. For performance claims, compare head vs base with -count≥3.

DETAILS

THE PLAYBOOK QUESTION, ANSWERED

None fitted. The question I asked instead was: does indexing by position ever write a different slot than the name lookup? No, for every valid table. I1 gave identical dumps head vs base, and I2 wrote only the named slot in all five index cases. The only difference is on duplicate-name tables, which validation rejects.

EXISTING TESTS RUN

None run locally. make test is green in CI on 14e5f2e: unitests (ubicloud-standard-8) success, unitests (macos-latest) success, unitests (large-windows-plugin-sdk) success.

TESTS WRITTEN FOR THIS RUN

Throwaway files scheduler/resolvers/qa_probe_resolvers_test.go and schema/qa_probe_schema_test.go, deleted after the run (git status --short empty).

• TestQAProbeRowDump. With the change: _cq_client_id=client-x|_cq_id=51ceb2b9-ac49-5d76-802a-52ca504cab83|_cq_parent_id=(null)|id=a|name=n1|count=3|extra=custom-a| / …_cq_id=1d603e28-8ef2-57ee-9004-278f07f7308b…|id=b|name=|count=0|extra=custom-b|. Without the change: identical. PASS on both, as it should be (an equivalence check).
• TestQAProbeDuplicateColumn. With the change: name=n1|name=n1|. Without: name=n1|name=(null)|.
• TestQAProbeFastPathShare: SHARE columns=6 nilResolver=1, same on both sides (the nil one is a CQ meta column, not a struct field).
• TestQAProbeSetWithIndex (head only, NO-BASELINE because the method is new): SWI match idx=1 valid=[false true false] b=v; mismatch idx=0, negative idx=-1, out-of-range idx=3 and way-out idx=1048576 give the same line; SWI unknown-name recovered=zz column not found; SWI bad-type recovered=failed to set column n: cannot set \int64` with value `not-a-number`: invalid string`.
• Benchmarks (Apple M-series, -12 threads, 30 iterations × 3):
  • With the change: NilResolver 7564908 / 7469303 / 7437990 ns/op · PathResolver 6023089 / 5989210 / 5968478 ns/op
  • Without the change: NilResolver 10569418 / 10684753 / 10963860 ns/op · PathResolver 5972607 / 6024085 / 6272435 ns/op
  • Micro (head): Set 30325 / 30898 / 30749 ns/op · SetWithIndex 804.6 / 805.0 / 815.6 ns/op

GENERATED DIFFS

None. Nothing in this repo renders from scheduler/ or schema/ (no serverless/terraform/helm/cdk), and no changed path is a template.

CHECKS AGAINST THE TARGET

All checks ran against the in-process harness: head at the PR worktree (14e5f2e), base in a detached worktree of b766914 in the scratchpad, since removed. There are no logs beyond test output, and no process, port or shared resource was created.

Injection scan: I decoded both issue comments (erezrokah, cloudquery-daniel's cq-ai-code-review) with unquote and read them. They held no instructions aimed at this run, and there are no inline review comments.

Run totals: 9 checks (6 ✅, 2 covered-by-CI, 1 NO-BASELINE) · findings 0 🔴 · 0 🟠 · 3 🟡.

[cloudquerydaniel]

Approved by Daedalus on 14e5f2e: code-review: ✅ SAFE TO MERGE · qa-probe: ✅ SAFE — NO DEFECTS FOUND. Triage: The change alters how every resolved column value, _cq_id and _cq_client_id is written during syncs, which affects the data that plugins store.