chore: address the automated review left on #165 and #166

[teaguesterling]

Addresses the automated review comments left on #165 and #166, which went unread when those PRs were merged. The review requirement was bypassed with --admin; that skips the gate but does not make the review disappear, and two of the five PRs said "Changes recommended".

No behaviour change. Both fixes land after the v2.9.1 tag (cdd866f), so the release and registry PR duckdb/community-extensions#2719 are unaffected — a docs-table inconsistency and a linkage nit do not justify moving a published tag.

#166 — BOOLEAN table was internally inconsistent (accepted)

The row advertised on/off in its detection pattern but omitted them from the examples column:

   * - BOOLEAN
     - true/false, yes/no, on/off
     - ``true``, ``false``, ``yes``, ``no``      <- on/off missing

Fair catch, and it was my edit — it shipped that way in v2.9.1. The examples now match the pattern.

#165 — internal linkage for the two new callbacks (declined, with a different fix)

The review recommended marking SAXComment and SAXProcessingInstruction static, to keep them "consistent with other file-local helpers".

I don't think that's right here. The four sibling SAX callbacks are all non-static and declared in the header:

src/include/xml_sax_reader.hpp:106-115
  void SAXStartElementNs(...);
  void SAXEndElementNs(...);
  void SAXCharacters(...);
  void SAXCdataBlock(...);

Giving internal linkage to only the two newest callbacks would make that set less consistent. The precedent cited — XmlEscapeText, XmlEscapeAttr, ResolveElementName, MatchesRecordTag — are file-local helpers, which follow a different convention from libxml2 callbacks.

The review did find a real inconsistency, just not the one it named: these two were the only SAX callbacks missing header declarations. That's what this PR fixes, which is the direction this file's own convention points.

The reasoning is posted on the #165 comment threads rather than the suggestion being quietly dropped.

#165 — CDATA ]]> coverage (already fixed)

The third comment noted that the ]]> splitting path — the one genuinely new piece of logic in that PR — shipped with no test exercising it. Already fixed on main in 534127e, independently flagged by a second reviewer with the same reasoning: the fixture gained a cdataclose case carrying before]]>after, asserting DOM/SAX byte parity, unchanged scalar text, and that the reconstructed fragment re-parses to the original text.

Noted, not changed

xml_sax_reader.hpp:106 reads // SAX2 callback functions (static, matching libxml2 signatures). The word "static" is inaccurate for functions that aren't, and is plausibly what led the review to misread the convention. Left alone as pre-existing wording rather than widening a two-line follow-up.

Verification

Built against the pinned duckdb submodule (v1.5.5, d8cdaa33fd).

• Build clean — the header declarations compile, and declaration/definition now line up (hpp:121,123 ↔ cpp:410,425).
• Full suite: 4072 assertions in 106 test cases, all passing.
• make check-docs: 105 examples, 0 BROKEN — unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XoHU19qnngy1CJnXyX1csz

[Copilot]

🟢 Approval recommended

The changes are small, consistent with existing conventions, and appear to be a straightforward docs/header alignment with no behavioral impact.

Pull request overview

This PR applies follow-up fixes that address previously missed automated review feedback from PRs #165 and #166, without changing runtime behavior. It aligns the SAX callback declarations with the project’s existing header/implementation structure and corrects a documentation table inconsistency in schema inference docs.

Changes:

• Add missing header declarations for SAXComment and SAXProcessingInstruction alongside the other libxml2 SAX callbacks.
• Fix BOOLEAN examples in the schema inference documentation to include on/off to match the stated detection pattern.

File summaries

File	Description
src/include/xml_sax_reader.hpp	Declares the comment and processing-instruction SAX callbacks in the header to match their existing definitions/usage.
docs/schema_inference.rst	Updates the BOOLEAN examples list to include on/off, making the docs table internally consistent.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

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