Skip to content

ci: add centralized docs drift check - #669

Merged
alex-clickhouse merged 1 commit into
mainfrom
joe/docs-drift-check
Oct 14, 2026
Merged

alex-clickhouse merged 1 commit into
mainfrom
joe/docs-drift-check

Conversation

@joe-clickhouse

Copy link
Copy Markdown
Contributor

Summary

Adds the shared docs-drift check already used by clickhouse-connect, dbt-clickhouse, and clickhouse-go. It reviews PR changes against this repo's docs and maintains an advisory needs-docs label and one sticky comment. A later docs fix clears the label and marks the comment resolved.

The custom agent definition is a solid first attempt based on the repo's current docs and public APIs. It covers the HTTP and experimental TCP clients, ADO.NET, type mappings, configuration, compression, NuGet packaging, and runnable examples. We can update or change it at any time, before or after this PR merges.

Automatic checks skip fork and Dependabot PRs. Maintainers can check reviewed PRs manually. The worker reads the agent definition from the trusted PR base, so this will start taking effect after merge.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 21:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The workflow integration and repository-specific review rubric are consistent with the shared implementation and current project structure.

0 open findings

What changed in this PR

Adds an advisory, centralized documentation-drift check without changing client runtime behavior.

Changes:

  • Adds automatic and manual workflow triggers for documentation review.
  • Defines repository-specific documentation ownership and drift rules.
File Description
.github/​workflows/​docs_drift_check.yml Dispatches eligible PRs to the shared docs-drift worker.
.claude/​agents/​docs-drift-reviewer.md Defines documentation scope, routing, and review behavior.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@polyglotAI-bot

Copy link
Copy Markdown
Collaborator

Summary
The PR adds documentation-drift guidance for the HTTP and experimental native TCP clients without changing client behavior. No actionable findings were identified in the rubric. Workflow files were excluded as required, and validation was limited to static inspection. ✅ Approve.

Final verdict

✅ Approve

Automated review by Polyglot AI (gpt-6-astra) of 3eeeae4. Replies on the inline threads are read on the next review. A maintainer can ask for a new review by mentioning @polyglotAI-bot or requesting a review from it.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3eeeae4. Configure here.

Comment thread .github/workflows/docs_drift_check.yml Resolved
@alex-clickhouse
alex-clickhouse merged commit 860fe1a into main Oct 14, 2026
5 checks passed
@alex-clickhouse
alex-clickhouse deleted the joe/docs-drift-check branch October 14, 2026 11:45
xwwwx pushed a commit to xwwwx/clickhouse-cs that referenced this pull request Oct 24, 2026
…e#448)

* Remove per-batch StreamWriter allocation in binary-insert serializers

The query line preceding each batch's rows (e.g. INSERT INTO ... FORMAT
RowBinary) was written via a StreamWriter, which allocated the writer plus a
4 KiB char buffer and a matching byte buffer on every batch. Encode it into a
pooled scratch buffer (new internal QueryLineWriter) and write directly instead,
removing one allocation per batch on both the object[] and POCO insert paths.

As a side effect the query line is now emitted as plain UTF-8 with a '\n'
terminator: the BOM that StreamWriter prepended is gone, and the newline is
'\n' on all platforms (was '\r\n' on Windows via Environment.NewLine).
ClickHouse accepts both, so behavior is unchanged.

Benchmark (200k-row POCO RowBinary insert, uncompressed, ENGINE Null):
  BatchSize 500  (400 batches): 7783.7 KB -> 4503.4 KB (-42%), Gen0 eliminated
  BatchSize 5000 (40 batches):   811.5 KB ->  482.0 KB (-41%)
~8.2 KB saved per batch; scales with batch count.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* adjust changelog/releasenotes

* QueryLineWriter: guard null target, tolerate null query

Address PR review: Write threw NullReferenceException on a null query (via
query.Length). Restore the prior StreamWriter.WriteLine(null) tolerance by
treating null as empty (writes only the newline), and throw a clear
ArgumentNullException for a null target instead of an NRE.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Rename QueryLineWriter to PooledStreamWriter, Write to WriteLine

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Avoid per-row byte[] allocation in fixed-size binary reads (ClickHouse#669)

The read/deserialization path allocated a fresh byte[] per value via
ExtendedBinaryReader.ReadBytes(int). For fixed-size types the bytes are
consumed locally (BigInteger/IPAddress copy them out), so the array is
pure garbage.

Add an internal guaranteed-read ReadBytes(Span<byte>) helper and read
into a stack buffer instead:
- Int128/256, UInt128/256 (AbstractBigIntegerType): stackalloc + single
  new BigInteger(span, isUnsigned: !Signed) call, collapsing the old
  byte-by-byte unsigned path.
- Decimal128/256 mantissa (DecimalType): stackalloc + new BigInteger(span).
- IPv4/IPv6: stackalloc + new IPAddress(span).
- FixedString (default string mode): stackalloc for Length <= 256, else a
  pooled buffer, feeding Encoding.UTF8.GetString(span). The ReadAsByteArray
  mode is unchanged since that buffer escapes as the column value.

Behavior is unchanged (two's-complement decoding, IPv4 byte reversal).
In a 200k-row single-column read benchmark, managed allocations per read
dropped ~33-67% (e.g. UInt256 18.3 MB -> 6.1 MB) with fewer gen0 GCs; the
residual per-row alloc is the boxed result object (ClickHouse#509 territory).

Adds ReadDecodeBenchmark and server-free tests covering the FixedString
stackalloc/pool boundary, ReadBytes(Span) fill/underflow, and signed/
unsigned BigInteger high-bit decode.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* perf(read): override PeekableStreamWrapper.Read(Span<byte>) for direct span reads

ReadBytes(Span<byte>) routed through the default Stream.Read(Span<byte>),
which rents an ArrayPool buffer and copies per call — allocation-free but
adding rent/copy overhead in the tight per-row read loop. Override it to
read directly into the span, preserving the peeked read-ahead byte
semantics (including peek-past-EOF, which now returns 0). Addresses PR
review feedback (ClickHouse#448).

* test: cover PeekableStreamWrapper.Read(Span<byte>) edge branches

Adds direct tests for the two defensive branches in the new span-read
override: an empty span read while a byte is buffered from Peek (must not
write into the zero-length buffer or consume the pending byte), and a span
read after peeking past end-of-stream (must return 0, not 0xFF). The
Read(Span<byte>) method is now fully covered.

* docs: add XML comment to PeekableStreamWrapper.Read(Span<byte>)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: remove ReadDecodeBenchmark from the PR

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.

4 participants