Skip to content

feat(clickhouse): INSERT FORMAT, DELETE forms+settings, MV, groupByTimeBucket, typed bindings#11

Open
lohanidamodar wants to merge 14 commits into
mainfrom
feat/clickhouse-insert-delete-settings-mv
Open

feat(clickhouse): INSERT FORMAT, DELETE forms+settings, MV, groupByTimeBucket, typed bindings#11
lohanidamodar wants to merge 14 commits into
mainfrom
feat/clickhouse-insert-delete-settings-mv

Conversation

@lohanidamodar
Copy link
Copy Markdown
Contributor

@lohanidamodar lohanidamodar commented May 17, 2026

Summary

Closes six ClickHouse builder/schema gaps that block migrating utopia-php/audit and utopia-php/usage onto utopia-php/query 0.3.x. The first three commits land the originally-scoped capabilities; the next three close gaps surfaced by the migration dry-run on audit and usage.

1. INSERT ... FORMAT JSONEachRow on the ClickHouse builder

  • New Builder\ClickHouse::insertFormat(string $format, array $columns = []) flips the next insert() into FORMAT-pragma mode. Output: INSERT INTO `t` (`col1`, `col2`) FORMAT JSONEachRow with no VALUES and no bindings — the row payload is streamed into the HTTP body by the calling adapter.
  • Returns a FormattedInsertStatement (extends Statement) exposing read-only columns and format properties so adapters can map row arrays to the correct column order and pick the right body encoder without re-parsing the SQL.

2. DELETE with trailing SETTINGS ... clause

  • Builder\ClickHouse::delete() appends the same SETTINGS fragment as SELECT when hint() / settings() has been called. Output: ALTER TABLE `t` DELETE WHERE ... SETTINGS k=v, ....
  • A hint() validated as key=value is just a SETTINGS entry on ClickHouse, so no parallel deleteSettings() API is introduced.

3. CREATE MATERIALIZED VIEW ... TO target_table AS ... on Schema\ClickHouse

  • New createMaterializedView(string $name, string $targetTable, Builder|string $body, bool $ifNotExists = true) and dropMaterializedView(string $name, bool $ifExists = true).
  • Body accepts either a Builder (its compiled SQL is inlined and bindings ride the returned Statement) or a raw SQL string for MV bodies whose subqueries don't yet round-trip through the builder.
  • Drop-in replacement for the inline DDL utopia-php/usage builds today for its SummingMergeTree daily rollup MV.

4. Query::groupByTimeBucket($attr, $interval) (new base method) + ClickHouse compilation

  • New Method::GroupByTimeBucket enum case and Query::groupByTimeBucket(string $attribute, string $interval) factory. Allowed intervals: 1m, 5m, 15m, 1h, 1d, 1w, 1M.
  • ParsedQuery gains a readonly timeBuckets field; Builder\ClickHouse::compileGroupByTimeBucket maps each interval to its toStartOf* function. Other dialects throw UnsupportedException from base Builder::compileGroupByTimeBucket at build-time.
  • Replaces the previous UsageQuery::groupByInterval subclass pattern, which no longer works on 0.3.x because Query::__construct calls Method::from() unconditionally and Method is a backed enum.
  • Selecting/ordering on the bucket uses the same pattern as groupByRaw: re-emit the function through selectRaw / orderByRaw when you need to reference it in the SELECT list or ORDER BY.

5. Named-typed {name:Type} placeholder bindings on Builder\ClickHouse

  • New Builder\ClickHouse::useNamedBindings() toggle (off by default — positional ? remains the default for parity with every other dialect). When enabled, ? placeholders are rewritten to ClickHouse {paramN:Type} form at Statement-emission time.
  • New withParamType($column, $type) / withParamTypes($map) for registering column → ClickHouse type. Type strings are validated against ^[A-Za-z][A-Za-z0-9_]*(?:\([^)]*\))?$.
  • When no registration matches, value-based inference fills in: int → Int64, float → Float64, bool → UInt8, null → Nullable(String), DateTimeInterface → DateTime64(3), default → String.
  • Statement gains a parallel readonly ?array $namedBindings. The positional $bindings is left intact so existing callers keep working.

