Skip to content

sql: add ClickHouse Map type support for sql_insert#4178

Merged
josephwoodward merged 3 commits intoredpanda-data:mainfrom
rickysaltzer:add-clickhouse-maps
Apr 2, 2026
Merged

sql: add ClickHouse Map type support for sql_insert#4178
josephwoodward merged 3 commits intoredpanda-data:mainfrom
rickysaltzer:add-clickhouse-maps

Conversation

@rickysaltzer
Copy link
Copy Markdown
Contributor

The ClickHouse driver requires map arguments to have concrete Go types matching the column schema (e.g. map[string]int64), but Bloblang args_mapping produces generic map[string]any values. This causes inserts to fail for any column using ClickHouse Map types.

Introduce an argsConverter for ClickHouse that introspects column scan types at connect time via a SELECT ... LIMIT 0 query, then normalizes map and slice arguments through a JSON round-trip into the expected Go types before passing them to the driver.

Applied to both sql_insert output and processor components. Includes integration tests covering common map types.

The ClickHouse driver requires map arguments to have concrete Go types
matching the column schema (e.g. map[string]int64), but Bloblang
args_mapping produces generic map[string]any values. This causes inserts
to fail for any column using ClickHouse Map types.

Introduce an argsConverter for ClickHouse that introspects column scan
types at connect time via a `SELECT ... LIMIT 0` query, then normalizes
map and slice arguments through a JSON round-trip into the expected Go
types before passing them to the driver.

Applied to both sql_insert output and processor components.
Includes integration tests covering common map types.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 26, 2026

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rickysaltzer
Copy link
Copy Markdown
Contributor Author

In reference to #3819

@rickysaltzer
Copy link
Copy Markdown
Contributor Author

Full disclosure, I used Claude to assist with this patch as I don't normally write in Go. Feel free to make any changes to conform to the project's standards.

@josephwoodward
Copy link
Copy Markdown
Contributor

josephwoodward commented Mar 27, 2026

Hi @rickysaltzer, thank you for your pull request. There's a number of linter violations that would be good to clean up and I've added a few comments.

If you are using Claude then you can also use the Claude skill here to review the pull request which can be pretty helpful.

- Removes unused regex based approach, as this was originally intended
  for `sql_raw`, but felt was better left to a future PR.

- Add logging to normalization failures
@rickysaltzer
Copy link
Copy Markdown
Contributor Author

Hey @josephwoodward -

Thanks for such a quick review, I've sent in some changes to hopefully address your comments.

@rickysaltzer
Copy link
Copy Markdown
Contributor Author

Commits

  1. 6475aea "Address Comments" — message format violation: should use sql: prefix (e.g., sql: remove unused regex and add normalization failure logging). Message is vague and doesn't describe the change. This is a fixup to the first commit and should be squashed before merge.

Review
Code changes look good. The argsConverter extension point is clean, reflection/JSON round-trip logic is correct, no race conditions or resource leaks, and integration tests follow existing patterns. The three issues flagged by @josephwoodward in the prior review round (unused parseInsertTableColumns/insertRegexp, unnecessary UseNumber(), silent normalization failures) appear to have been addressed in commit 6475aea.

LGTM once the commits are squashed.

@josephwoodward
Copy link
Copy Markdown
Contributor

Thanks for addressing the comments @rickysaltzer. I see the linter is complaining about something, if you wouldn't mind addressing that when you have a moment that'd be great. Also, if you rebase your changes on the main branch the tests should go green.

@rickysaltzer
Copy link
Copy Markdown
Contributor Author

Hey @josephwoodward - linting should be taken care of if you want to approve the workflows to check.

@josephwoodward josephwoodward merged commit fb4d503 into redpanda-data:main Apr 2, 2026
7 of 8 checks passed
@josephwoodward
Copy link
Copy Markdown
Contributor

image

@josephwoodward
Copy link
Copy Markdown
Contributor

josephwoodward commented Apr 2, 2026

Thank you for your contribution @rickysaltzer, your change is now available in v4.86.0

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