Join adjacent character data into one text node per run - #101
Open
gthb wants to merge 1 commit into
Open
Conversation
The DOM builder created a separate text node for every character-data
token, so entity and character references split the surrounding text
into adjacent sibling text nodes: <a>A&B</a> parsed as three text
nodes. Entities are fully decoded during parsing (there is no entity
node kind in the DOM), so the split carried no information; it was an
artifact of add_text_data creating a node per token. Attribute values
already accumulate their literal and reference fragments into a single
string.
Adjacent sibling text nodes violate the XPath 1.0 data model, which
requires maximal text runs ('a text node never has an immediately
following or preceding sibling that is a text node', XPath 1.0 section
5.7), and sxd-xpath consumes this DOM directly. The split nodes make
text()-based predicates both wrong and slow there: contains(text(), s)
tests only the first fragment, and string-converting a multi-node
nodeset rebuilds a whole-document order index per evaluation
(sxd-xpath#121). Coalescing restores the single-node fast path: an
XPath consumer scanning a 40MB worksheet with ~35k entity-bearing
<f> elements went from killed-after-6-minutes to ~7s, with strictly
more correct matches.
Buffer character data in the DOM builder and emit one text node per
contiguous run, flushing when a child element, comment, or processing
instruction interrupts, or when the containing element closes. Update
the three tests that asserted the split shape; add coverage for
coalescing across entity/char references and CDATA, and for runs
legitimately split by a comment or child element.
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.
What
The DOM builder creates a separate text node for each character-data token, so entity and character references and CDATA sections split the surrounding character data into adjacent sibling text nodes:
The cause:
add_text_datadoescreate_text+append_childonce perCharData/CDatatoken and once per decoded reference fromContentReference.This PR joins each contiguous run of character data into a single text node during parsing.
Why
The split doesn't seem deliberate or desired:
it preserves no information; references are decoded during parsing, and the DOM has no node kinds for entities or CDATA sections.
it departs from attribute values, which already coalesce into a single string.
it violates the XPath 1.0 data model, which sxd-xpath consumes directly from this DOM. XPath 1.0 §5.7 (Text Nodes):
In sxd-xpath, the split breaks both correctness and performance of
text()-based expressions:contains(text(), "needle")converts thetext()nodeset to a string by taking only the first node in document order, so it tests only the first fragment, missing any content after a reference.For nodesets with more than one node,
Nodeset::document_order_firstrebuilds a document-order index of the entire document per evaluation (Computation of document order needs to be cached sxd-xpath#121). With split text nodes, every entity-bearing element takes that path.Real-world impact: scanning a 40 MB spreadsheet worksheet XML (in xlpath) with ~35k formula elements containing
"/>for//f[contains(text(),"OFFSET(")]-style queries:before: over 6 minutes before being killed, missing matches wherever the needle followed a reference;
after: ~7 seconds, with more correct results.
(This does change the node count/identity that consumers observe. But code that relies on that split ... probably shouldn't :))
How
Buffer character data instead of emitting text nodes, in the DOM builder. Flush and emit a text node when a child element, comment, or processing instruction interrupts, or when the containing element closes.
Add tests covering coalescing across references and CDATA, and runs legitimately split by a comment or a child element.
Update three existing tests which asserted the split.
Out of scope
Could add a DOM-style
Node.normalize()API, complementing this, since adjacent text nodes can still be constructed programmatically via repeatedappend_child(create_text(..)). But parse output seems worth fixing regardless.