6. Lightweight DELETE FROM alongside ALTER TABLE DELETE

  • New Builder\ClickHouse::deleteMode($mode) picks between DELETE_MODE_LIGHTWEIGHT (DELETE FROM t WHERE …, the new default) and DELETE_MODE_MUTATION (ALTER TABLE t DELETE WHERE …, opt-in).
  • The two forms are storage-path-significant and not interchangeable, so the builder never auto-translates between them. The trailing SETTINGS clause is whatever the caller registers — lightweight_deletes_sync = 0 and mutations_sync = 0 are not auto-paired to the chosen mode.
  • The previous commit's DELETE behavior (ALTER TABLE … DELETE, item 2 above) is now reachable explicitly via deleteMode(Builder::DELETE_MODE_MUTATION); the lightweight form is the new default to match the audit cleanup() baseline before the migration.

Architectural follow-ups (3 commits)

A read-only audit pass after item 6 flagged three deviations from the library's established patterns. All three are landed in this PR so the architecture stays consistent:

  • refactor(clickhouse): wire Binding value object through the rewritersrc/Query/Builder/Binding.php shipped declared-but-unused while the rewriter kept a parallel list<?string> of column hints. Replace the bare string array with list<?Binding>, trim Binding to {value, column} (the only fields read at the rewrite site), and remove the unused name / type / withName / withType surface so we don't ship a public shape we never exercise.
  • refactor(clickhouse): factor materialized views into Feature/TraitcreateMaterializedView / dropMaterializedView landed inline on Schema\ClickHouse, deviating from the Feature\Views + Trait\Views pattern every other Schema feature follows. Factor into new Schema\Feature\ClickHouse\MaterializedViews interface + Schema\Trait\ClickHouse\MaterializedViews trait, mirroring how the Builder side already scopes CH-only features under Feature/ClickHouse/ and Trait/ClickHouse/. Schema\ClickHouse now implements MaterializedViews and uses the trait.
  • test(clickhouse): move new builder + schema tests under Feature/ClickHouse/ — Move InsertFormatTest, DeleteSettingsTest, GroupByTimeBucketTest, NamedBindingsTest from tests/Query/Builder/ClickHouse/ to tests/Query/Builder/Feature/ClickHouse/ (where ApproximateAggregatesTest, ArrayJoinsTest, AsofJoinsTest already live), and MaterializedViewTest to tests/Query/Schema/Feature/ClickHouse/MaterializedViewsTest.php (mirroring the new feature location). All moves use git mv so file history follows; test count unchanged.

Downstream migration plan

  • utopia-php/audit PR #120 will get follow-up commits to (a) drop the whereRaw escape hatch in favor of typed Query::lessThan('time', …) filters paired with withParamType('time', 'DateTime64(3)'), and (b) switch back to the lightweight DELETE form (now the default) so the audit storage path matches the pre-migration behavior. No code change is needed inside audit for that second item — the default behavior just becomes correct again after this PR ships.
  • utopia-php/usage migration PR (not yet open) will land once this PR is tagged. It replaces the local UsageQuery::groupByInterval subclass with Query::groupByTimeBucket, drops the local SELECT/ORDER raw expressions in favor of selectRaw('toStartOfHour(time) AS bucket') plus groupByTimeBucket('time', '1h'), and opts into useNamedBindings() so the existing HTTP transport stops post-processing positional placeholders.

Out of scope (deferred — explicitly not in this PR)

  • LowCardinality column type, SummingMergeTree shorthand, tuple cursor helpers, MV-body subquery handling — all medium/low priority per the audit/usage migration dry-run.
  • Schema-aware builders that derive types from a schema object. The type registration in item 5 is explicit per-column, not schema-driven.
  • HTTP transport, retry, healthCheck — those stay in audit/usage.
  • Library version bump / tag.

