fix: preserve CDATA, comments and PIs when streaming nested XML

[teaguesterling]

Closes #158.

The report

The same document read through the DOM path and through SAX streaming produced different bytes for a captured XML fragment:

dom_sax_equal │ dom_cdata │ sax_cdata │            value             │          value
    false     │     4     │     0     │ <k><![CDATA[a < b & c]]></k> │ <k>a &lt; b &amp; c</k>

Cause

SAX does not serialize a DOM tree — it reconstructs the nested fragment from parser callbacks. The handler set registered exactly four:

handler.startElementNs = SAXStartElementNs;
handler.endElementNs   = SAXEndElementNs;
handler.characters     = SAXCharacters;
handler.cdataBlock     = SAXCdataBlock;   // delegated straight to SAXCharacters

Two consequences, one root cause:

1. cdataBlock delegating to characters meant a CDATA section arrived as ordinary text and was re-escaped, losing its framing.
2. libxml2 discards callbacks that are not registered, so comments and processing instructions never reached the accumulator at all and vanished from the fragment silently.

So CDATA was one of three, not a defect on its own. Measured on the pinned build, four of five nested cases diverged:

case	DOM	SAX (before)
cdata	<k><![CDATA[a < b & c]]></k>	<k>a &lt; b &amp; c</k>
comment	<k>x<!-- note -->y</k>	<k>xy</k>
mixed	<k>pre<![CDATA[mid & more]]>post</k>	<k>premid &amp; morepost</k>
pi	<k><?target data?>y</k>	<k>y</k>
entity	<k>a &lt; b &amp; c</k>	(already equal)

Which side is right

The DOM serializer is this extension's reference, and the SAX path already mirrors it deliberately elsewhere — XmlEscapeText escapes a literal CR as 
 specifically because "the same node serialized to different bytes under DOM vs SAX", and XmlEscapeAttr mirrors attribute-value normalization for "DOM/SAX byte-parity". CDATA, comments and PIs are the same contract, so SAX is the side that changes.

Preserving them is also the information-preserving direction: normalizing DOM instead would discard the distinction.

Fix

• cdataBlock emits <![CDATA[...]]> when reconstructing a nested fragment, closing and reopening the section around any ]]> exactly as libxml2's serializer does, so the reconstructed fragment still parses back to the same text.
• New comment and processingInstruction handlers emit <!--...--> and <?target data?> in the same context.

All three keep the existing depth contract: only relative_depth > 1 is a nested fragment. At depth 1 the value is scalar text, where a CDATA section contributes its unwrapped content and comments and PIs contribute nothing — matching xmlNodeGetContent on the DOM side.

That boundary matters, and the test pins it: preserving CDATA framing in fragments must not start leaking <![CDATA[ markers into scalar VARCHAR columns, and registering comment/PI handlers must not start injecting them into scalar text.

Verification

Built against the pinned duckdb submodule (d8cdaa33fd, v1.5.5). The checkout's previous build was 146 commits off-pin, so it was rebuilt at the pin before anything was measured.

After the fix, all five cases agree:

id	equal	value (both modes)
cdata	true	<k><![CDATA[a < b & c]]></k>
comment	true	<k>x<!-- note -->y</k>
entity	true	<k>a &lt; b &amp; c</k>
mixed	true	<k>pre<![CDATA[mid & more]]>post</k>
pi	true	<k><?target data?>y</k>

• test/sql/github_issue_158_dom_sax_parity.test: 31 assertions, all passing. Before the fix, four of its five nested rows compared false while the scalar control was already all true — so the test discriminates between the two paths rather than failing uniformly.
• Full suite: 4007 assertions in 103 test cases, all passing.

The fixture carries each payload twice — once nested as <s><k>…</k></s> and once as the leaf <t> — so the nested and scalar contracts are checked against the same input.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XoHU19qnngy1CJnXyX1csz

[Copilot]

🟡 Changes recommended

The new CDATA ]]> splitting logic is correctness-critical but currently lacks explicit regression coverage, and two newly added callbacks can be given internal linkage to avoid exporting unused global symbols.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes DOM vs SAX streaming byte-parity when reconstructing nested XML fragments by ensuring SAX preserves CDATA framing, comments, and processing instructions (Issue #158). This aligns the streaming reconstruction contract with the DOM serializer for nested fragments while keeping scalar-text behavior unchanged.

Changes:

• Implement CDATA re-framing in SAX nested-fragment reconstruction, including splitting embedded ]]> sequences.
• Add SAX handlers to serialize comments (<!-- -->) and processing instructions (<? ?>) into nested fragments.
• Add a DOM/SAX parity fixture and SQL regression test covering nested vs scalar behavior boundaries.

File summaries

File	Description
test/xml/dom_sax_parity.xml	Adds XML fixture cases for nested vs scalar parity (CDATA/comment/entity/mixed/PI).
test/sql/github_issue_158_dom_sax_parity.test	Adds regression tests asserting DOM/SAX byte parity for nested fragments and unchanged scalar-text semantics.
src/xml_sax_reader.cpp	Extends SAX handler set and nested-fragment reconstruction to preserve CDATA/comment/PI nodes.

Review details

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

---

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

[teaguesterling]

Good catch, and already addressed on main in 534127e — independently flagged by a second reviewer with the same reasoning: the splitting loop was the one genuinely new piece of logic and it shipped with no test exercising the case it exists for.

The fixture now carries a cdataclose row with the payload before]]>after, written as the standard two-section encoding, and the test asserts three things about it: DOM and SAX emit the same bytes, the scalar text column still yields the unwrapped before]]>after under both modes, and the reconstructed SAX fragment re-parses to the original text — which is the property that actually matters if the split were wrong.

Measured: both paths emit <k><![CDATA[before]]]]><![CDATA[>after]]></k>, and the round-trip returns before]]>after.

[teaguesterling]

Not taking this one, though it points at something real.

The sibling SAX callbacks — SAXStartElementNs, SAXEndElementNs, SAXCharacters, SAXCdataBlock — are all non-static and declared in src/include/xml_sax_reader.hpp:106-115. Giving internal linkage to only these two would make the callback set less consistent, not more. The file-local helpers cited as precedent (XmlEscapeText, XmlEscapeAttr, ResolveElementName, MatchesRecordTag) are helpers rather than libxml2 callbacks, so they follow a different convention.

You did find a genuine inconsistency, just not this one: these two are the only SAX callbacks not declared in the header. I'm fixing it that way instead — adding the declarations alongside the other four — in a follow-up.

[teaguesterling]

Not taking this one, though it points at something real.

The sibling SAX callbacks — SAXStartElementNs, SAXEndElementNs, SAXCharacters, SAXCdataBlock — are all non-static and declared in src/include/xml_sax_reader.hpp:106-115. Giving internal linkage to only these two would make the callback set less consistent, not more. The file-local helpers cited as precedent (XmlEscapeText, XmlEscapeAttr, ResolveElementName, MatchesRecordTag) are helpers rather than libxml2 callbacks, so they follow a different convention.

You did find a genuine inconsistency, just not this one: these two are the only SAX callbacks not declared in the header. I'm fixing it that way instead — adding the declarations alongside the other four — in a follow-up.