Skip to content

perf(column): bulk-append fast paths for Array and LowCardinality#1863

Open
vvminashkin wants to merge 2 commits into
ClickHouse:mainfrom
vvminashkin:perf/array-lowcardinality-bulk-append
Open

perf(column): bulk-append fast paths for Array and LowCardinality#1863
vvminashkin wants to merge 2 commits into
ClickHouse:mainfrom
vvminashkin:perf/array-lowcardinality-bulk-append

Conversation

@vvminashkin

Copy link
Copy Markdown

Summary

When doing Append for Array or LowCardinality column types, the library calls AppendRow for each element, which in turn causes a new allocation per element. This PR fixes the issue by introducing a typed appendBulk methods, reducing the number of allocations per Append call to a constant.

goos: darwin
goarch: arm64
pkg: github.com/ClickHouse/clickhouse-go/v2/lib/column
cpu: Apple M3 Pro
                                                    │ /tmp/bench_baseline.txt │      /tmp/bench_optimized.txt      │
                                                    │         sec/op          │   sec/op     vs base               │
ArrayAppend_Int64_100x10-12                                     18.364µ ± 10%   2.578µ ± 2%  -85.96% (p=0.002 n=6)
ArrayAppend_Int64_1000x10-12                                    194.71µ ±  5%   25.74µ ± 5%  -86.78% (p=0.002 n=6)
ArrayAppend_Int64_100x100-12                                    163.62µ ±  3%   22.84µ ± 1%  -86.04% (p=0.002 n=6)
ArrayAppend_String_100x10-12                                    24.577µ ±  2%   4.708µ ± 7%  -80.85% (p=0.002 n=6)
ArrayAppend_String_1000x10-12                                   281.07µ ±  2%   49.80µ ± 3%  -82.28% (p=0.002 n=6)
ArrayAppend_NullableInt64_100x10-12                             14.964µ ±  1%   4.441µ ± 1%  -70.32% (p=0.002 n=6)
ArrayAppend_NullableInt64_1000x10-12                            161.76µ ±  3%   48.15µ ± 5%  -70.24% (p=0.002 n=6)
LowCardinalityAppend_String_1000x26-12                           40.59µ ±  8%   12.47µ ± 3%  -69.28% (p=0.002 n=6)
LowCardinalityAppend_String_1000x1000-12                         39.85µ ±  0%   11.68µ ± 3%  -70.69% (p=0.002 n=6)
LowCardinalityAppend_String_10000x26-12                          421.0µ ±  5%   125.1µ ± 6%  -70.29% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_1000x26-12                        22.20µ ±  1%   12.37µ ± 1%  -44.31% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_1000x26_WithNulls-12             21.647µ ±  2%   9.736µ ± 1%  -55.02% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_10000x26-12                       286.5µ ±  1%   133.1µ ± 7%  -53.53% (p=0.002 n=6)
geomean                                                          69.71µ         18.09µ       -74.05%

                                                    │ /tmp/bench_baseline.txt │       /tmp/bench_optimized.txt        │
                                                    │          B/op           │     B/op       vs base                │
ArrayAppend_Int64_100x10-12                                     12.430Ki ± 2%   9.370Ki ±  0%   -24.62% (p=0.002 n=6)
ArrayAppend_Int64_1000x10-12                                     391.6Ki ± 5%   130.2Ki ±  1%   -66.75% (p=0.002 n=6)
ArrayAppend_Int64_100x100-12                                     305.2Ki ± 5%   122.5Ki ±  1%   -59.87% (p=0.002 n=6)
ArrayAppend_String_100x10-12                                     25.97Ki ± 0%   18.68Ki ±  1%   -28.07% (p=0.002 n=6)
ArrayAppend_String_1000x10-12                                   1121.4Ki ± 2%   348.3Ki ±  2%   -68.94% (p=0.002 n=6)
ArrayAppend_NullableInt64_100x10-12                              4.446Ki ± 1%   9.676Ki ±  0%  +117.63% (p=0.002 n=6)
ArrayAppend_NullableInt64_1000x10-12                             291.1Ki ± 9%   169.9Ki ±  3%   -41.63% (p=0.002 n=6)
LowCardinalityAppend_String_1000x26-12                          20.407Ki ± 4%   1.520Ki ±  4%   -92.55% (p=0.002 n=6)
LowCardinalityAppend_String_1000x1000-12                        20.406Ki ± 1%   1.604Ki ±  4%   -92.14% (p=0.002 n=6)
LowCardinalityAppend_String_10000x26-12                          604.9Ki ± 2%   185.4Ki ±  5%   -69.35% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_1000x26-12                        2.639Ki ± 3%   1.544Ki ±  4%   -41.48% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_1000x26_WithNulls-12              2.511Ki ± 2%   1.188Ki ±  2%   -52.66% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_10000x26-12                       396.6Ki ± 2%   185.1Ki ± 10%   -53.33% (p=0.002 n=6)
geomean                                                          54.46Ki        21.76Ki         -60.04%

                                                    │ /tmp/bench_baseline.txt │         /tmp/bench_optimized.txt         │
                                                    │        allocs/op        │  allocs/op    vs base                    │
