Skip to content

feat: migrate ClickHouse adapter to utopia-php/query 0.3.x builder - #4

Open
lohanidamodar wants to merge 2 commits into
mainfrom
feat/utopia-query-0.3.x
Open

feat: migrate ClickHouse adapter to utopia-php/query 0.3.x builder#4
lohanidamodar wants to merge 2 commits into
mainfrom
feat/utopia-query-0.3.x

Conversation

@lohanidamodar

@lohanidamodar lohanidamodar commented May 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Migrates the ClickHouse adapter to utopia-php/query 0.3.x (stable pin 0.3.*, resolving to the tagged 0.3.3 release — no dev-branch pins). Every table CREATE, materialized view, INSERT envelope, SELECT and DELETE now compiles through Schema\ClickHouse / Builder\ClickHouse; the adapter keeps only its HTTP transport, routing logic, and the few DDL shapes the library cannot express yet.

Rebased onto current main (7c3fbf1) as a single commit, so the migration now also covers everything that landed while this PR was open:

What changed

Schema (DDL via Schema\ClickHouse)

  • createTable() emits the events/gauges tables through Table\ClickHouse: typed columns with codec(), Engine::MergeTree, partitionBy, orderBy, settings.
  • createDailyTable() uses Engine::SummingMergeTree; createDailyMaterializedView() goes through Schema\ClickHouse::createMaterializedView (the MV body remains a hand SELECT — subquery-aggregation MV bodies do not round-trip through the builder yet).
  • Column shapes the typed API cannot express are emitted via rawColumn() so the deployed DDL stays byte-for-byte identical: DateTime64(3, 'UTC') (timezone argument), LowCardinality(Nullable(String)) (the typed API emits the invalid Nullable(LowCardinality(...)) nesting), and the hyphenated skip-index names (index-path), which the typed index API rejects.
  • Projections (ADD PROJECTION), MODIFY SETTING, the ADD COLUMN IF NOT EXISTS dim backfills and the retention MODIFY TTL / REMOVE TTL ALTERs stay raw SQL — no schema-layer support yet. The new geo / sdk / sdkVersion / ordinal columns flow into both the schema-layer CREATE and the raw backfill path automatically, since both read from Metric::getEventSchema() / getGaugeSchema().

Reads (SELECT via Builder\ClickHouse)

  • Every builder is initialised with useNamedBindings() + withParamTypes(); all schema columns are pinned to their ClickHouse parameter types (DateTime64(3, 'UTC'), Int64, String) so placeholders never fall back to PHP value inference.
  • Migrated: find/findFromTable, findAggregatedFromTable (interval buckets + dim group-bys), count (including the bounded LIMIT max subquery), sum and the routed daily/hybrid paths, findDaily, sumDaily, sumDailyBatch, getTotal*, getTimeSeries.
  • The hybrid daily+raw sum compiles its two sides as independent builder statements and merges them under SUM(...) FROM (... UNION ALL ...), prefixing one side's named bindings to avoid placeholder collisions.
  • Cursor pagination keeps the tuple-keyset comparison as a whereRaw fragment with its own named bindings (upstream builder helper deferred).
  • contains() / containsAny() / notContains() now pass straight through to the builder. The 0.3 ClickHouse builder compiles them to position(column, ?), which is the same substring semantics Align contains query with utopia-php/database semantics #28 introduced, and matches needles literally — so the adapter no longer needs its own escapeLikeWildcards() helper for % / _. The adapter still validates that every needle is a string and still rejects empty value lists.

Writes

  • addBatch() compiles the INSERT INTO t (...) FORMAT JSONEachRow envelope via insertFormat(); JSONEachRow body assembly and the non-retrying POST transport are unchanged.

Deletes

  • purge() / purgeDaily() emit lightweight DELETE FROM through the builder.

Query 0.3 API surface

  • Query::getMethod() returns the Method enum, so all string TYPE_* comparisons are gone (ClickHouse and Database adapters).
  • BREAKING: the UsageQuery::TYPE_GROUP_BY_INTERVAL / TYPE_GROUP_BY string constants are removed. UsageQuery::groupByInterval() and UsageQuery::groupBy() remain and now map to Method::GroupByTimeBucket / Method::GroupBy; groupBy() also accepts an array of columns to stay signature-compatible with the 0.3 base class.
  • composer.lock moves utopia-php/query 0.1.1 -> 0.3.3 and nothing else; utopia-php/validators and utopia-php/database stay on main's pins.

Test plan

  • CI Linter (Pint, psr12) — pass
  • CI CodeQL job, which runs composer check (PHPStan level max over src + tests) — pass
  • CI Tests on the pinned docker-compose.yml stack (ClickHouse 25.11 + MariaDB 10.7) — OK (264 tests, 1305 assertions)

