Repository navigation
chore: sync upstream v1.7.0 into the fork - #8
Merged
Merged
Conversation
* Allow removal of `FINAL` keyword. Rewrite to conform to normal ActiveRecord definition structure. * Rename :check_command to :check_command! to reflect that its purpose is to raise an error on a failed check. * Rewrite :settings, while we're in here, to conform to the AR query method API, too. * Allow :unscope to handle :settings and :final. * Invoke correct method for Rails 7.2 and above.
…Nixx#220) The regex in assign_database_to_subquery! used a lookbehind (?<=from) which matched 'from' anywhere in the SQL, including in column names like 'sourced_from'. This caused the next identifier to be incorrectly prefixed with the database name. Bug example: SELECT sourced_from, now() FROM table Would become: SELECT sourced_from, default.now() FROM default.table Causing error: Function with name 'default.now' does not exist Fixed by using \bfrom\s+ to match 'from' only as a complete word (SQL keyword), not as part of column names. Added tests covering: - Column names containing 'from' - Verification that functions are not prefixed - Verification that table names are correctly prefixed All 93 tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
* Add JSON column type support
Adds support for ClickHouse JSON column type to fix schema dump failures.
- Add json: { name: 'JSON' } to NATIVE_DATABASE_TYPES constant
- Register JSON type mapping using ActiveRecord::Type::Json
- Add comprehensive test coverage for JSON column operations
- Include tests for column recognition, CRUD operations, and migrations
- Handle ClickHouse JSON type limitations (non-nullable, numbers as strings)
Resolves "Unknown type 'JSON' for column" errors in Rails schema dumps.
Requires allow_experimental_json_type = 1 setting in ClickHouse 21.1+.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
* Fix JSON migration test for ClickHouse 25.10 compatibility
Use MergeTree engine for JSON migration test instead of relying on the
default Log engine. The Log engine doesn't support JSON columns with
dynamic subcolumns in ClickHouse, which causes test failures in version
25.10.
This change ensures compatibility with both ClickHouse 24.9 and 25.10.
All 98 tests now pass on both versions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
Rails is using `execute_batch` when truncating tables and loading fixtures. https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L232 https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L515 But since this gem is changing signature of `raw_execute`, then it does not work out of the box and needs to call `execute` instead. https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L616-L620
The minitest and rspec helpers were changing ActiveRecord::Base.connection to ClickHouse without restoring it afterward. This broke multi-database Rails apps where the primary database is PostgreSQL/MySQL.
The shared @response_format instance variable on the adapter is mutated by with_response_format, which is unsafe when an adapter is checked out across threads (e.g. Capybara system specs where the test thread and Puma server thread share a connection). One thread's `nil` override inside a wrapped INSERT path could leak into another thread's SELECT default, causing the SELECT to be sent without a FORMAT clause and the response to come back as a raw String body instead of a parsed Hash — manifesting as `Response: undefined method 'map' for nil` from internal_exec_query. Replace the shared ivar with a per-thread, per-adapter stack stored on Thread.current. with_response_format pushes/pops; the new private response_format reader falls back to a new @default_response_format ivar (renamed from @response_format) when no override is in scope. Public API is unchanged. Add spec/single/response_format_thread_safety_spec.rb covering the cross-thread isolation regression and single-thread nesting contract.
* Add URL-based configuration support for the ClickHouse adapter Allows configuring the adapter via a single `url` key (e.g. `clickhouse://user:pass@host:8123/db?ssl=true`) instead of requiring every field to be specified individually. Explicit config keys take precedence over URL-parsed values. * Fix URL parsing bypass when using Rails 7.2+ adapter registry Rails 7.2+ uses the `register` API which calls `ClickhouseAdapter.new` directly, bypassing `clickhouse_connection` where URL parsing lived. Move `parse_clickhouse_url` to a public class method on `ClickhouseAdapter` and call it from `initialize` so it works regardless of entry point.
* Account for arel node scopes without a relation In this case, we get an error when trying to use the `Arel::Nodes::Addition` class, since `Arel::Nodes::Addition#relation` is not defined * Define matviews above tables in structure dump * Fix bug with deserializing inserts in rails 8 * Fix specs
* Test ToSql visitor behavior * Flag cluster specs for setup/teardown
- Add Float32/Float64 subtype handling in Map OID - Add Bool subtype handling in Map OID - Fix Date deserialize crash when value already deserialized - Prevent TimeZoneConverter wrapping Map OID (fixes Hash crash) - Add comprehensive tests for new functionality Fixes serialization issues where Float/Bool values were quoted as strings, causing SYNTAX_ERROR in ClickHouse. Also fixes TypeError on Date re-parsing and 'Hash values are not supported' crash with Map(String, DateTime).
Map(String, DateTime) columns crashed with "Hash values are not supported" when time_zone_aware_attributes was enabled, because TimeZoneConverter tried to wrap Hash values directly. Move the create_time_zone_conversion_attribute? guard from the Railtie into ClickhouseActiverecord.load so it is applied in all contexts (not only in Rails apps).
…PNixx#239) When deserialize was called with an already-parsed DateTime or Time value (e.g. via model.instantiate), it would crash with: TypeError: no implicit conversion of DateTime into String because DateTime.parse was called on an object instead of a string. Root cause: already_deserialized? used bare DateTime/Date which resolved to OID::DateTime/OID::Date (sibling classes in the same module) instead of the standard library types, so the guard never triggered. Fix by using ::DateTime, ::Date, ::Time with explicit root namespace.
) This allows applications using clickhouse-activerecord to set `config.active_record.permanent_connection_checkout = :disallowed` without errors being raised. https://guides.rubyonrails.org/configuring.html#config-active-record-permanent-connection-checkout
…ery limits (PNixx#266) Every connection was made with verify_mode: OpenSSL::SSL::VERIFY_NONE, so an SSL connection to ClickHouse never actually checked the server's certificate. TLS verification is now on by default; set insecure: true to opt back out, or sslca to point at a CA bundle (which was previously accepted as config but never actually wired up to the connection - a bug fixed here as well). Also adds: - open_timeout: bounds how long establishing the underlying TCP connection is allowed to take. Unlike read_timeout/write_timeout, which only apply once a connection exists, there was previously no way to bound a dead network path that never completes the TCP handshake. - Read queries (SELECT, via internal_exec_query) are retried once on a fresh connection when the failure happens before the server responds at all (Net::OpenTimeout, EOFError, ECONNRESET, IOError) - the kind of failure a stale pooled connection produces. Writes are never retried, and Net::ReadTimeout is deliberately not retried on any query, since the server already received the query and re-sending it would just pile another copy of the same load onto it. - max_execution_time / cancel_http_readonly_queries_on_client_close config, merged into the session settings sent with every request, so a server-side query limit can be configured without a raw SQL SETTINGS clause. - Timeouts/cancellations now raise Rails-standard error classes instead of a generic ActiveRecord::ActiveRecordError: ActiveRecord::StatementTimeout (server-side cancellation), ActiveRecord::AdapterTimeout (Net::ReadTimeout - client gave up, query may still be running), and ActiveRecord::ConnectionFailed (connection retry exhausted). All four new config keys are also supported as clickhouse:// URL query params, matching the existing read_timeout/write_timeout/etc. convention.
The base ActiveRecord Column does not define array/array?; each adapter that supports arrays adds it on its own Column subclass (e.g. PostgreSQL). The ClickHouse Column never did, so tooling that detects array columns via `column.respond_to?(:array) && column.array` (such as annotaterb) silently treated `Array(...)` columns as their scalar subtype. Add `Column#array`, matching the top-level `Array(...)` SQL type, and have the schema dumper reuse it instead of re-deriving array-ness from a regex. Also fix the schema dumper's map-value regex so `Map(_, Array(_))` columns are dumped as `map: :array` (the intended single-option form) rather than relying on the previous match-anywhere behavior; both forms reload to the same SQL type.
Float64 columns were dumped as `t.float "name"`, and `t.float` always maps to Float32, so a database built from schema.rb ended up with a narrower column than one built by running migrations. There was also no DSL to declare Float64 at all: `t.float :x, limit: 8` silently ignored the limit. Mirror the integer handling so the width travels with `limit`: - add `float64` to NATIVE_DATABASE_TYPES (`float` stays Float32) - add `TableDefinition#float` that routes `limit: 8` to `:float64` - resolve `:float` with `limit: 8` in `type_to_sql` so add_column / change_column do not produce "Float32(8)" - teach `extract_limit` about Float32 (nil) and Float64 (8) so the dumper's generic `schema_limit` writes `t.float "x", limit: 8` Closes PNixx#268
* Add joins_final for FINAL-aware association joins (sc-214234) ClickHouse's `final` modifier only wrapped the query's primary FROM source, so `.final.joins(:assoc)` emitted a plain `INNER JOIN <table>` against the un-merged table. Code had to work around this with hand-written `INNER JOIN (SELECT * FROM <table> FINAL) alias ON ...`. Add `joins_final(*associations)`, which adds the join(s) like `joins` and renders `INNER JOIN <table> FINAL ON ...` via the ClickHouse Arel visitor. Implemented with a small `Arel::Nodes::FinalTable` marker that wraps the join's table source; because every join visitor emits its table via `visit o.left`, one visitor method covers all join types. Joins are matched by table name and finalized before the FROM-level FINAL node is applied. Supports chaining, dedup with existing joins, and `unscope(:joins_final)`. Add gem specs covering generated SQL and real execution against ClickHouse (FINAL requires an engine that supports it, e.g. ReplacingMergeTree). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SeQx7AkCPiyk5yqj7m3koX * Add left_joins_final (LEFT OUTER JOIN ... FINAL) joins_final only emits INNER JOIN ... FINAL. left_joins_final is the LEFT OUTER counterpart: it adds the association via left_outer_joins and renders FINAL on the joined table, for optional/catalog joins whose rows must be preserved when there is no match while still merging the joined table with FINAL. It shares joins_final_values, so the existing final-join marking and unscope(:joins_final) handling apply unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uhz69nyLNfuciaTh8UhSa * Test mixed joins_final + left_joins_final on the same association Addresses review feedback: cover the edge case where the same association is added via both joins_final and left_joins_final. The shared joins_final_values store dedups via |= and ActiveRecord collapses an association present in both joins and left_outer_joins into a single INNER JOIN, so FINAL is applied exactly once. Behavior verified consistent across ActiveRecord 7.2.3, 8.0.1, and 8.1.3.
`QueryCache` wraps `execute` where it is included, in `AbstractAdapter`, but `Clickhouse::SchemaStatements` is included into the adapter afterwards, so its own `execute` sits ahead of that wrapper in the ancestors. A statement run through it therefore leaves the query cache holding the reads it had already answered, and a repeated `SELECT` inside a request or a job reports the state from before an `INSERT`. Other adapters keep the wrapper by calling `super` and implementing the hook below it, which this adapter cannot do while `execute` owns the HTTP request and `raw_execute` takes a different signature than the one Rails calls. Reapply `dirties_query_cache` instead, which restores the wrapper without moving any of that.
* Treat DEFAULT NULL as a nil column default. * Add missing assertions
* Compare ClickHouse server versions as versions, not floats * Fix version comparison in specs
Brings in the 12 upstream changes since the July sync (PNixx#262 through PNixx#276): TLS verification option and sslca wiring, read-query retry with Rails error classes, max_execution_time settings, query cache dirtying on raw writes, joins_final, Float64 round-tripping, DEFAULT NULL handling, and version-aware server comparisons. Conflicts: kept the fork's Column equality and write_query? alongside upstream's Column#array and RETRYABLE_CONNECTION_ERRORS; took upstream's connect, which passes open_timeout to Net::HTTP.start the same way the fork did, and kept the fork's ConnectionNotEstablished rescue.
…etry count Upstream's connection_hardening_spec covers passing open_timeout to Net::HTTP.start, so only the fork's ConnectionNotEstablished mapping keeps a spec. The fork's raw_execute retry runs inside upstream's read-query retry, so a persistent EOFError on a SELECT makes four POSTs, not two.
kiliman
commented
Oct 20, 2026
kiliman
left a comment
kiliman
left a comment
Author
There was a problem hiding this comment.
Self-review: flagging the two judgment calls before anyone has to ask.
| @connection_parameters[:host], | ||
| @connection_parameters[:port], | ||
| use_ssl: @connection_parameters[:ssl], | ||
| verify_mode: @connection_parameters[:insecure] ? OpenSSL::SSL::VERIFY_NONE : OpenSSL::SSL::VERIFY_PEER, |
Author
There was a problem hiding this comment.
Took upstream's connect verbatim, but kept the fork's rescue Net::OpenTimeout → ConnectionNotEstablished a few lines down (unchanged, so it isn't in this diff).
Upstream's PNixx#266 passes open_timeout to Net::HTTP.start exactly the way our #7 did, so I dropped our copy of that plumbing to keep fork-only code small. The rescue is the one piece of #7 upstream doesn't have. Swarm's Apiary callers rescue ActiveRecord::ActiveRecordError to fall back to Postgres. The adapter connects eagerly in initialize, outside upstream's read-query retry, so without the rescue a cold connect timeout escapes as a bare Net::OpenTimeout and the fallback never runs.
The cost is 4 fork-only lines. The connect timeout spec guards them, and it fails with got #<Net::OpenTimeout> when the rescue is removed. I'd rather upstream this rescue than delete it here. Happy to open that PR against PNixx so the fork can drop it on the next sync.
|
|
||
| expect { adapter.exec_query('SELECT 1') }.to raise_error(ActiveRecord::ConnectionFailed) | ||
| # The fork's raw_execute also retries EOFError once, inside each of these two attempts. | ||
| expect(http_connection).to have_received(:post).exactly(4).times |
Author
There was a problem hiding this comment.
Tradeoff: I changed upstream's expected count from 2 to 4 instead of reconciling the two retry layers.
The fork's raw_execute retries EOFError, ECONNRESET and Net::HTTPBadResponse once, for every statement. Upstream's internal_exec_query now retries SELECTs once more, with a reconnect, around that. So a SELECT on a connection that keeps failing makes 2 × 2 = 4 POSTs. It still ends in ActiveRecord::ConnectionFailed, which is the behavior this spec is really protecting. These failures are immediate and re-sending a SELECT is safe, so the extra attempts cost milliseconds.
Reconciling the layers, for example by teaching raw_execute to skip errors the outer retry handles, would mean more fork-only code in the hottest path just to save two instant retries. The cost of my choice is a one-line divergence in an upstream spec, which may conflict again on a future sync. If you'd prefer one layer, I'd keep raw_execute's, since it also covers writes and HTTPBadResponse, and narrow upstream's list to Net::OpenTimeout and IOError.
kiliman
marked this pull request as ready for review
October 20, 2026 15:31
RubyBrewsday
approved these changes
Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TL;DR Catches the fork up to upstream v1.7.0. GitHub says we're 63 commits behind, but only 12 upstream changes are actually missing. Our two earlier syncs were squash-merged, which kept the code but dropped upstream's history, so GitHub kept counting commits we already had. After this merge the fork is 0 behind, and our remaining fork-only code gets smaller.
Important
Please merge with "Create a merge commit", not squash. A squash drops the upstream ancestry again, and the "behind" count and the phantom conflicts come right back on the next sync.
What reviewers should look at:
connectnow uses upstream's code verbatim.The Full Story 🔍
Details
Why "63 behind" was really 12
The July sync (#3) brought in upstream through PNixx#234 (June 9), and its commit body lists every change. Because it was squash-merged, git's merge-base stayed at a July 2025 commit. A plain
git merge upstream/mastertherefore re-fought everything #1 and #3 had already resolved, and produced 15 conflicts.The first commit on this branch fixes that with
git merge -s ours 7a9c62f. It tells git "we already have upstream through PNixx#234" without changing a single file. That's safe becausegit diff 7a9c62f origin/mastershowed only fork-specific changes. With the correct merge-base, the real upstream merge had 3 conflicts.Conflicts
Column: kept the fork's==/hash(MATERIALIZED dedup fix) and added upstream's newarrayreader.SchemaStatements: kept the fork'swrite_query?and added upstream'sRETRYABLE_CONNECTION_ERRORS.connect: took upstream's version. Upstream's Harden HTTP connection: open_timeout, TLS verification, retry, query limits PNixx/clickhouse-activerecord#266 passesopen_timeouttoNet::HTTP.startthe same way our feat: [ENT-861] support open_timeout and raise ConnectionNotEstablished on connect timeout #7 did, so most of our open_timeout code now simply is upstream's.What's left of #7, and why it stays
Only the
rescue Net::OpenTimeout → ActiveRecord::ConnectionNotEstablishedinconnect, plus one spec. The other three open_timeout specs duplicated upstream'sconnection_hardening_spec, so they're gone.Invariant: a connect timeout must reach the app as an ActiveRecord error. Swarm's Apiary callers rescue
ActiveRecord::ActiveRecordErrorto fall back to Postgres (for exampleactive_subscriptions_count). The adapter connects eagerly ininitialize, which is outside upstream's new read-query retry. So without this rescue, a cold connect timeout surfaces as a bareNet::OpenTimeout: the request fails fast, but it still fails. That's the hang ENT-861 was meant to turn into a fallback. A mutation check confirmed it: with the rescue removed, the spec fails withgot #<Net::OpenTimeout>. This rescue is a good candidate to upstream, which would let the fork drop it entirely.Stacked retries
The fork's
raw_executeretriesEOFError,ECONNRESETandNet::HTTPBadResponseonce, for every statement. Upstream's PNixx#266 adds its own retry around SELECTs ininternal_exec_query, which reconnects and also coversNet::OpenTimeoutandIOError. Together, a SELECT on a connection that keeps failing makes 4 POSTs instead of 2, and still raisesActiveRecord::ConnectionFailed. Those failures are immediate and re-sending a SELECT is safe, so I documented the count in upstream's spec rather than reworking either layer. Reconciling them would only add fork-only code.Worth knowing: upstream deliberately never retries writes, and the fork's
raw_executeretry has covered writes since 2024. This PR doesn't change that, but it's the one real difference in approach between the two.Changes consumers will notice
TIMEOUT_EXCEEDEDnow raisesActiveRecord::StatementTimeout, and a SELECTNet::ReadTimeoutraisesActiveRecord::AdapterTimeout. Both used to be a plainActiveRecordError. Both areStatementInvalidsubclasses, sorescue ActiveRecord::StatementInvalidnow catches timeouts it used to let through.VERIFY_NONE, and that's still the default (insecure: true). Settinginsecure: falseturns verification on, andsslcais now actually wired up (it was accepted before but never used). Turning it on in swarm is a separate follow-up.joins_final, Float64 round-tripping in the schema dumper,DEFAULT NULLread asnil, and server-version comparisons that no longer treat versions as floats.Verification
libfirst on the load path, with no Gemfile change:primary_keys,max_execution_timeoverrun raisedStatementTimeout,From Claudia with 💙