ArrayAppend_Int64_100x10-12                                   1100.000 ± 0%      3.000 ±  0%   -99.73% (p=0.002 n=6)
ArrayAppend_Int64_1000x10-12                                 11000.000 ± 0%      3.000 ±  0%   -99.97% (p=0.002 n=6)
ArrayAppend_Int64_100x100-12                                 10100.000 ± 0%      3.000 ±  0%   -99.97% (p=0.002 n=6)
ArrayAppend_String_100x10-12                                  1100.000 ± 0%      3.000 ±  0%   -99.73% (p=0.002 n=6)
ArrayAppend_String_1000x10-12                                11000.000 ± 0%      3.000 ±  0%   -99.97% (p=0.002 n=6)
ArrayAppend_NullableInt64_100x10-12                            100.000 ± 0%      3.000 ±  0%   -97.00% (p=0.002 n=6)
ArrayAppend_NullableInt64_1000x10-12                          1000.000 ± 0%      3.000 ±  0%   -99.70% (p=0.002 n=6)
LowCardinalityAppend_String_1000x26-12                          1.000k ± 0%     0.000k ±  0%  -100.00% (p=0.002 n=6)
LowCardinalityAppend_String_1000x1000-12                      1000.000 ± 0%      1.500 ± 33%   -99.85% (p=0.002 n=6)
LowCardinalityAppend_String_10000x26-12                         10.00k ± 0%      0.00k ±  0%  -100.00% (p=0.002 n=6)
LowCardinalityAppend_StringPtr_1000x26-12                        0.000 ± 0%      0.000 ±  0%         ~ (p=1.000 n=6) ¹
LowCardinalityAppend_StringPtr_1000x26_WithNulls-12              0.000 ± 0%      0.000 ±  0%         ~ (p=1.000 n=6) ¹
LowCardinalityAppend_StringPtr_10000x26-12                       0.000 ± 0%      0.000 ±  0%         ~ (p=1.000 n=6) ¹
geomean                                                                     ²                 ?                      ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

