Skip to content

Commit 36ba9d5

Browse files
committed
refactor(clickhouse): drop builder state mutation in bulkInsert(); document Format::serialize() column contract
- bulkInsert() no longer assigns to the fluent insertFormat / insertFormatColumns fields after the envelope is compiled. The compileFormatInsertEnvelope() helper is already parameterised, so those assignments were stale state — reusing a builder instance for a subsequent regular insert() previously inherited the residual format envelope. - Format::serialize() PHPDoc now spells out the $columns === null contract: ordering is derived from the keys of the first row, with no cross-row consistency check. Positional formats (TabSeparated) corrupt silently on inconsistent row shapes; named formats (JSONEachRow) tolerate reordering but the explicit $columns argument acts as a projection filter. - New tests guard both invariants: builder-reuse after bulkInsert() and the explicit-columns ordering contract on Format::serialize() across rows whose keys vary.
1 parent ddf9377 commit 36ba9d5

3 files changed

Lines changed: 63 additions & 7 deletions

File tree

src/Query/Builder/ClickHouse.php

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,6 @@ public function bulkInsert(Format $format, iterable $rows, array $columns = []):
176176

177177
$body = $format->serialize($materialized, empty($columns) ? null : $columns);
178178

179-
$this->insertFormat = $format->value;
180-
$this->insertFormatColumns = $columns;
181-
182179
return new FormattedInsertStatement(
183180
$sql,
184181
[],

src/Query/Builder/ClickHouse/Format.php

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,27 @@ enum Format: string
1919

2020
/**
2121
* Serialize an iterable of associative rows into the body payload for
22-
* this format. Columns are derived from the first row; subsequent rows
23-
* use the same column ordering. An empty iterable yields an empty
24-
* string — ClickHouse accepts an empty body as a zero-row insert.
22+
* this format. An empty iterable yields an empty string — ClickHouse
23+
* accepts an empty body as a zero-row insert.
24+
*
25+
* When `$columns` is null the column ordering is derived from the keys
26+
* of the first row encountered. Subsequent rows are serialized against
27+
* whatever shape they themselves carry — there is no cross-row
28+
* consistency check. The implications differ per format:
29+
*
30+
* - For positional formats (e.g. {@see Format::TabSeparated}) values
31+
* are emitted in row-key order. If later rows reorder their keys the
32+
* columns silently misalign with the envelope's column list. Pass
33+
* `$columns` explicitly whenever row shapes are not guaranteed
34+
* identical, or whenever the format is positional.
35+
* - For named formats (e.g. {@see Format::JSONEachRow}) key ordering
36+
* does not affect correctness because each value is paired with its
37+
* key in the wire format. `$columns` still acts as a projection
38+
* filter: rows missing a listed column receive `null`, and row keys
39+
* outside the list are dropped.
2540
*
2641
* @param iterable<array<string, mixed>> $rows
27-
* @param list<string>|null $columns Optional explicit column ordering. When null, derived from the first row.
42+
* @param list<string>|null $columns Optional explicit column ordering. When null, derived from the keys of the first row.
2843
*/
2944
public function serialize(iterable $rows, ?array $columns = null): string
3045
{

tests/Query/Builder/Feature/ClickHouse/BulkInsertTest.php

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,4 +352,48 @@ public function testInsertFormatRejectsEmptyColumnNameMatchingBulkInsert(): void
352352
->insertFormat('JSONEachRow', ['id', ''])
353353
->insert();
354354
}
355+
356+
public function testBulkInsertDoesNotPersistFormatStateOnBuilder(): void
357+
{
358+
$builder = (new Builder())
359+
->into('events');
360+
361+
$builder->bulkInsert(Format::JSONEachRow, [['id' => 1]]);
362+
363+
$regular = $builder
364+
->into('users')
365+
->set(['name' => 'alice'])
366+
->insert();
367+
368+
$this->assertNotInstanceOf(FormattedInsertStatement::class, $regular);
369+
$this->assertStringNotContainsString('FORMAT', $regular->query);
370+
$this->assertSame(
371+
'INSERT INTO `users` (`name`) VALUES (?)',
372+
$regular->query,
373+
);
374+
$this->assertSame(['alice'], $regular->bindings);
375+
}
376+
377+
public function testFormatSerializeExplicitColumnsPinOrderingAcrossInconsistentRows(): void
378+
{
379+
$rows = [
380+
['id' => 1, 'event' => 'login'],
381+
['event' => 'view', 'id' => 2],
382+
['id' => 3],
383+
];
384+
385+
$tabSeparated = Format::TabSeparated->serialize($rows, ['id', 'event']);
386+
$this->assertSame(
387+
"1\tlogin\n2\tview\n3\t\\N",
388+
$tabSeparated,
389+
);
390+
391+
$jsonEachRow = Format::JSONEachRow->serialize($rows, ['id', 'event']);
392+
$this->assertSame(
393+
'{"id":1,"event":"login"}' . "\n"
394+
. '{"id":2,"event":"view"}' . "\n"
395+
. '{"id":3,"event":null}',
396+
$jsonEachRow,
397+
);
398+
}
355399
}

0 commit comments

Comments
 (0)