Repository navigation
Conversation
An axis step with a name test returned the correct result on its first evaluation and an empty sequence on every evaluation after that, within a single query, whenever the context node carried a predicate context item. Silent -- no error, no warning, and data loss proportional to loop length. The duplicate-suppression guard in selectPrecedingSiblings, selectFollowingSiblings, selectPreceding and selectFollowing decided whether it had already handled a node by reading a context stamp back off the NodeProxy. Those proxies are shared with LocationStep's cached structural-index result, which is held for a whole query execution, so on the second evaluation the guard was reading stamps left by the first and suppressed every node. Each call now records which nodes it has stamped and with which context id, in a map local to that call, and the guard consults that instead of the proxy. Within one call the behavior is identical to before; across calls there is nothing to leak, whatever the caller does with the cache. The recorded value is exactly what the guard used to read off the proxy, so the transformation is faithful rather than a reinterpretation. The preceding/following guards carry an extra contextId != NO_CONTEXT_ID condition that the sibling ones do not; that asymmetry is preserved at the call sites rather than folded into the helper. The guard itself is still needed: with it disabled the new regression test passes, but ten existing tests fail across axes.xql, npt.xqm and positional-nested.xql. It suppresses genuine duplicates between different reference nodes within one evaluation. The bug is where that state was stored, not the guard. Cost: a small map allocated per call when a context id is in play, bounded by the number of nodes actually stamped rather than by the size of the cached set. Extracting the five-clause guard into a named helper also drops PMD NPath complexity in every method it touches -- selectPrecedingSiblings 27865 to 13285, selectFollowingSiblings 11125 to 5293, selectFollowing 831 to 471, and selectPreceding below the reporting threshold entirely. The range index named in the report is not required to trigger this; any predicate produces the context item that does. Nor is it a regression: both the caching and the guard are identical in the eXist-6.4.1 tag and current develop. SiblingAxisRepeatedEvaluationRegressionTest evaluates each affected axis four times over the same node and asserts stability, pinning the wildcard form, a node reached without a predicate, an unaffected axis, and a non-matching name alongside. Two of its eight cases fail on unfixed develop; the preceding:: and following:: cases pass either way and are pins rather than demonstrations. Addresses eXist-db#6690 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📊 XQTS result comparisonComparison of this run against Warning 70 test cases were recorded in only one of the two runs (0 only in the previous run, 70 only in the current run). The runner's JUnit output is not fully deterministic (see eXist-db/exist-xqts-runner#74), so totals and per-category deltas include recording noise; the newly passing/failing lists count only tests recorded in both runs.
Relative to 🔴 Newly failing tests (1)
⚪ Recorded only in this run (70)
Runtime: 467.3s (+58.32s vs |
duncdrum
left a comment
duncdrum
left a comment
There was a problem hiding this comment.
I prefer this approach compared to option A
duncdrum
left a comment
duncdrum
left a comment
There was a problem hiding this comment.
Thanks for this fix! I read through the diff and found nothing blocking. Two small suggestions inline.
| * @param idx the index into {@link #nodes} of the node just stamped | ||
| */ | ||
| private void recordStamp(final Map<Integer, Integer> stamped, final int idx) { | ||
| if (nodes[idx].getContext() != null) { |
There was a problem hiding this comment.
Small thought: this records whatever the proxy's context is after the stamping block, not the context id this call actually applied. When contextId == IGNORE_CONTEXT the block is skipped, and for NO_CONTEXT_ID propagatePredicateContextFrom may leave the head context unchanged. In both cases the map could hold a stale id from an earlier evaluation, and a later reference in the same call carrying that id would be suppressed as a duplicate (the #6690 symptom, just narrower).
It looks limited to the NO_CONTEXT_ID path of the sibling selectors, so maybe not a big deal. Recording the contextId that was actually stamped, and only when addContextNode ran, would sidestep it. WDYT?
| } | ||
|
|
||
| @Test | ||
| public void precedingSiblingNameTestIsStableAcrossEvaluations() throws XMLDBException { |
There was a problem hiding this comment.
Nice coverage of the repeated-evaluation case! These all use a single context node per call, with the context id coming from a predicate. It might be worth adding a case with two context nodes sharing a sibling in one call, e.g. (//a, //b)/following-sibling::c, since that is what the per-call stamped map is there for. A NO_CONTEXT_ID case would also cover the point above.
This branch has not been deployed
2 participants
[This PR was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
This is one of two candidate fixes for #6690. See #6700 for the design question, the trade-offs, and a link to the alternative candidate. Please do not merge this without reading that issue — the two PRs are deliberately competing, not stacked.
Addresses #6690
Summary
An axis step with a name test returned the correct result on its first evaluation and an empty sequence on every evaluation after that, within a single query, whenever the context node carried a predicate context item. Silent — no error, no warning, data loss proportional to loop length.
What Changed
exist-core/src/main/java/org/exist/dom/persistent/NewArrayNodeSet.javaThe duplicate-suppression guard in
selectPrecedingSiblings,selectFollowingSiblings,selectPrecedingandselectFollowingdecided whether it had already handled a node by reading a context stamp back off theNodeProxy. Those proxies are shared withLocationStep's cached structural-index result, which is held for a whole query execution — so on the second evaluation the guard was reading stamps left by the first, and suppressed every node.This change stops the guard reading the proxy. Each call now records which nodes it has stamped and with which context id, in a map local to that call, and the guard consults that instead. Within one call the behavior is identical to before; across calls there is nothing to leak, whatever the caller does with the cache.
Two small private helpers,
alreadyStampedInThisCallandrecordStamp, keep the four methods readable and the transformation obviously faithful — the recorded value is exactly what the guard used to read off the proxy.Note the preceding/following guards carry an extra
contextId != NO_CONTEXT_IDcondition that the sibling ones do not; that asymmetry is preserved at the call sites rather than folded into the helper.Why not simply remove the guard
Removing it is not an option. With it disabled the new regression test passes, but ten existing tests fail across
axes.xql,npt.xqmandpositional-nested.xql— including the file the guard's original 2019 commit added its tests to. It suppresses genuine duplicates between different reference nodes within one evaluation. The bug is where that state was stored, not the guard.Cost
A small map allocated per call when a context id is in play. It is bounded by the number of nodes actually stamped, not by the size of the cached set, so it scales with the result rather than the index — but it is an allocation in a hot path where there was none, and that is a fair thing to weigh against candidate A's smaller footprint. Discussed in #6700.
Test Plan
SiblingAxisRepeatedEvaluationRegressionTestfails on unfixeddevelop, passes with the fixxquery.CoreTests— the wholesrc/test/xquerycorpus, including the ten tests naive guard removal breaks — greenmvn testonexist-coregreenlicense:checkcleanScope
One class, four methods, no behavior change within a single evaluation.
🤖 Generated with Claude Code