```

@CLAassistant

CLAassistant commented May 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@vvminashkin
vvminashkin force-pushed the perf/array-lowcardinality-bulk-append branch from f22c4e2 to 730de76 Compare May 21, 2026 14:51
@github-actions

Copy link
Copy Markdown

Summary

Adds bulk-append fast paths to Array.Append (depth-1) and LowCardinality.Append. For Array, a generated [][]T type switch flattens all rows into one inner col.values.Append(flat) call, eliminating the per-element reflection loop and amortizing the inner column's loop preamble. For LowCardinality, typed []string / []*string fast paths bypass reflection entirely and reuse the existing dictionary map. The reported wins are substantial (60-80% latency, near-zero alloc/op for several cases) and the approach is sound — the existing Append code path was effectively never hitting the typed appendRowPlain dispatch because Append passed value.Index(i) (a reflect.Value) into AppendRow, so the type switch always fell through to the reflection branch.

Must fix

  • No correctness tests for the new bulk paths. The new files only add benchmarks; there are no unit/integration tests that compare bulk-path output against the row-by-row path. Several non-trivial behaviours need explicit coverage:

    • The errors.As(err, &convErr) fallback in appendBulkPlain / appendRowPlain (e.g. [][]int into Array(Int64) — the inner Int64.Append([]int) returns ColumnConverterError, so the retry-via-AppendRow path with reflect.Convert must run).
    • Nullable inner column with mixed nil / non-nil pointers ([][]*int64 into Array(Nullable(Int64))).
    • LowCardinality(Nullable(String)) via []*string with embedded nil entries — both the null-key-0 path and dictionary lookup.
    • Round-trip equivalence: encode a column populated via the new bulk path and assert the encoded bytes (or decoded values) match the same data appended row-by-row.

    Without these, a future refactor of an inner column's Append can silently break bulk semantics with no failing test.

Should fix

  • appendBulkPlain appends offsets before attempting the inner Append, then assumes any error is a ColumnConverterError whose contract is "no state mutation." Two issues here:

    1. The contract isn't documented anywhere on the Interface.Append side — it's an emergent property of the current implementations. Worth either documenting it on Interface.Append in lib/driver/driver.go ("Append must not mutate column state when returning ColumnConverterError") or writing the bulk path defensively (append offsets only after the inner Append succeeds, or after the per-item retry completes).
    2. On a non-ColumnConverterError return (or on a type mismatch like [][]string passed to Array(Int64)), the bulk path leaves N dirty offsets in the Array even though zero values landed in the inner column. The old per-row path left at most one dirty offset. Callers are expected to discard a column on error, but worth flagging — and a comment near col.appendOffset calls noting "state is left inconsistent on error; caller must discard" would help future readers.
  • appendBulkPlain and the modified appendRowPlain / appendNullableRowPlain dispatch purely on input type, without consulting col.values' actual type. That's fine when the inner column accepts or cleanly rejects the flat slice, but it puts more weight on every inner Append implementing the "reject up-front, never partially mutate" contract. A short comment on the new helpers explaining this assumption (and on the codegen template) would make the invariant visible.

  • LowCardinality.initIndex silently discards errors from col.index.AppendRow(nil) — inherited from the original code, but the extraction into a named helper is a good moment to either propagate the error or comment why it's safe to ignore (e.g., "AppendRow(nil) is infallible for all dictionary-eligible base types"). CLAUDE.md is explicit about not swallowing errors.

  • LowCardinality fast path only covers []string / []*string. LowCardinality(Int64), LowCardinality(FixedString(N)), and LowCardinality(Date) are also common in the wild. Either generate this dispatcher from the same codegen as array_gen.go (so future types stay in sync), or leave a TODO/issue noting it as a follow-up. Not a blocker.

  • Benchmarks include col.Reset() in the timed section. Reset() resets several sub-columns and reallocates the dictionary map, so its cost is mixed into the per-iteration number every 1024 iters. Wrap it in b.StopTimer() / b.StartTimer() for cleaner numbers.

Nits

  • nit: appendNullableRowPlain (lib/column/array.go:172-178) has a stray blank line before the closing }.
  • nit: appendNullableBulkPlain (lib/column/array.go) has a stray blank line between var convErr *ColumnConverterError and the if !errors.As(...) check; tighten to a single declaration-and-check, or just inline var convErr *ColumnConverterError; if !errors.As(err, &convErr) { ... }.
  • nit: The dispatcher method (col *Array) appendBulkPlain(v any) and the generic helper appendBulkPlain[T any](col *Array, arr [][]T) share a name. Legal in Go, but consider renaming the helper (e.g. bulkAppendValues) to make stack traces and grep less ambiguous.
  • nit: arrayBenchResetEveryN / lcBenchResetEveryN are identical (1024). Either consolidate into one package-level constant in a shared bench_helpers_test.go, or just inline the literal — the named constant doesn't carry useful semantics here.
  • nit: The doc comment on appendBulkPlain (helper) is good; the dispatcher method's doc says "avoiding the reflection loop in Array.Append" but the loop is in Array.Append only after the dispatcher returns (false, nil) — wording could be tightened to "avoiding the reflection loop in Array.Append for typed [][]T inputs."
  • nit: In prose / commit message, wrap ClickHouse SQL types in backticks per the repo convention ("Array" → `Array`, "LowCardinality" → `LowCardinality`).

Verdict

Needs discussion — performance gain is clear and well-measured, but the new bulk paths need correctness tests before merge, and the implicit "Append doesn't mutate on ColumnConverterError" contract that the design relies on should either be documented as an Interface invariant or worked around defensively.

@vvminashkin

Copy link
Copy Markdown
Author
  • appendBulkPlain appends offsets before attempting the inner Append, then assumes any error is a ColumnConverterError whose contract is "no state mutation." Two issues here:

    1. The contract isn't documented anywhere on the Interface.Append side — it's an emergent property of the current implementations. Worth either documenting it on Interface.Append in lib/driver/driver.go ("Append must not mutate column state when returning ColumnConverterError") or writing the bulk path defensively (append offsets only after the inner Append succeeds, or after the per-item retry completes).
    2. On a non-ColumnConverterError return (or on a type mismatch like [][]string passed to Array(Int64)), the bulk path leaves N dirty offsets in the Array even though zero values landed in the inner column. The old per-row path left at most one dirty offset. Callers are expected to discard a column on error, but worth flagging — and a comment near col.appendOffset calls noting "state is left inconsistent on error; caller must discard" would help future readers.

On (2): the state is invalid either way - caller has to discard on any error, so it doesn't matter.

On (1): fair point. I can add a doc comment on Interface.Append saying implementations must not mutate state when returning ColumnConverterError, since the retry path in both appendRowPlain and appendBulkPlain depends on it. Want me to add it?

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.

3 participants