feat!: migrate ClickHouse adapter to the utopia-php/query 0.6 builder - #4
Merged
Conversation
Greptile SummaryMigrates the ClickHouse adapter from hand-assembled SQL strings to the
Confidence Score: 5/5
Important Files Changed
Reviews (13): Last reviewed commit: "chore: correct the changelog version and..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
abnegate
reviewed
May 19, 2026
lohanidamodar
force-pushed
the
feat/utopia-query-0.3.x
branch
from
July 6, 2026 03:42
4530574 to
8fc13b3
Compare
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
force-pushed
the
feat/utopia-query-0.3.x
branch
from
July 26, 2026 05:22
8fc13b3 to
597c4fa
Compare
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.
The branch was pinned at 0.3.* while query had released 0.4, 0.5 and 0.6. No adapter changes were needed: the Parser -> Classifier rename, the UnsupportedException removal, nested join conditions and Schema\Order are all symbols this package does not use. Query::contains() is deprecated in 0.6 in favour of containsString() for substring matching. One test called it; containsString() returns the same Method::Contains the adapter switches on, so the swap changes no SQL. The two other hits are unrelated - one is Utopia\Database\Query, a different package, and one is a comment. pint pass, phpstan [OK] No errors, UsageQueryTest 29 tests / 84 assertions. The ClickHouse suite needs a live server and runs in CI.
lohanidamodar
changed the title
Contributor
Author
Now on utopia-php/query 0.6, and ready for reviewBoth this PR and its sibling were pinned at No adapter changes were needed. The breaking changes in between are all symbols these packages don't use:
The one thing 0.6 surfaced was CI is green on 0.6 including the ClickHouse integration suites, which is what actually exercises the emitted SQL against a server. Release order this unblocks: merge and tag here, then cloud swaps its |
Eight public statics - isGroupByInterval, extractGroupByInterval, removeGroupByInterval, isGroupBy, extractGroupBy, removeGroupBy, isAggregate and removeAggregate - had no caller outside their own tests. The only references in src were the helpers calling each other, a closed loop. They are left over from the pre-0.3 design, where the adapter pulled hints out of the query list before compiling it. parseQueries() now switches on Method directly (GroupByTimeBucket, GroupBy, Max), so nothing needs to ask a Query what kind it is or to filter it back out of a list. Checked the consumer, not just this package: cloud calls groupBy, groupByInterval, aggregate and the inherited parse, and none of the removed methods. What stays is what is actually used - VALID_INTERVALS and VALID_AGGREGATES, the three factories, and extractAggregate, which the adapter uses to read an aggregate's name back off its Method. 278 -> 171 lines. Also dropped testGroupByIsMethod and testAggregateIsMethod: after the isMethod() override went away in the 0.3 migration they only asserted that the base library resolves its own enum. phpstan [OK] No errors, pint pass, UsageQueryTest 15 tests / 49 assertions.
Review caught that Method::ContainsAny is handled by the ClickHouse adapter - it is in VALUE_REQUIRED_METHODS and has an arm in parseQueries - while Database::convertQueriesToDatabase had neither a case for it nor a default. The predicate fell out of the switch, contributed no WHERE fragment, and the query returned every row: a wrong answer wearing the shape of a working one, and the two backends disagreeing on the same input. Verified before fixing: no 'case Method::ContainsAny' and no 'default:' in that switch, against an arm at ClickHouse.php:3743. Mapped it to DatabaseQuery::containsAny(), and added a default that throws. The missing case was the bug; the missing default is why it could happen quietly, and why the next method added upstream would repeat it. That is the same reasoning the ClickHouse adapter already applies to empty filter values, where it refuses rather than emit a full-table match. testContainsAnyFiltersRatherThanBeingDropped asserts the filtered set is genuinely smaller than the unfiltered one, which is what a dropped predicate would break. It lives in UsageBase so both adapters run it, and needs the live MariaDB/ClickHouse services - so its red has not been observed locally, only the static gates have run here.
…fault The default-throw added in 48c1cda made the silent drops visible, and re-review immediately used that to find four more: IsNull, IsNotNull, CursorAfter, CursorBefore and Max were all reaching it. Turning a silent wrong answer into a loud failure is progress, but throwing on methods the adapter can actually serve is its own regression. - IsNull / IsNotNull now delegate to DatabaseQuery::isNull()/isNotNull(). They take no values, which is why they are absent from VALUE_REQUIRED_METHODS. - Max joins GroupBy and GroupByTimeBucket as explicitly not pushed down: this adapter returns raw rows, so an aggregate hint is ignored rather than refused. - CursorAfter / CursorBefore refuse with a message naming the limitation. This adapter has never implemented cursor pagination and DatabaseQuery's cursors take a Document rather than the value carried here, so the honest answer is to say so instead of returning an unpaginated set that reads as success. Every Method the ClickHouse adapter handles now has an explicit arm here - verified by diffing the two switches - so the default is left for methods that genuinely do not exist yet, which is what it is for.
Contributor
Author
|
Right, and fixed in Diffed the two switches rather than fixing only the two you named, which turned up five reaching the default:
Every |
Two findings from the re-review, both accurate. The CHANGELOG still announced 'query 0.3.x' and a bump 'to 0.3.*' after composer.json moved to 0.6.*. That is the file a consumer reads to decide whether an upgrade will break them, so a stale version label there is worse than a stale comment. parseQueries() in the ClickHouse adapter had no default arm, the same shape that let ContainsAny be silently dropped by the Database adapter in 48c1cda. It has 24 arms and accepts a whitelist, so nothing reaches the end today - but that was equally true of the Database switch until a method was added upstream. Both adapters now refuse an unhandled method rather than compiling a query without it. pint pass, phpstan [OK] No errors, UsageQueryTest 15/49.
This was referenced Aug 25, 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
2 participants
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
Migrates the ClickHouse adapter to
utopia-php/query0.3.x (stable pin0.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 throughSchema\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:events_daily, including theREMOVE TTLguard anchored on error code 36 (feat(clickhouse): optional retention TTL on the raw events table #24, feat(clickhouse): extend retention TTL to the events_daily table #27)sdk/sdkVersiondimension columns (Add sdk and sdkVersion dimension columns to usage events #25)ordinalgauge dimension column (feat: add ordinal gauge dimension column #26)containsaligned with utopia-php/database substring semantics (Align contains query with utopia-php/database semantics #28)What changed
Schema (DDL via
Schema\ClickHouse)createTable()emits the events/gauges tables throughTable\ClickHouse: typed columns withcodec(),Engine::MergeTree,partitionBy,orderBy,settings.createDailyTable()usesEngine::SummingMergeTree;createDailyMaterializedView()goes throughSchema\ClickHouse::createMaterializedView(the MV body remains a hand SELECT — subquery-aggregation MV bodies do not round-trip through the builder yet).rawColumn()so the deployed DDL stays byte-for-byte identical:DateTime64(3, 'UTC')(timezone argument),LowCardinality(Nullable(String))(the typed API emits the invalidNullable(LowCardinality(...))nesting), and the hyphenated skip-index names (index-path), which the typed index API rejects.ADD PROJECTION),MODIFY SETTING, theADD COLUMN IF NOT EXISTSdim backfills and the retentionMODIFY TTL/REMOVE TTLALTERs stay raw SQL — no schema-layer support yet. The new geo /sdk/sdkVersion/ordinalcolumns flow into both the schema-layer CREATE and the raw backfill path automatically, since both read fromMetric::getEventSchema()/getGaugeSchema().Reads (SELECT via
Builder\ClickHouse)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.find/findFromTable,findAggregatedFromTable(interval buckets + dim group-bys),count(including the boundedLIMIT maxsubquery),sumand the routed daily/hybrid paths,findDaily,sumDaily,sumDailyBatch,getTotal*,getTimeSeries.SUM(...) FROM (... UNION ALL ...), prefixing one side's named bindings to avoid placeholder collisions.whereRawfragment 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 toposition(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 ownescapeLikeWildcards()helper for%/_. The adapter still validates that every needle is a string and still rejects empty value lists.Writes
addBatch()compiles theINSERT INTO t (...) FORMAT JSONEachRowenvelope viainsertFormat(); JSONEachRow body assembly and the non-retrying POST transport are unchanged.Deletes
purge()/purgeDaily()emit lightweightDELETE FROMthrough the builder.Query 0.3 API surface
Query::getMethod()returns theMethodenum, so all stringTYPE_*comparisons are gone (ClickHouse and Database adapters).UsageQuery::TYPE_GROUP_BY_INTERVAL/TYPE_GROUP_BYstring constants are removed.UsageQuery::groupByInterval()andUsageQuery::groupBy()remain and now map toMethod::GroupByTimeBucket/Method::GroupBy;groupBy()also accepts an array of columns to stay signature-compatible with the 0.3 base class.composer.lockmovesutopia-php/query0.1.1 -> 0.3.3and nothing else;utopia-php/validatorsandutopia-php/databasestay onmain's pins.Test plan
composer check(PHPStan level max oversrc+tests) — passdocker-compose.ymlstack (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::testEventsTableSwapsBloomForSetOnLowCardinalityand::testGaugesTableSwapsBloomForSetOnLowCardinality, are a ClickHouse-version artefact: 26.x renders skip indexes asINDEX `index-status` (status) TYPE set(0)(parenthesised column list) where 25.11 rendersINDEX `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 TABLEschema assertions (codecs,LowCardinality(Nullable(String)),DateTime64(3, 'UTC'), hyphenatedindex-*names), projection/dim-routing, cursor pagination, purge propagation, the retention-TTL tests, and thecontainswildcard-escaping e2e test added in #28.Still deferred upstream (follow-up PRs)
whereRaw)LowCardinality(Nullable(...))column type and timezone argument forDateTime64MODIFY SETTING/ADD COLUMN IF NOT EXISTS/MODIFY TTLin the schema layer