1.24.0 release#954
Merged
Merged
Conversation
…ted date strings (#947) ## Description Fixes #946. `formatQueryParams` serialized a JS `Date` as a bare Unix-seconds integer (e.g. `1683244800`) in **every** context. Inside a container that becomes `[1683244800]`, which ClickHouse accepts for `Array(DateTime)` but **rejects** for `Array(Date)`/`Array(Date32)`: ``` Code: 27. DB::Exception: Cannot parse input: expected '\'' before: '1683244800]': value [1683244800] cannot be parsed as Array(Date) for query parameter 'l'. (CANNOT_PARSE_INPUT_ASSERTION_FAILED) ``` The `Date` branch now mirrors the existing `boolean` handling and uses the `isInArrayOrTuple` flag: inside arrays, tuples, and maps a `Date` is emitted as a quoted UTC `'YYYY-MM-DD'` string. Verified against ClickHouse 26.5 that this is the **single** representation the element parser accepts for **all** temporal element types — `Array(Date)`, `Array(Date32)`, `Array(DateTime)`, and `Array(DateTime64)` (bare int, quoted int, and full datetime strings are all rejected by `Array(Date)`). Scalar `Date`/`DateTime`/`DateTime64` binding is **unchanged** (scalar still uses the bare Unix-timestamp form that scalar `DateTime` relies on). ### Behavioral note (deliberate tradeoff) Because a query parameter is type-agnostic on the client side (the formatter can't see whether the target column is `Array(Date)` or `Array(DateTime)`), a `Date` used inside `Array(DateTime)`/`Array(DateTime64)` is now bound at **day precision** — the time-of-day is dropped (`2022-05-02 13:25:55` → `2022-05-02 00:00:00`). Date-only is the only encoding `Array(Date)` accepts, so this is unavoidable for a type-blind formatter; an integration test pins this behavior. Scalar `DateTime`/`DateTime64` are untouched and keep full precision. This is the JS analog of a cross-client class also fixed in clickhouse-java (#2898) and reported for clickhouse-connect (#188). ## Changes - `packages/client-common/src/data_formatter/format_query_params.ts`: in the `value instanceof Date` branch, when `isInArrayOrTuple` is true, return `'${value.toISOString().slice(0, 10)}'`. (This file is the single shared source; `client-node`/`client-web` symlink it, so both clients get the fix.) ## Test - **Unit** (`format_query_params.test.ts`): `Date` in an array, nested array, tuple, and object value all render as quoted date strings; time-of-day/millis are dropped; a `Date` composes with other element types in a mixed array. Existing scalar `Date` tests are retained as the contrast that pins the unchanged scalar output. - **Integration** (`select_query_binding.test.ts`): binding `[Date, Date]` to `Array(Date)` round-trips to `["2023-05-05","2021-01-02"]` (fails on `main` with the parse error above); a `Date` bound to `Array(DateTime)` returns `2022-05-02 00:00:00`, documenting the day-precision tradeoff and guarding that `Array(DateTime)` no longer errors. Verified the unit tests fail on unpatched code with the exact bug (`expected '[1659081134]' to be '['2022-07-29']'`) and pass with the fix; full unit suite (389 passed) and the `select_query_binding` integration suite (31 passed) are green; `prettier`, `eslint`, and `tsc` are clean. ## Pre-PR validation gate - [x] Deterministic repro confirmed (server rejects `[<unix>]` for `Array(Date)`; new tests fail on `main`) - [x] Root cause documented above - [x] Fix targets the root cause (container element encoding) - [x] Test fails without fix, passes with fix - [x] No existing tests broken; no existing tests weakened - [x] Fix on the live runtime path (proven by an end-to-end integration test through `client.query`) - [x] Convention compliance verified per `AGENTS.md` / `CONTRIBUTING.md` - [ ] CHANGELOG entries (added in a follow-up commit on this branch) --------- Co-authored-by: Polyglot AI <293096396+polyglotAI-bot@users.noreply.github.com>
## Summary Adds the Contributor Covenant v2.1 as the Code of Conduct for clickhouse-js
…s, and dangerously_log_query_text (#950) ## Summary Implements the clickhouse-js instrumentation feedback from **Langfuse** — closes #948 ([Slack thread](https://clickhouse-inc.slack.com/archives/C03B9QS104X/p1784122553160099) with Steffen Schmitz). The goal: make the client's OpenTelemetry spans carry enough detail that a downstream app can drop its own hand-rolled ClickHouse span wrapper. ### Coverage of #948 **1. Record `X-ClickHouse-Summary` on the `query()` span, and record all keys.** - Node parses the summary on `connection.query` (`parse_summary: true`) and records it on the `clickhouse.query` span (previously only `command`/`exec`/`insert` carried it). - The **Web** client now parses the `X-ClickHouse-Summary` header for `query`/`command`/`exec`/`insert` — it parsed **none** before. - `setResponseSpanAttributes` now iterates over **every key** in the parsed summary (`clickhouse.summary.<key>`) instead of a hardcoded subset, so `total_rows_to_read`, `memory_usage`, and future keys (`real_time_microseconds`, …) arrive for free. `memory_usage` added to `ClickHouseSummary`. **2. Opt-in query text attribute (`db.query.text`).** - New `dangerously_log_query_text` client option (default `false`). When enabled, the raw SQL is attached as OTEL `db.query.text` and (Node) included in the `error`-level request-failure logs. Bound `query_params` values and credentials are **never** logged or traced. Parameterized queries record only the statement text (with placeholders), which is exactly Langfuse's use case. **3. Row counts for non-streaming result consumption.** - `db.response.returned_rows` is now set for non-streaming `json()` consumption (`JSON` / `JSONObjectEachRow` / other single-document JSON formats) too, not just row-streaming paths. Parity across node and web `ResultSet`. **4. A small, scoped hook for enriching span attributes.** - New per-request `span_attributes` bag on `query`/`command`/`exec`/`insert`/`ping` (the per-call option Steffen preferred). Merged into the operation span; caller attributes never override the client's own `db.*`/`server.*`/`clickhouse.*` attributes. ### Relationship to #884 This **supersedes #884**. That PR *drops* the query-text plumbing from the Node log/transport layer; per the design discussion, this PR instead **guards** it behind `dangerously_log_query_text` (safe-by-default — query text is absent from logs unless explicitly opted in). The `logRequestError` hardening from #884 (scalars only, no full params/search-params object) is preserved here, and the no-SQL-leak regression test is carried forward and extended to cover the flag-on path. #884 can be closed once this lands. ## Verification - New unit tests: common tracer (`db.query.text` on/off, summary on the query span, all-keys recording, `span_attributes` merge + precedence), node ResultSet span (`returned_rows` for `JSON`/`JSONObjectEachRow`), node no-leak/guarded-log regression, web summary parsing (present/absent/malformed). - `npm run build`, `npm run typecheck`, `npm run lint`, full Node (406) and Web (255) unit suites all pass. - End-to-end against a live ClickHouse: confirmed the `clickhouse.query` span carries the full summary (incl. `total_rows_to_read`, `memory_usage`), `db.query.text`, and the caller's `span_attributes` with core attributes intact; and `returned_rows` is present for non-streaming `json()`. ## Checklist - [x] Unit tests added - [x] Docs updated (`docs/howto/tracing.md`) - [x] CHANGELOG entries added (client-node, client-web) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Retarget the staged (unreleased) `# 1.23.2` CHANGELOG section to `# 1.24.0` in both client packages, ahead of cutting the 1.24.0 beta. #950 added new public API (`dangerously_log_query_text` config option + per-request `span_attributes`), which is an additive **minor** release. `1.23.2` was never published, so the section's contents — the OTEL features (#950) and the #947 `Array(Date)` binding fix — ship together as `1.24.0`. Header-only change; content is unchanged. Required by the release skill's Step 1 (the CHANGELOG must carry the exact version header before the version bump). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bumps `@clickhouse/client-web` from `1.23.1` to `1.24.0` (minor bump). Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Bumps `@clickhouse/client` from `1.23.1` to `1.24.0` (minor bump). Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Syncs release with main to cut @clickhouse/client and @clickhouse/client-web 1.24.0, including OTEL instrumentation enhancements, a query-params binding fix for Array(Date)/Array(Date32), and accompanying docs/tests/changelog updates.
Changes:
- Bump Node + Web client versions to 1.24.0 and update both package changelogs accordingly.
- Improve tracing/logging: parse/attach
X-ClickHouse-Summary, addspan_attributes, add opt-indangerously_log_query_text, and recorddb.response.returned_rowsfor non-streamingjson(). - Fix container-nested
Datequery-param serialization and add unit/integration coverage; add Code of Conduct.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/client-web/src/version.ts | Version constant bumped to 1.24.0. |
| packages/client-web/src/result_set.ts | Records returned_rows for non-streaming json(); adds best-effort row counting helper. |
| packages/client-web/src/connection/web_connection.ts | Adds X-ClickHouse-Summary parsing and plumbs summary through results. |
| packages/client-web/package.json | Package version bumped to 1.24.0. |
| packages/client-web/CHANGELOG.md | Adds 1.24.0 release notes (OTEL + Date array binding fix). |
| packages/client-web/tests/unit/web_summary.test.ts | Adds unit tests for Web summary header parsing (present/absent/malformed). |
| packages/client-node/src/version.ts | Version constant bumped to 1.24.0. |
| packages/client-node/src/result_set.ts | Records returned_rows for non-streaming json(); adds best-effort row counting helper. |
| packages/client-node/src/connection/node_base_connection.ts | Enables summary parsing for query; tightens error logging and gates raw SQL logging behind dangerously_log_query_text. |
| packages/client-node/package.json | Package version bumped to 1.24.0. |
| packages/client-node/CHANGELOG.md | Adds 1.24.0 release notes (OTEL + Date array binding fix). |
| packages/client-node/tests/unit/node_result_set_span.test.ts | Adds unit tests asserting returned_rows for non-streaming JSON and JSONObjectEachRow. |
| packages/client-node/tests/unit/node_log_query_text.test.ts | Adds regression tests ensuring SQL logging is opt-in and never logs params/credentials. |
| packages/client-common/src/data_formatter/format_query_params.ts | Fixes Date formatting inside containers to quoted YYYY-MM-DD. |
| packages/client-common/src/connection.ts | Extends connection result types to carry summary for query results. |
| packages/client-common/src/config.ts | Adds dangerously_log_query_text config option and plumbs it into connection params. |
| packages/client-common/src/client.ts | Adds span_attributes, attaches summary to query spans, iterates all summary keys, and gates db.query.text. |
| packages/client-common/src/clickhouse_types.ts | Adds optional memory_usage to ClickHouseSummary; defines WithClickHouseSummary. |
| packages/client-common/tests/unit/tracing.test.ts | Expands tracing tests for summary keys, query-span summary, db.query.text, and span_attributes. |
| packages/client-common/tests/unit/format_query_params.test.ts | Adds unit tests for container-nested Date formatting behavior. |
| packages/client-common/tests/integration/select_query_binding.test.ts | Adds integration tests for Array(Date) binding and the Array(DateTime) day-precision tradeoff. |
| package-lock.json | Updates lockfile versions for bumped packages. |
| docs/howto/tracing.md | Documents summary key recording, returned_rows behavior, span_attributes, and dangerously_log_query_text. |
| CODE_OF_CONDUCT.md | Adds Contributor Covenant v2.1 Code of Conduct. |
Comment on lines
+324
to
+327
| const parse = this.params.json?.parse ?? JSON.parse; | ||
| try { | ||
| return parse(summaryHeader) as ClickHouseSummary; | ||
| } catch (err) { |
Comment on lines
+133
to
+137
| const parsed = this.jsonHandling.parse<ResultJSONType<T, Format>>(text); | ||
| // Record the returned row count for non-streaming consumption too, so | ||
| // `db.response.returned_rows` is present regardless of the format / | ||
| // consumption style (see countReturnedRows). | ||
| const rows = countReturnedRows(parsed, this.format as DataFormat); |
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
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.
Sync
releasefrommainto cut @clickhouse/client and @clickhouse/client-web1.24.0.Merging this triggers the automatic
head-tagged beta publish for both clients (path-scoped: the shared common sources changed in #950, so both clients republish), each followed by an e2e run against a live ClickHouse.What's in 1.24.0
X-ClickHouse-Summaryon theclickhouse.queryspan with all keys recorded (incl.total_rows_to_read,memory_usage); Web client now parses the summary header;db.response.returned_rowsfor non-streamingjson(); per-requestspan_attributesbag; opt-indangerously_log_query_text(db.query.text).Array(Date)/Array(Date32)query-parameter binding.Included commits: #950, #951 (changelog retarget), #952 / #953 (version bumps), plus #947 and #942 (Code of Conduct) which had not yet been synced to
release.🤖 Generated with Claude Code