Locally the rebase was additionally verified without Docker, against a natively installed ClickHouse 26.8.1 + MariaDB 11.8.8: 264 tests, 1293 assertions, 2 failures, 0 errors, and an origin/main (7c3fbf1) run on the exact same servers produced an identical result — same two tests, same assertion count. Those two, ClickHouseSchemaTest::testEventsTableSwapsBloomForSetOnLowCardinality and ::testGaugesTableSwapsBloomForSetOnLowCardinality, are a ClickHouse-version artefact: 26.x renders skip indexes as INDEX `index-status` (status) TYPE set(0) (parenthesised column list) where 25.11 renders INDEX `index-status` status TYPE set(0), and the assertions expect the un-parenthesised form. They pass on CI's 25.11 image.

Green coverage includes the SHOW CREATE TABLE schema assertions (codecs, LowCardinality(Nullable(String)), DateTime64(3, 'UTC'), hyphenated index-* names), projection/dim-routing, cursor pagination, purge propagation, the retention-TTL tests, and the contains wildcard-escaping e2e test added in #28.

Still deferred upstream (follow-up PRs)

  • Tuple-cursor builder helper (cursor pagination still uses whereRaw)
  • LowCardinality(Nullable(...)) column type and timezone argument for DateTime64
  • Skip-index names containing hyphens
  • Projections / MODIFY SETTING / ADD COLUMN IF NOT EXISTS / MODIFY TTL in the schema layer
  • MV bodies with aggregation subqueries

@greptile-apps

greptile-apps Bot commented May 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR migrates the ClickHouse adapter, UsageQuery, and the Database adapter from the removed Query::TYPE_* string constants to the Method enum surface introduced in utopia-php/query 0.3.x, while also compiling all DDL, SELECT, INSERT, and DELETE operations through the typed named-binding builder and schema layers.

  • Schema (DDL): createTable, createDailyTable, and createDailyMaterializedView now go through Schema\ClickHouse / Table\ClickHouse; columns the typed API cannot yet express (e.g., DateTime64(3,'UTC'), LowCardinality(Nullable(String)), hyphenated index names) fall back to rawColumn() so the deployed DDL stays byte-for-byte identical.
  • Reads (SELECT): Every query path uses useNamedBindings() + withParamTypes() with per-column ClickHouse type pins; the hybrid daily+raw UNION ALL prefixes one side's bindings via prefixNamedBindings() to prevent key collisions.
  • Writes / Deletes: addBatch() now compiles the INSERT INTO … FORMAT JSONEachRow envelope via insertFormat(); purge()/purgeDaily() use the builder's DELETE FROM path.

Confidence Score: 5/5

  • Safe to merge — the migration is a mechanical but well-scoped replacement of string-based SQL assembly with the typed builder API, and the deployed DDL and query semantics remain unchanged.
  • All SQL compilation now goes through typed named bindings with no string interpolation of user values. The hybrid UNION ALL path correctly prefixes one side's bindings to avoid collisions. The schema DDL falls back to rawColumn() for column types the library cannot yet express, keeping the actual table definitions byte-for-byte identical. CI reports 264 tests passing on the target ClickHouse version. The only items left in the review are style notes about hardcoded interval strings in the fill helpers.
  • No files require special attention; the fill-helper style notes in src/Usage/Adapter/ClickHouse.php are non-blocking.

Important Files Changed

