Merge entity-split text nodes after parsing - #3
Open
gthb wants to merge 1 commit into
Open
Conversation
sxd-document emits a separate text node per character-data token, so every entity reference splits the surrounding text into sibling text nodes (A&B parses as three). That violates the XPath 1.0 data model (maximal text runs) and broke text()-based queries in two ways: - contains(text(), "X") silently tested only the first fragment, missing content that follows an entity reference; - string-converting a multi-node nodeset sends sxd-xpath through Nodeset::document_order_first, which rebuilds a document-order index of the entire document per predicate evaluation. Over a 40MB worksheet with ~35k entity-bearing formulas this is effectively unbounded: a corpus query that completes in 1m34s with this fix previously ran 10.5 hours without finishing (and one worker reached 7GB RSS). Normalize each parsed document once (the standard DOM normalize() operation) before evaluation, restoring maximal text runs so text() nodesets are single-node and stay on sxd-xpath's fast path. Known residual: genuinely distinct multi-element nodesets (e.g. contains(x:v, ...) with several v children) still hit the slow sxd path; that needs an upstream sxd-xpath fix (caching DocOrder).
Author
Considering that project's activity we should perhaps not hold our breath 😊 |
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
1 participant
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
Merge runs of adjacent sibling text nodes after parsing each XML part, so
text()-based XPath predicates evaluate correctly, in reasonable time, on entity-bearing content.Why
sxd-document creates a separate text node per character-data token, so entity and character references and CDATA sections split the surrounding text into adjacent sibling text nodes:
IF(C9>0,OFFSET(…))parses as several text nodes. That violates the XPath 1.0 data model (§5.7: "a text node never has an immediately following or preceding sibling that is a text node") and breakscontains(text(), "…")twice over:containsmisses matches where the needle follows an entity reference (common in spreadsheet formulas)A
contains(text(),"OFFSET(")scan of a ~3,050-workbook corpus ran 10.5 hours without completing (one 40 MB sheet with ~35k entity-bearing formulas takes over 6 minutes by itself). With this fix the corpus completes in 1m34s, and finds matches which the old behaviour missed.How
normalize_text_nodes()performs the standard DOMnormalize()operation once per parsed document, merging each run of adjacent sibling text nodes. With maximal text runs restored,text()nodesets are single-node — the correct data model and sxd-xpath's fast path.Out of scope
A genuinely multi-node nodeset being string-converted (e.g.
contains(x:v, "…")over severalvchildren) still rebuilds the index per evaluation. That's left to (sxd-path#121), no workaround attempted here.Note
I contributed a fix for the root cause upstream in sxd-document#101 (joining character data during parsing). If that is accepted and released, this workaround becomes redundant and can be dropped along with the dependency bump.