Skip to content

[Fix] Accept parenthesized ClickHouse record keys - #169

Merged
XbzOnGit merged 2 commits into
mainfrom
fix/clickhouse-parenthesized-keys
Oct 25, 2026
Merged

XbzOnGit merged 2 commits into
mainfrom
fix/clickhouse-parenthesized-keys

Conversation

@XbzOnGit

@XbzOnGit XbzOnGit commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

ClickHouse 26.11.1.738 renders a single-column primary or sorting key as (model_id), where 25.12.2.54 renders model_id. DMI's live-table validator rejected the parenthesized form, so a valid single-key record layout failed initialization before the insert stage became ready.

Accept both forms for primary keys and sorting keys while retaining strict identifier-list validation.

Implementation details

  • Trim surrounding whitespace and remove one outer pair of parentheses before parsing the key. Bare and backtick-quoted identifiers retain their existing validation.
  • Continue rejecting function calls, arithmetic, nested tuples, empty wrapped lists, trailing commas, and other malformed keys. Parsed column names and their order must still match the record layout exactly.
  • Move the existing parser and identifier validator into an internal header so the actual C++ parser can be tested without building CUDA or connecting to a server.
  • Add parser regressions and live record round trips for single-column and multi-column keys, including DMI-created tables and pre-existing tables. Add live rejection cases for expressions in each key.

The Ubuntu 26.04 CI run also exposed two existing TLS-test assumptions. Its libcurl 8.18.0 OpenSSL backend can finish the TLS handshake and then reject certificate trust before sending HTTP data, so a server-side failed-handshake count is not a portable verification result. The tests now require certificate rejection, one connection without retries, and no request or application bytes; the trusted-CA capture round trip still runs. This changes test instrumentation and assertions only; the runner selection and production TLS verification are unchanged.

Compatibility

Verified against ClickHouse 25.12.2.54 and 26.11.1.738. Table DDL, record schemas, public APIs, and stored data formats are unchanged. Existing checks for column types/order, primary keys, sorting keys, and index granularity remain active.

Validation

Reproduced from current main (e98a9ff) in a separate clone and Conda environment. Both ClickHouse test instances used separate data directories and ports; the existing server was left running.

Check 25.12.2.54 26.11.1.738
Existing record-storage suite on unchanged main 7 passed 4 passed, 3 failed
Record-storage suite with the fix and new regressions 14 passed 14 passed
Live schema-migration suite 14 passed 14 passed
  • Native parser regressions: 41 passed. Before the fix, all six parenthesized acceptance cases failed.
  • Record schema and format regressions: 10 passed.
  • Clean CPU host-backend build and git diff --check: passed.

Setup: Python 3.12.15 and PyTorch 2.14.1+cpu. Storage tests exercised the compiled DMXHostEngine host backend, with their full-backend availability guard locally redirected to the verified host build. The local build supplied the existing host link's missing libcrypto dependency; no build-system change is included. No training or GPU execution was needed for this storage-validation regression.

TLS-test compatibility validation:

  • Reproduced the original untrusted-CA test failure with libcurl 8.18.0: one accepted connection, zero failed handshakes, certificate rejection, and zero HTTP requests.
  • libcurl 8.18.0: all 68 native catalog-connection tests and 41 key-parser tests passed; the live verified-TLS capture test passed.
  • libcurl 7.68.0: all seven HTTPS tests and the live verified-TLS capture test passed.

Full CI run passed on commit 9960491:

  • Ubuntu 26.04 CPU suite: 2,701 passed, one expected no-CUDA skip; package-layout and skip/collection gates passed.
  • Ubuntu 26.04 ClickHouse live suite: 291 passed, no skips.
  • Ubuntu 24.04 native-backend job: full CUDA build/import, sink binding, and ring-test compilation passed; host-only ring tests reported 151 and 21 assertions passed, zero failed.

@Samfisheryu Samfisheryu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed for correctness, performance, and clean-slate design. No blocking implementation issues found. The normalization preserves strict identifier and key-order validation, and parsing remains confined to startup schema checks.

Local validation: 72 tests passed — 41 parser, 10 schema/format, 14 record-storage against ClickHouse 25.12, and 7 HTTPS tests. The record-storage tests used a freshly built host backend, with the full-backend availability guard redirected to that verified build for this run. A focused check of the TLS terminator's untrusted and trusted paths also passed.

@XbzOnGit
XbzOnGit merged commit e43b404 into main Oct 25, 2026
3 checks passed
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