Skip to content

feat: add struct column extraction helper#1875

Open
rbroggi wants to merge 4 commits into
ClickHouse:mainfrom
rbroggi:add-struct-columns-helper
Open

feat: add struct column extraction helper#1875
rbroggi wants to merge 4 commits into
ClickHouse:mainfrom
rbroggi:add-struct-columns-helper

Conversation

@rbroggi

@rbroggi rbroggi commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add clickhouse.StructColumns to extract deterministic insert column lists from structs using the same mapping rules as AppendStruct/ScanStruct
  • support ch tag options in struct mapping by using the name before the first comma
  • document using explicit column lists with AppendStruct when tables contain server-side DEFAULT columns
  • add a ClickHouse API example that uses StructColumns so omitted columns can use their DEFAULT values
  • harden StructColumns by de-duplicating returned column names and ignoring anonymous non-struct embeds without panicking

Fixes #1874
Related to #587

Testing

  • go test -race -count=1 -v . -run 'TestStructColumns|TestStructIdx|TestMapper'
  • make up && go test -race -count=1 -v ./tests -run TestAppendStruct && go test -race -count=1 -v ./examples/clickhouse_api -run TestStructColumns && make down
  • go test -count=1 ./... was also run earlier; it failed in existing JSON integration tests against clickhouse/clickhouse-server:latest (26.5.1) with timestamp parsing errors in TestJSONString, unrelated to this change.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Summary