Test plan

  • composer format clean (Pint).
  • composer lint clean.
  • composer check clean (PHPStan max level).
  • composer test green — 5227 tests, 12166 assertions.
  • New snapshot tests under tests/Query/Builder/Feature/ClickHouse/{InsertFormatTest,DeleteSettingsTest,GroupByTimeBucketTest,NamedBindingsTest}.php and tests/Query/Schema/Feature/ClickHouse/MaterializedViewsTest.php.
  • Unsupported-dialect coverage in tests/Query/Builder/MariaDBTest.php.
  • CI green on the PR.

Adds `Builder\ClickHouse::insertFormat(string $format, array $columns = [])` which
flips the builder into FORMAT-pragma mode for the next `insert()` call. The
compiled output is `INSERT INTO \`t\` (\`col1\`, \`col2\`) FORMAT <name>` with
no VALUES and no bindings — the row payload is streamed into the HTTP body by
the calling adapter.

The returned `FormattedInsertStatement` extends `Statement` with two extra
read-only properties — `columns` and `format` — so adapters can map row arrays
to the correct column order and pick the right body encoder without having to
re-parse the SQL.

Motivates the next-step migration of utopia-php/audit's `INSERT INTO t FORMAT
JSONEachRow` POSTs to the ClickHouse HTTP interface onto the builder.
`Builder\ClickHouse::delete()` now appends the same SETTINGS fragment as SELECT
when `hint()` or `settings()` has been called on the builder. The compiled
output becomes `ALTER TABLE \`t\` DELETE WHERE ... SETTINGS k1 = v1, k2 = v2`.

This is what utopia-php/audit's async cleanup needs to emit so the HTTP DELETE
returns as soon as the mutation is scheduled rather than after it runs to
completion — i.e. `lightweight_deletes_sync = 0`.

The two stores stay merged (a `hint()` validated as `key=value` is just a
SETTINGS entry on ClickHouse), so no parallel `deleteSettings()` API is
introduced.
Adds `Schema\ClickHouse::createMaterializedView()` and `dropMaterializedView()`.

`createMaterializedView(string $name, string $targetTable, Builder|string $body, bool $ifNotExists = true)`
emits `CREATE MATERIALIZED VIEW [IF NOT EXISTS] \`name\` TO \`target\` AS <body>`.
The body accepts either a `Builder` (its compiled SQL is inlined and its
bindings ride the returned `Statement`) or a raw SQL string, mirroring the
flexibility we need for MV bodies whose subqueries do not yet round-trip
through the builder.

`dropMaterializedView(string $name, bool $ifExists = true)` emits the
symmetric `DROP VIEW [IF EXISTS] \`name\`` — ClickHouse uses the regular
`DROP VIEW` form for both regular and materialized views.

Drop-in replacement for the inline DDL utopia-php/usage builds today for its
SummingMergeTree daily rollup MV.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 17, 2026

📊 Coverage

Metric PR Baseline Δ
Lines 91.96% (7305/7944) 91.85% +0.10%
Methods 84.58% (1086/1284) 84.56% +0.02%
Classes 66.34% (136/205) 65.84% +0.50%

Full per-file breakdown in the job summary.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR closes six ClickHouse builder/schema gaps needed to migrate utopia-php/audit and utopia-php/usage onto the 0.3.x query library. The six capabilities are: INSERT … FORMAT <name> with a FormattedInsertStatement subtype, DELETE SETTINGS clause + a lightweight vs. mutation mode toggle, CREATE/DROP MATERIALIZED VIEW, groupByTimeBucket with ClickHouse toStartOf* compilation, named typed {name:Type} bindings, and all the reset() and withExecutor() correctness fixes landed in follow-up commits.

  • Named bindingsbindingColumn is tracked via try/finally scoping in compileFilter and propagated through the addBinding/addBindings overrides; bindingMeta and $bindings are always reset and accumulated in lockstep, so applyNamedTypedBindings always has a consistent parallel array to resolve types from.
  • FormattedInsertStatementwithExecutor() is correctly overridden to return a self carrying columns and format; namedBindings is always null (FORMAT INSERT has no placeholders), which is correct.
  • GroupByTimeBucket — queries are routed exclusively to ParsedQuery::timeBuckets (never groupBy), preventing any double-processing in buildGroupByClause; the compileGroupBy guard handles the standalone Query::compile() path.

