vismaytiwari opened a new pull request, #16402:
URL: https://github.com/apache/lucene/pull/16402
### Description
Fixes #15769.
`PathHierarchyTokenizer` is documented to emit each path prefix at the
*same* position, so that query parsers treat them as synonyms — a query for
`Books/NonFic/Science/Physics` becomes `(Books OR Books/NonFic OR
Books/NonFic/Science OR Books/NonFic/Science/Physics)`. That "ancestor path"
use case has been broken since 10.0: GITHUB#12875 (5d6086e) changed every token
to `positionIncrement = 1`, so the parser now builds a *phrase* instead and the
query matches nothing.
As @hossman points out in the issue, GITHUB#12875 was motivated purely by
making the tokenizer pass `assertAnalyzesTo()`, not by any real-world problem,
and it silently regressed the documented behavior.
This restores `positionIncrement = 0` for every token after the first (the
first still advances by 1). The alternative floated in the issue — keeping
incrementing positions but adding a `PositionLengthAttribute` — doesn't work:
`posLen > 1` pushes `QueryBuilder` onto its graph (`isGraph`) code path and
`testAncestorQuery` still fails. Plain synonyms are what the design intended
and what the query builders expect.
The one cost is that the overlapping-prefix offsets are not a valid token
graph, so `assertAnalyzesTo()` can't validate them under its strict
graph-offset check. The tokenizer test passes `graphOffsetsAreCorrect = false`
for these inputs. That's consistent with how the tokenizer is already treated
elsewhere — it's annotated `@IgnoreRandomChains(reason = "broken offsets")` and
`checkRandomData` is already called with `offsetsAreCorrect = false`.
### Tests
- `testAncestorQuery` / `testDescendantQuery`: end-to-end regression tests
for both documented use cases (adapted from Hoss Man's patch on the issue),
asserting exact hit counts.
- `testTokenizerViaAnalyzerOutput` and the existing
`assertTokenStreamContents` cases assert the `positionIncrement` `{1, 0, 0,
...}` pattern again.
### Note for reviewers
GITHUB#12875 made the same `positionIncrement = 1` change to
`ReversePathHierarchyTokenizer`, which likely has the same latent problem for
its mirror use case. I kept this PR scoped to the forward tokenizer that #15769
is about — happy to follow up on the reverse one separately if that's wanted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