This PR adds clickhouse.StructColumns(v any) ([]string, error), a helper that extracts a deterministic, ordered list of ClickHouse column names from a struct using the same ch-tag mapping rules as AppendStruct/ScanStruct. The motivating use case (issue #1874) is preparing a batch with an explicit column list so that server-side DEFAULT columns omitted from the struct can be filled by ClickHouse. The PR also refactors tag parsing into a shared chTagName helper (splitting on the first comma to support ch:"name,option"), adds a cached lookup, documentation, an example, and unit tests. The approach is sound — it mirrors the existing structIdx traversal, and the deterministic field-order output is exactly what an explicit INSERT (...) list needs. A few edge cases around the shared traversal are worth addressing before merge.

Must fix

(none)

Should fix

  • Duplicate column names are not de-duplicated. structColumns appends every mapped field, so two fields resolving to the same column name (e.g. two non-pointer embedded structs sharing a field, or a field whose tag collides with an embedded one) produce a list like ["a", "a"]. structIdx collapses these via its map (last-wins), so AppendStruct still succeeds while StructColumns yields an invalid INSERT INTO t (a, a). Since the whole point is to feed the result back into PrepareBatch, consider de-duplicating, or at least documenting that callers must avoid colliding field names.
  • Recursion can panic on a non-struct anonymous field. Recursion is guarded only by f.Type.Kind() != reflect.Ptr, then calls structColumns(f.Type) -> t.NumField(), which panics if an embedded field is a named non-struct type (e.g. an embedded type MyDuration time.Duration, or an embedded interface). This is the same latent issue as structIdx, but StructColumns is a new public API that now exposes the panic on otherwise-valid Go structs, and CLAUDE.md forbids panics in library code. Tightening the guard to f.Anonymous && f.Type.Kind() == reflect.Struct fixes it cheaply (and arguably should be applied to structIdx too).
  • Behavior change to structIdx via chTagName. Switching structIdx from f.Tag.Get("ch") to chTagName(f) changes existing AppendStruct/ScanStruct mapping for tags carrying options: ch:"name,opt" now maps to name (previously the literal name,opt), and ch:"-,opt" now omits the field (previously kept it). This is an improvement and matches the PR intent, but it is a behavior change on a heavily-used code path, so it is worth an explicit mention in the changelog/release notes.

Nits

  • nit: On a cache miss, StructColumns returns copyStrings(columns) even though columns is freshly allocated by structColumns(t) and never aliased to the stored copy (Store makes its own copy). The return copy is redundant; return columns, nil is safe and saves an allocation.
  • nit: structColumnsCache is a package-level global, whereas the existing structMap cache is a per-connection field (conn.go, conn_http_query.go, etc.). A global keyed by reflect.Type is fine (bounded, no eviction needed), but it diverges from the established pattern; a one-line comment on why it is global would help, and CLAUDE.md flags package-level globals as needing a clear reason.
  • nit: The example (examples/clickhouse_api/struct_columns.go) covers only the native API. StructColumns is protocol- and API-agnostic (pure reflection), so this is largely moot, but the repo review checklist asks for std-API coverage where relevant.

Verdict

Needs discussion — the core helper is well-designed, tested, and documented. The de-duplication and the non-struct-embed panic guard are small, worthwhile robustness fixes for a new public API; once those are addressed (or consciously waived) this is good to merge.

@rbroggi

rbroggi commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I pushed b48cc74 to address the recommended robustness fixes:

  • de-duplicated StructColumns output in first-occurrence order, so generated INSERT column lists do not contain duplicate identifiers
  • guarded anonymous embed traversal with Kind() == reflect.Struct in both StructColumns and structIdx, avoiding panics for embedded named scalar/interface types
  • removed the redundant cache-miss return copy and added a comment explaining why the StructColumns cache is package-level
  • expanded tests for duplicate columns and anonymous non-struct embeds

Re-ran:

  • go test -race -count=1 -v . -run 'TestStructColumns|TestStructIdx|TestMapper'
  • make up && go test -race -count=1 -v ./tests -run TestAppendStruct && go test -race -count=1 -v ./examples/clickhouse_api -run TestStructColumns && make down

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

🤖 Claude review

Adds a package-level StructColumns helper that extracts the ClickHouse column list from a struct using the same ch-tag mapping rules as AppendStruct/ScanStruct, plus tag-option support (ch:"name,opt"), a README section, and an example. Fixes #1874. The change is well-scoped, correct, and well-tested — I recommend approving.

Why it's sound:

  • structColumns collects the same name set as structIdx (which backs both AppendStruct and ScanStruct): identical skip rules for ch:"-", unexported non-anonymous fields, and pointer/non-struct anonymous embeds. Since AppendStruct maps by name, order need not match — every returned column is mappable.
  • The structIdx guard change (!= reflect.Ptr== reflect.Struct) is a real bug fix: the old code recursed into anonymous embedded non-struct/non-pointer types (e.g. an embedded named string or interface) and panicked in NumField(). Only Struct supports NumField, so no valid behavior is lost.
  • Refactoring structIdx onto the shared chTagName adds tag-option support to both mapping surfaces; grep found no existing code relying on the previous full-tag-string behavior.
  • Caching is race-safe: Load/Store both copy, and the first (cache-miss) call returns the freshly built slice while the cache holds a separate copy.

Test coverage is strong: dedup, anonymous-non-struct no-panic, cache-copy isolation, error inputs, tag options, plus a real native round-trip in the example test.

Blind spots: could not run the live example round-trip (no ClickHouse server here) to confirm server-side DEFAULT filling with a partial column list; the author reports it passing. StructColumns is protocol- and API-surface-agnostic (pure reflection), so native/HTTP and native/std parity is not a concern here.

Verdict: ✅ Approve

General findings

  • 💡 Nit — nit: returned column names are unescaped; document the identifier assumption
    StructColumns returns raw ch-tag names, and both the README snippet and struct_columns.go join them directly into the SQL (strings.Join(columns, ", ")). A tag such as ch:"my col" or a reserved word would produce an invalid INSERT.

    This is fine for the common case (identifier-safe tags), but a one-line note on the doc comment that names are returned verbatim (callers must backtick-quote non-identifier names) would keep the helper hard to misuse.

Inline comments are attached to the relevant lines. This summary updates in place on re-review.

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.

Expose struct column extraction helper for AppendStruct inserts

2 participants