Confidence Score: 5/5

All six features are narrowly scoped to ClickHouse-specific paths and leave every other dialect's behavior unchanged; the existing 5000+ test suite plus the new snapshot tests cover the critical new code paths thoroughly.

The implementation is internally consistent: bindingMeta and bindings are kept in lockstep by the overridden addBinding/addBindings (the base addBindings uses array_push directly, so there is no double-metadata accumulation), reset() clears all new fields, Statement::withExecutor forwards namedBindings, and FormattedInsertStatement::withExecutor is correctly covariant. No stale-state, type-mismatch, or double-compile paths were identified.

No files require special attention.

Important Files Changed

Filename Overview
src/Query/Builder/ClickHouse.php Major additions: FORMAT INSERT, deleteMode(), named typed bindings with type inference, compileGroupByTimeBucket override. reset() correctly clears all new fields. addBinding/addBindings overrides correctly keep bindingMeta in lockstep with bindings.
src/Query/Builder.php Adds bindingColumn tracking with try/finally scoping in compileFilter, timeBuckets loop in buildGroupByClause, base compileGroupByTimeBucket stub, and addBinding/addBindings extended signatures. Changes are backward compatible.
src/Query/Builder/ClickHouse/FormattedInsertStatement.php New class preserving columns and format metadata through withExecutor() correctly. Parent constructor called with correct positional arguments; namedBindings defaults to null which is correct for FORMAT INSERT (always empty bindings).
src/Query/Builder/Statement.php Adds nullable namedBindings parameter as the last optional arg. withExecutor() now correctly forwards namedBindings to the cloned Statement.
src/Query/Query.php Adds groupByTimeBucket() factory with interval validation against a closed constant set. groupByType() parser correctly routes GroupByTimeBucket to timeBuckets (not groupBy) preventing any double-processing.
src/Query/Schema/Trait/ClickHouse/MaterializedViews.php createMaterializedView accepts Builder or raw string body; Builder bindings are forwarded correctly. dropMaterializedView emits DROP VIEW which is valid ClickHouse syntax for materialized views.
src/Query/Builder/Binding.php Simple readonly value object carrying a binding value and its source column name. Clean design.
src/Query/Builder/ParsedQuery.php Adds timeBuckets field with default [] for backward compatibility. Correctly typed as list of {attribute,interval} maps.
src/Query/Method.php Adds GroupByTimeBucket enum case at the correct position alongside GroupBy.
src/Query/Builder/Feature/Aggregates.php Interface addition of groupByTimeBucket with correct docblock pointing to allowed intervals constant.

Reviews (4): Last reviewed commit: "test(clickhouse): move new builder + sch..." | Re-trigger Greptile

Comment thread src/Query/Builder/ClickHouse.php Outdated
Comment thread src/Query/Schema/ClickHouse.php Outdated
Comment thread src/Query/Builder/ClickHouse/FormattedInsertStatement.php
lohanidamodar and others added 3 commits May 17, 2026 09:18
…nctions

Add a first-class base-library method for time bucketing so adapters do not
have to subclass `Query` to model `GROUP BY toStartOfHour(time)`-style
clauses. The new method is dialect-aware: only ClickHouse implements it
today; other dialects throw `UnsupportedException` from the base builder.

API
- `Method::GroupByTimeBucket` enum case + `Query::groupByTimeBucket($attr,
  $interval)` factory; intervals validated against
  `Query::GROUP_BY_TIME_BUCKET_INTERVALS` (`1m`, `5m`, `15m`, `1h`, `1d`,
  `1w`, `1M`).
- `Feature\Aggregates::groupByTimeBucket()` interface method + trait
  implementation that pushes a `GroupByTimeBucket` query onto the pending
  list.

