perf: cache UDT struct field mappings to avoid per-row tag scanning (+small bug fix in marshalUDT)

[mykaul]

Summary

• Both marshalUDT and unmarshalUDT previously rebuilt a map[string]reflect.Value from struct tags on every call — allocating a map and scanning all struct fields per UDT column per row.
• Replaced with a sync.Map-based cache (udtFieldCache) keyed by reflect.Type, mapping CQL tag names to field indices. Built once per struct type, reused forever.
• Also fixes a latent bug in marshalUDT where reflect.TypeOf(value) was used instead of k.Type(), which would panic if marshalUDT were called directly with a pointer-to-struct.
• No API changes; no behavioral changes.

Benchmark (4-field UDT struct)

Path	Before	After	Improvement
Unmarshal ns/op	~324	~121	2.7x faster
Marshal ns/op	~462	~257	1.8x faster

Testing

All unit tests pass (-race -tags unit, 10665 tests across 42 packages). Includes new BenchmarkUnmarshalUDTStruct and BenchmarkMarshalUDTStruct.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR optimizes UDT (User-Defined Type) marshaling and unmarshaling by introducing a sync.Map-based cache for struct field resolution. Previously, both marshalUDT and unmarshalUDT rebuilt a tag-to-field lookup map on each call. The change scans struct types once, caches the mapping from cql tag names to struct field indices, and reuses that mapping to select fields by index. The implementation preserves existing fallback behavior for untagged fields and adds comprehensive tests covering cache isolation across types, concurrent stress scenarios, and performance benchmarks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is clearly related to the changeset, providing context about the problem (per-call tag scanning), the solution (sync.Map-based cache), bug fixes, performance improvements, and test coverage.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The PR title accurately describes the main change: caching UDT struct field mappings to avoid per-row tag scanning, plus mentions a bug fix in marshalUDT.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mykaul]

Review: UDT struct field-mapping cache (perf/udt-field-cache)

Deep review of the UDT field-cache change. Summary: the cache design is correct; I found one latent bug fix the change happens to introduce, and one gap (the PR added only benchmarks, no correctness unit tests). I added correctness tests (test-only) and verified everything passes under -race.

Findings (by severity)

LOW (resolved by the change — note for awareness): Pre-cache marshalUDT built its field map from t := reflect.TypeOf(value) without dereferencing pointers, while it dereferenced the reflect.Value. Passing a pointer-to-struct (*MyUDT) would call t.NumField() on a pointer type and panic (reflect: NumField of non-struct type *T). The new code uses t := k.Type() (post-deref), which is correct and fixes this. Added a regression test (TestUDTFieldCache_PointerToStruct).

LOW (test gap): The PR added two benchmarks but no correctness unit tests for the cache. Addressed below.

No issues found in:

• Cache key correctness: keyed by reflect.Type → unique per Go type, no cross-type collision. Same Go type reused for different UDTs is correct (mapping is struct-intrinsic; per-UDT element iteration handles differing element sets). Verified by tests.
• Field selection parity: cache stores field indices (not instance-bound reflect.Value) and resolves k.Field(idx) per call; the cql-tag scan iterates top-level fields exactly as before; the FieldByName fallback for untagged/promoted fields is unchanged. Tag precedence, missing-field skip, and extra-UDT-field skip are all preserved.
• Concurrency: sync.Map, build-once, stored map[string]int is read-only after store. Any duplicate concurrent build is last-writer-wins with identical content (benign). Confirmed clean under -race with a 32-goroutine stress test.
• Memory: unbounded only by the number of distinct struct types ever marshaled/unmarshaled — acceptable, as documented.
• *Map-based / UDTMarshaler / Unmarshaler / map[string]any targets: correctly bypass the struct path before the cache lookup. Unchanged.
• Unexported tagged fields: behavior identical to pre-cache (both store them; marshalUDT skips via CanInterface(), unmarshalUDT would panic via Addr().Interface() — pre-existing, not introduced here).

Changes I made (TEST-ONLY — no hot-path edits)

marshal_test.go (+sync import), 6 new unit tests:

• TestUDTFieldCache_DistinctTypesNoCollision — two types sharing cql names don't collide; A still correct after B cached.
• TestUDTFieldCache_SameTypeDifferentUDTs — one struct type used with two different UDT defs.
• TestUDTFieldCache_UntaggedFallback — FieldByName fallback for untagged exported field.
• TestUDTFieldCache_ExtraUDTFieldSkipped — UDT element with no struct field is skipped.
• TestUDTFieldCache_PointerToStruct — regression for the pointer panic noted above.
• TestUDTFieldCache_Concurrent — 32 goroutines × 200 iters, race coverage of cache build/read.

Test results

• go build ./... ✅ go vet ./... ✅
• go test -tags unit -race -run 'UDT|Marshal|Unmarshal|TestUDTFieldCache' . → 46 passed ✅
• Full go test -tags unit -race .: only failures are the known environmental PKI cases (TestSSLSimple*, TestShardAwarePortMocked*/with_TLS, TestPolicyConnPoolSSL — missing testdata/pki/ca.crt), identical to the origin/master baseline (11 failures on both).
  • Note: one full-suite -race run also flaked TestSchemaRefreshConcurrent/GetTable/after_table_invalidation ("expected 4 queries, got 8"). It passes in isolation, passed on a clean re-run, is unrelated to UDT marshaling, and does not reproduce on master full-suite runs reliably either → pre-existing concurrency flake, not a regression from this change.

Rebase

Rebased onto origin/master cleanly — no conflicts.

Push decision

Pushed (--force-with-lease) — my additions are test-only and all unit tests pass (policy: test/comment/doc-only changes may be pushed). I did not modify the hot-path cache logic.

Benchmark requirement (author's hot-path)

The cache itself is a hot-path change and per the agreed policy still needs benchmark validation before merge (machine was busy; not run here at scale). Recommended:

# baseline
git checkout origin/master
go test -tags unit -run='^$' -bench='UDTStruct' -benchmem -count=10 . | tee /tmp/base.txt
# branch
git checkout perf/udt-field-cache
go test -tags unit -run='^$' -bench='UDTStruct' -benchmem -count=10 . | tee /tmp/new.txt
benchstat /tmp/base.txt /tmp/new.txt

Smoke run (-benchtime=100x) on this branch: BenchmarkUnmarshalUDTStruct ≈ 219 ns/op, 2 allocs/op; BenchmarkMarshalUDTStruct ≈ 516 ns/op, 8 allocs/op — directionally consistent with removing per-call map build, but run benchstat against master for a real delta.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@marshal_test.go`:
- Around line 1551-1556: The benchmark input is wrapped with an extra
bytesWithLength call causing unmarshalUDT to treat the entire UDT payload as a
single field; remove the outer bytesWithLength and construct data as the
concatenation of the four already length-prefixed fields so unmarshalUDT will
iterate over all 4 fields (update the data assignment that currently calls
bytesWithLength(...) with nested bytesWithLength calls to instead concatenate
the individual bytesWithLength(...) results).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d265f3a0-1036-4afb-a750-b44a01b410a0

📥 Commits

Reviewing files that changed from the base of the PR and between bed2bb0 and f654955.

📒 Files selected for processing (2)

• marshal.go
• marshal_test.go

[mykaul]

Update — addressed CodeRabbit finding

Fixed the malformed benchmark input in BenchmarkUnmarshalUDTStruct (marshal_test.go).

• The UDT payload was double-wrapped with an extra outer bytesWithLength(...) envelope. Since unmarshalUDT reads each field directly with readBytes (no outer length prefix), the first read consumed the entire payload as field "first" and the loop exited after a single field — so the benchmark only measured ~1 of 4 fields.
• The input is now the bare concatenation of the four length-prefixed fields, so all 4 fields (first, second, third, fourth) are decoded each iteration.
• Verified against unmarshalUDT: with the old data only First was populated (Second/Third/Fourth stayed zero); with the fix all four decode correctly.

The fix was squashed into the perf commit that introduced the benchmark. go build, gofmt, and the full unit suite pass; rebased on origin/master and force-pushed.