fix(params): quote nested scalar values - #341
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
==========================================
+ Coverage 95.99% 96.05% +0.06%
==========================================
Files 42 42
Lines 849 862 +13
==========================================
+ Hits 815 828 +13
Misses 34 34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes ClickHouse native-parameter encoding for DateTime64 values when they appear nested inside compound types (e.g., arrays/tuples), by emitting quoted ISO datetime literals for nested values while preserving the existing top-level numeric Unix timestamp behavior.
Changes:
- Update
DateTime64param conversion to emit quoted ISO datetime literals when the value is nested. - Add a multipart request-body regression test covering
Array(Tuple(DateTime64(6), UUID)).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Client/Http/RequestFactoryTest.php | Adds regression test asserting nested DateTime64 inside an array/tuple is emitted as a quoted ISO literal in the request body. |
| src/Param/ParamValueConverterRegistry.php | Updates the datetime64 converter to switch encoding based on the nested flag (ISO+quoted when nested; Unix timestamp when top-level). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fb02387 to
d447ea0
Compare
|
Addressed in d447ea0. Nullable and LowCardinality now preserve the nested flag and resolve parameterized inner types via Type::fromString(), with regression coverage for both wrappers around DateTime64(6). |
d447ea0 to
d2dc5d6
Compare
|
CI fix pushed in d2dc5d6: documented the UnsupportedParamType exception from convertWrappedValue. Focused PHPUnit, PHPCS, and syntax checks pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Param/ParamValueConverterRegistry.php:117
- When $nested is true, DateTime64 values are formatted using the DateTimeInterface instance’s local timezone (format('Y-m-d H:i:s.u')). This can shift the represented instant when the DateTime has a non-default timezone (the string literal has no offset), whereas the non-nested path uses an epoch-based representation (format('U.u')) that is timezone-safe. Consider normalizing nested DateTimeInterface values to a fixed timezone (e.g., UTC) before formatting so nested and top-level encodings represent the same instant.
if ($value instanceof DateTimeInterface) {
$value = $nested
? $value->format('Y-m-d H:i:s.u')
: $value->format('U.u');
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Param/ParamValueConverterRegistry.php:118
- When
$nestedis true,DateTimeInterfacevalues are formatted as a timezone-less local datetime (Y-m-d H:i:s.u). This can change the represented instant compared to the top-levelU.uencoding if theDateTimeInterfacecarries a non-UTC timezone, because ClickHouse will interpret the literal in the server/type timezone. Consider normalizing to UTC (or the type’s timezone, if applicable) before formatting so nested and top-level encodings represent the same moment.
if ($value instanceof DateTimeInterface) {
$value = $nested
? $value->format('Y-m-d H:i:s.u')
: $value->format('U.u');
}
src/Param/ParamValueConverterRegistry.php:123
- PR description says nested
DateTime64values should be emitted as quoted ISO datetime literals. In the$nestedbranch, non-DateTimeInterfaceinputs (string/float/int) are currently just quoted as-is (e.g.1675213323.123456becomes'1675213323.123456') rather than being converted to an ISO datetime string. If numeric/epoch inputs are expected to work for nestedDateTime64, this likely won’t meet the stated decision and may still be hard for ClickHouse to parse depending on context/scale.
if (is_string($value) || is_float($value) || is_int($value)) {
return $nested
? "'" . Escaper::escape((string) $value) . "'"
: $value;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/Client/Http/RequestFactoryTest.php:186
- The expected Unix timestamp is hard-coded, but
new DateTimeImmutable('2026-07-30 12:00:00.123456')is interpreted in the runtime default timezone. If CI/default timezone changes, this assertion will fail even if the converter behavior is correct. Compute the expected value from the same literal usingformat('U.u')(or set an explicit timezone) to make the test deterministic.
self::assertStringContainsString('1785412800.123456', $request->getBody()->__toString());
src/Param/ParamValueConverterRegistry.php:117
- When
DateTime64is nested, DateTimeInterface values are formatted asY-m-d H:i:s.uwithout any timezone/offset. If the DateTime object has a non-default timezone (or ClickHouse server TZ differs), this can shift the represented instant compared to the top-level numeric encoding which preserves absolute time. Consider documenting this behavior, normalizing to a specific timezone, or including an offset in the formatted string if ClickHouse supports it.
'datetime64' => static function (mixed $value, Type|string|null $type = null, bool $nested = false) {
if ($value instanceof DateTimeInterface) {
$value = $nested
? $value->format('Y-m-d H:i:s.u')
: $value->format('U.u');
041d93a to
a6b018d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Param/ParamValueConverterRegistry.php:137
- Switching IPv4/IPv6 from noopConverter() (mixed) to stringConverter() (string) tightens accepted value types and can break callers that previously passed the integer representation of IPs (ClickHouse accepts IPv4 as UInt32). If you want to keep supporting both forms while still quoting nested string literals, consider a dedicated converter that only quotes when the value is a string.
'IPv4' => self::stringConverter(),
'IPv6' => self::stringConverter(),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/Client/Http/RequestFactoryTest.php:211
- This test also hardcodes a Unix timestamp while constructing DateTimeImmutable without an explicit timezone, which can fail if the default timezone differs. Use a fixed timezone and build the expected string from the DateTimeImmutable used in the request params.
new RequestOptions(
[
'inputs' => [
new DateTimeImmutable('2026-07-30 12:00:00.123456'),
],
tests/Client/Http/RequestFactoryTest.php:190
- This test hardcodes a Unix timestamp and creates DateTimeImmutable without an explicit timezone, making the assertion dependent on the runtime default timezone. Use an explicit timezone and derive the expected numeric representation from the same DateTimeImmutable instance.
This issue also appears on line 207 of the same file.
'value' => new DateTimeImmutable('2026-07-30 12:00:00.123456'),
],
),
);
.github/workflows/infection.yml:26
- The ClickHouse readiness check uses curl without a connect/overall timeout, which can stall the job longer than intended if the TCP connect hangs. Add small curl timeouts (and optionally show errors) so each attempt fails fast.
if curl --fail --silent http://127.0.0.1:28123/ping; then
Summary
DateTime64,IPv4, andIPv6parameter values.Root cause
Array, tuple, and wrapped parameter serializers pass nested values to their scalar converters.
DateTime64,IPv4, andIPv6did not produce ClickHouse string literals in that context, which caused invalid native parameter syntax.