AST shape
- `ParsedQuery` gains a new readonly `timeBuckets` field
  (`list<array{attribute, interval}>`) rather than folding into `groupBy`:
  bucket call sites are structurally different from plain columns
  (function call vs identifier) and downstream builders need to dispatch
  on that distinction without parsing strings. `Query::groupByType` routes
  `Method::GroupByTimeBucket` queries into this field; `Builder.compile()`
  routes the method to `compileGroupBy`.

Compilation
- `Builder::compileGroupByTimeBucket()` is `protected` and throws by
  default; `buildGroupByClause` calls it for every entry in
  `$grouped->timeBuckets`, so unsupported dialects fail loudly at
  build-time rather than silently dropping the clause.
- `Builder\ClickHouse::compileGroupByTimeBucket()` maps each allowed
  interval to its `toStartOf*` function name via a closed lookup table.

Selecting / ordering on the bucket follows the same pattern as
`groupByRaw`: callers re-emit the bucket expression through `selectRaw`
or `orderByRaw` when they need to reference it in the SELECT list or
ORDER BY. Keeping the call sites explicit avoids ambiguity about which
alias the GROUP BY clause is referring to.

Tests
- `tests/Query/Builder/ClickHouse/GroupByTimeBucketTest.php` snapshots
  the compiled SQL for all seven intervals, exercises composition with
  plain `groupBy()` and `selectRaw/orderByRaw`, and pins the
  `ParsedQuery::timeBuckets` shape.
- `tests/Query/Builder/MariaDBTest.php` covers the unsupported-dialect
  path with an `UnsupportedException` assertion.

README updated with a Time bucketing subsection under ClickHouse and a
`groupByTimeBucket` row in the feature matrix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ClickHouse over HTTP requires `{name:Type}`-style parameter placeholders for
type-safe parameterization. The builder previously emitted positional `?`
only, which forced adapters to post-process the compiled SQL — fragile
and easy to get wrong against complex predicates. This commit makes the
named-typed form a first-class, opt-in feature of the ClickHouse builder
without disturbing the positional contract every other dialect relies on.

API
- `Builder\ClickHouse::useNamedBindings(bool $enabled = true)` — toggle.
  Off by default; positional `?` and `Statement::$bindings` keep working
  unchanged.
- `Builder\ClickHouse::withParamType(string $column, string $type)` /
  `withParamTypes(array $map)` — register a ClickHouse type for a column.
  Type strings are validated against
  `^[A-Za-z][A-Za-z0-9_]*(?:\([^)]*\))?$` so we reject anything that
  isn't a plain type name with an optional parenthesised parameter list
  (e.g. `DateTime64(3)`, `Nullable(String)`).
- New `Builder\Binding` value object scaffolds the binding-with-metadata
  shape for future per-call type overrides; the placeholder rewriter
  uses a parallel `list<?string>` keyed by binding index for now.

Wiring
- Base `Builder::addBinding(mixed $value, ?string $column = null)` takes
  an optional column hint. Existing callers pass nothing and continue to
  push to `list<mixed> $bindings` unchanged.
- Base `Builder::compileFilter()` snapshots `$bindingColumn` from the
  current query attribute before dispatching the match, and restores in
  a `finally` so nested filters (AND/OR/Having) don't leak column hints.
- `Builder\ClickHouse` overrides `addBinding`/`addBindings` to mirror
  the column hint into `$bindingMeta` in lockstep with `$bindings`.
  Index N in either array always corresponds to the N-th `?` in the
  compiled SQL.

Statement
- `Statement` gains a readonly `?array $namedBindings` (default null) so
  callers that read the typed map directly don't have to parse the SQL.
  `FormattedInsertStatement` keeps working — its positional `parent::`
  call hits `Statement::__construct` with the new param defaulted.

Rewriter
- `ClickHouse::applyNamedTypedBindings(Statement)` runs at every CH
  Statement boundary (`build()`, `insert()`, `update()`, `delete()`),
  walks `?` placeholders left-to-right with the same regex
  `AssertsBindingCount` uses, looks up `$paramTypes[$column]` per
  binding, and falls back to `inferClickHouseType($value)` when no
  registration matches. The positional `$bindings` payload is untouched
  so consumers can keep using it.