Filename Overview
src/Usage/Adapter/ClickHouse.php Core migration: all DDL, SELECT, INSERT, and DELETE paths now compile through utopia-php/query 0.3.x builders with typed named bindings. One style concern: the zero-fill/locf fill helpers hardcode an === '1h' branch rather than using the same central interval map used by getTimeSeries's allowlist guard.
src/Usage/UsageQuery.php Migrated from removed TYPE_* string constants to Method enum values; VALID_INTERVALS gains 30m; groupBy() now accepts `array
src/Usage/Adapter/Database.php All TYPE_* string comparisons replaced with Method enum cases in convertQueriesToDatabase. Straightforward mechanical update, no logic changes.
composer.json Dependency flipped from the dev-branch alias to the stable 0.3.* pin with minimum-stability: stable restored, resolving the previously flagged mutability concern.

Reviews (8): Last reviewed commit: "Merge origin/main into feat/utopia-query..." | Re-trigger Greptile

Comment thread src/Usage/Adapter/ClickHouse.php Outdated
Comment thread composer.json Outdated
Comment thread phpstan-baseline.neon Outdated
Comment thread src/Usage/Adapter/ClickHouse.php Outdated
Comment on lines 156 to 170
@@ -172,49 +166,24 @@ public function setTimeout(int $milliseconds): self
return $this;
}

/**
* Enable or disable query logging for debugging.
*
* @param bool $enable Whether to enable query logging
* @return self
*/
public function enableQueryLogging(bool $enable = true): self
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the docs on public methods

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored docblocks on the public setter / lifecycle methods that lost them during the migration rewrite (__construct, setTimeout, enableQueryLogging, setCompression, setKeepAlive, setMaxRetries, setRetryDelay, setAsyncInserts, clearQueryLog, getName, healthCheck, setNamespace, setDatabase, setSecure, plus the namespace/tenant/sharedTables getters). Done in 0e0bbd6.

Comment thread src/Usage/Adapter/ClickHouse.php Outdated
Comment on lines +977 to +980
'integer' => $table->addColumn($id, ColumnType::BigInteger),
'float' => $table->addColumn($id, ColumnType::Float),
'boolean' => $table->addColumn($id, ColumnType::Boolean),
'datetime' => $table->datetime($id, 3),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be shorthand methods we can use for integer, float and boolean too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped the addColumn(..., ColumnType::BigInteger|Float|Boolean) calls in the match block to the dedicated bigInteger() / float() / boolean() shorthand, plus the value BigInteger column in createDailyTable(). ColumnType import is no longer needed. SQL snapshot tests still green. Done in c2a261e.

Comment thread src/Usage/Adapter/ClickHouse.php Outdated
Comment thread src/Usage/Adapter/ClickHouse.php Outdated
Bump utopia-php/query from 0.1.* to 0.3.* and compile the ClickHouse
adapter's SQL through the shared builder and schema layer instead of
hand-assembled strings:

- Reads (find, count, sum, totals, time series, daily, hybrid
  daily+raw union) build through the ClickHouse query builder with
  typed named bindings; every schema column is pinned to its
  ClickHouse parameter type so placeholder types never fall back to
  PHP value inference.
- CREATE TABLE for the events/gauges/daily tables and the daily
  materialized view go through the schema layer. Column shapes the
  typed API cannot express (DateTime64 timezone argument,
  LowCardinality(Nullable(...)), hyphenated skip-index names) are
  emitted as raw definitions so deployed DDL is byte-for-byte
  unchanged. Projections, ALTER-based dim backfills, the retention
  TTL ALTERs and the MV body stay raw SQL, which the schema layer
  cannot express yet.
- INSERT envelopes compile via insertFormat(); the JSONEachRow body
  assembly and transport are unchanged.
- Query 0.3 API: getMethod() returns the Method enum, so all string
  TYPE_* comparisons are gone. UsageQuery keeps its groupByInterval()
  and groupBy() factories, now mapped to Method::GroupByTimeBucket and
  Method::GroupBy; groupBy() also accepts an array of columns to stay
  signature-compatible with the base class.
- contains()/containsAny()/notContains() pass straight through to the
  builder, which compiles them to position(column, needle) on
  ClickHouse. That keeps the utopia-php/database substring semantics
  while matching needles literally, so the adapter no longer has to
  escape LIKE wildcards itself.
@lohanidamodar
lohanidamodar force-pushed the feat/utopia-query-0.3.x branch from 8fc13b3 to 597c4fa Compare July 26, 2026 05:22
Main added an aggregate('max') hint for rolling a gauge level series up to a
coarser interval. It arrived encoded as a TYPE_AGGREGATE string constant, which
is exactly what query 0.3 removes, so it is re-expressed as Method::Max - the
same translation groupByInterval and groupBy already had. Values stay empty
because base Query::max() reserves them for an alias; extractAggregate() reads
the function name back off the method, so the adapter's string-based contract
is unchanged.

Two real defects came out of the merge rather than the conflict markers:

- findAcrossTenants() (new on main) reads with a null tenant, but this branch
  had narrowed the read chain to a non-nullable string when tenant filtering
  moved into applyTenantFilter(). Every cross-tenant read would have died on a
  TypeError. applyTenantFilter/applyFilters/findAggregatedFromTable now take
  ?string, and a null tenant adds no predicate - widening the scope, never
  narrowing it to the empty one, which still fails fast.
- The aggregate('max') override reassigned $valueExpr after the builder had
  already consumed it. That worked upstream, where the SQL was assembled as a
  string at the end, but here it was dead: the hint would have been accepted
  and silently ignored. Moved above the builder.

The switch in parseQueries kept a 'case UsageQuery::TYPE_AGGREGATE' arm, which
after the enum migration can never match - replaced with case Method::Max.

composer: took main's client ^0.3 and database ^7.0.0 with our query 0.3.*
pin, and regenerated the lock from main's rather than hand-merging 16 conflict
blocks. Exactly one package moves against main: query 0.1.1 -> 0.3.3.

Gates: pint pass, phpstan [OK] No errors, UsageQueryTest 29 tests / 84
assertions, each rewritten assertion driven red first. The ClickHouse suite
needs a live server and was not run here.
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.

2 participants