Tests
- `tests/Query/Builder/ClickHouse/NamedBindingsTest.php` snapshots both
  paths — explicit registration and value-based inference — plus the
  DELETE rewrite, LIMIT/OFFSET inference, default-off behavior, type
  validation, and `reset()` clearing of binding metadata.

README updated with a Named-typed bindings subsection under ClickHouse
and a new row in the feature matrix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…E DELETE

`Builder\ClickHouse::delete()` previously emitted only the mutation form
(`ALTER TABLE … DELETE`), which rewrites parts asynchronously. That is
not equivalent to the lightweight form (`DELETE FROM … WHERE …`), which
marks rows deleted via a mask and is async by default. Adapters that
expected the lightweight semantics — e.g. audit's `cleanup()` pre-migration
— would observe a silent storage-path regression after switching to this
builder. Make the choice explicit, with lightweight as the default to
match the ClickHouse server default and the audit baseline.

API
- `Builder\ClickHouse::deleteMode(string $mode)` — pick either
  `DELETE_MODE_LIGHTWEIGHT` (`'lightweight'`, the default) or
  `DELETE_MODE_MUTATION` (`'mutation'`, opt-in). Unknown modes throw
  `ValidationException`.
- Class constants `DELETE_MODE_LIGHTWEIGHT` and `DELETE_MODE_MUTATION`
  expose the wire strings so call sites can avoid magic strings.

Compilation
- `delete()` branches on `$deleteMode` to emit either
  `DELETE FROM `table` WHERE …` or `ALTER TABLE `table` DELETE WHERE …`.
- The trailing `SETTINGS …` clause is unchanged — the builder emits
  whatever `settings()`/`hint()` registered. We do not pair
  `lightweight_deletes_sync = 0` with the lightweight mode nor
  `mutations_sync = 0` with the mutation mode automatically; callers
  pick the setting that matches their chosen storage path.
- `reset()` restores the lightweight default.

Tests
- `tests/Query/Builder/ClickHouse/DeleteSettingsTest.php` extended with
  coverage of both forms, the explicit mutation opt-in, settings-clause
  composition for each form, the validation error path, and reset
  behavior. Renamed the original `testDeleteWithoutSettingsEmitsAlterTableDelete`
  to `testDefaultDeleteEmitsLightweightDeleteFrom` to reflect the new
  default.
- `tests/Query/Builder/ClickHouseTest.php` — existing tests that
  asserted the mutation SQL now either call
  `deleteMode(Builder::DELETE_MODE_MUTATION)` explicitly (when they are
  there to lock down the mutation form) or assert the new lightweight
  SQL (when they were testing generic delete behavior).
- `tests/Query/Builder/ClickHouse/NamedBindingsTest.php` —
  `testDeleteUsesNamedTypedPlaceholdersWhenEnabled` updated to match the
  new default DELETE form.

README updated with a DELETE subsection describing both forms and the
storage-path tradeoff.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lohanidamodar lohanidamodar changed the title feat(clickhouse): INSERT FORMAT, DELETE SETTINGS, materialized views feat(clickhouse): INSERT FORMAT, DELETE forms+settings, MV, groupByTimeBucket, typed bindings May 17, 2026
Comment thread src/Query/Builder/ClickHouse/FormattedInsertStatement.php
Comment thread src/Query/Builder/ClickHouse.php
Comment thread src/Query/Builder/ClickHouse.php Outdated
Standard ClickHouse formats are CamelCase, but user-registered or future
format names may use underscores (e.g. `My_Format`). The previous regex
threw `ValidationException` for valid identifiers.
The previous regex only allowed a single set of parentheses, so common
ClickHouse types like `Nullable(DateTime64(3))` or `Array(Decimal(38, 18))`
were rejected. Widened the pattern to allow one level of nested
parentheses, which covers every ClickHouse type that has a parameterized
inner type.
Previously a reused builder kept emitting `{paramN:Type}` placeholders
after `reset()` even when the caller expected fresh positional bindings,
and stale entries in `$paramTypes` could attach the wrong type to a
column that shared a name across queries. Reset now restores both fields
to their defaults.
…ment::withExecutor

`FormattedInsertStatement` previously inherited `Statement::withExecutor()`,
which called `new self()` on the parent class and silently dropped the
`columns` and `format` properties, returning a plain `Statement`. Adapters
that chain `withExecutor()` on the result of a FORMAT INSERT would then
crash on property-access. Added a covariant override that rebuilds a full
`FormattedInsertStatement`, plus a regression test that asserts the
returned instance keeps both fields. The constructor docblock is also
realigned with the actual parameter order.
…aller-trusted

The string overload of `createMaterializedView()` inlines its argument
verbatim, so a caller who derives the body from any external source can
inject SQL into the resulting DDL. Added an `@security` docblock notice
that points callers at the Builder overload for parameterised inputs and
makes the trust boundary explicit at the call site.
The ClickHouse builder kept a parallel `list<?string> $bindingMeta` to
remember which column produced each `?` placeholder, while
`Builder\Binding` sat declared but unused. Replace the bare string array
with `list<?Binding>` and trim `Binding` to the fields actually read at
the rewrite site — `value` plus `column`. The `name` and `type` fields
were never set by any caller and the `withName`/`withType` factories
were never invoked.

`resolveBindingType()` now reads `$bindingMeta[$index]->{column,value}`
directly instead of indexing `$this->bindings` in parallel, and
`addBinding()` / `addBindings()` construct the typed value objects so
the meta list and the positional bindings list stay in lockstep.
`createMaterializedView` and `dropMaterializedView` landed as inline
public methods on `Schema\ClickHouse`, which deviated from the
established Feature interface + Trait pattern that every other Schema
feature (Views, Databases, Triggers, …) follows.

There is no precedent for dialect-scoped Schema features in the
codebase today, but the Builder side already mirrors `Feature/ClickHouse`
+ `Trait/ClickHouse` for CH-only features (ApproximateAggregates,
ArrayJoins, AsofJoins, LimitBy, WithFill). Adopt the same segments here:
new `Schema\Feature\ClickHouse\MaterializedViews` interface and
`Schema\Trait\ClickHouse\MaterializedViews` trait. `Schema\ClickHouse`
now `implements MaterializedViews` and `use`s the trait, matching how it
already consumes `Views` and `Databases`.

The `Builder|string` body union is left as-is — no `RawExpression`
wrapper exists yet that could replace it, and introducing one would
expand the change well beyond this refactor.
…House/

Four new builder tests landed at `tests/Query/Builder/ClickHouse/`, a
third layout next to the existing `tests/Query/Builder/Feature/ClickHouse/`
(ApproximateAggregatesTest, ArrayJoinsTest, AsofJoinsTest). Move them
into the established location and re-namespace from
`Tests\Query\Builder\ClickHouse` to `Tests\Query\Builder\Feature\ClickHouse`:

- InsertFormatTest
- DeleteSettingsTest
- GroupByTimeBucketTest
- NamedBindingsTest

The schema MV test (previously `tests/Query/Schema/ClickHouse/MaterializedViewTest`)
also moves to `tests/Query/Schema/Feature/ClickHouse/MaterializedViewsTest`
to mirror the new `Schema\Feature\ClickHouse\MaterializedViews` location
that the previous commit introduced. Class renamed to `MaterializedViewsTest`
to match the source feature name.

All moves use `git mv` so file history follows. Test count is unchanged
(5227 tests, 12166 assertions).
Comment on lines +387 to +397
/**
* Track each binding's value + column hint in lockstep with the positional
* list so the placeholder rewriter can attach the right ClickHouse type to
* the right `?`.
*/
#[\Override]
protected function addBinding(mixed $value, ?string $column = null): void
{
parent::addBinding($value, $column);
$this->bindingMeta[] = new Binding($value, $column ?? $this->bindingColumn);
}
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.

Can we instead store it at bind time of the query, even if we add an unused name param in other dialects? I prefer to avoid bind-matching like this, it's caused a few bugs in current DB library

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.

We can add this at a higher level, Postgres supports materialized views too

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