serhiy-bzhezytskyy opened a new pull request, #16475: URL: https://github.com/apache/lucene/pull/16475
### Description `CheckIndex` rejects a reader whose `Fields#iterator()` returns field names out of order, but the requirement is stated nowhere and checked nowhere else. What relies on the order is `MergedIterator`, used by `MultiFields#iterator` and by `PerFieldPostingsFormat#merge`. It documents *"the behavior is undefined if the iterators are not actually sorted"* rather than checking, so an unsorted producer yields wrong results with no error anywhere. Measured, with `removeDuplicates=true` as both callers use: | input | result | |---|---| | `[a,b]` + `[a,c]` — sorted | `[a, b, c]` — the shared name is deduplicated | | `[b,a,c]` + `[a,z]` — one unsorted | **`[a, b, a, c, z]`** — `a` twice, from two different sub-iterators | Meanwhile: - `Fields#iterator()` said only *"Returns an iterator that will step through all fields names"*. - `FieldsConsumer#write` has a `Notes` list of what an implementation must do and may assume, and did not mention the order, although `Lucene103BlockTreeTermsWriter` relies on it and `AssertingFieldsConsumer` has asserted it since 2013. - `FieldsProducer`, which every postings reader extends, had no mention either. - The comment on the `CheckIndex` check cites `MultiFieldsEnum`, which no longer exists. So a new `PostingsFormat` — a public extension point — can return `HashMap#keySet()`, pass the whole test suite, and only be caught later by `CheckIndex`, if anyone runs it. All thirteen `Fields` implementations in the repository do honour the order today, but by four different means: a `TreeMap` in four of them, an explicit sort on the way out in two, and in `FreqProxFields` a `LinkedHashMap` plus a comment relying on the caller having sorted first. ### Changes - `Fields#iterator()`, `FieldsConsumer#write` and `FieldsProducer` state the requirement, and `Fields#iterator()` also says what breaks without it. - `AssertingLeafReader.AssertingFields` asserts it. That class already wrapped the `Fields` that term vectors expose, so the check covers that path too; `AssertingPostingsFormat` calls the same helper rather than carrying a second copy. - `AssertingFieldsConsumer#write` now wraps the incoming `Fields`, so a violation fails during the write that caused it rather than in a later reader. This is part of the TODO that has sat in that method since 2013 (`017a3bf6281`). - The `CheckIndex` comment names `MultiFields` and `MergedIterator`. Doing the TODO's last part literally does not work, which is worth recording: `AssertingFieldsProducer` wraps `Terms` in `AssertingLeafReader.AssertingTerms`, which encodes read-side expectations, and on the write side the consumer pulls a `PostingsEnum` straight out of the incoming `Fields` and drives it differently. Reusing the producer trips `assert super.docID() == nextDoc` in `AssertingPostingsEnum` and fails three tests in `BasePostingsFormatTestCase`. So the wrapper is split: one class checks only the `iterator()` contract and is safe on both sides, a subclass adds the `Terms` wrapping for the read side. The *"limited CheckIndex"* half of the TODO is left in place, unaddressed. ### Relationship to #9967 That issue asks whether the order check can be removed from `CheckIndex`. It cannot: `MultiFields` still exists and still merges these iterators, so @jpountz's 2019 answer — *"We rely on the order for merging, see MultiFields"* — still holds. But the follow-up question in the same thread was never answered: > Should we make this more explicit and robust then? For E.g., since we do not explicitly maintain a sort order but rely on the key set to do the right thing, a change from `Collections.unModifiableSet` to `Set.copyOf` breaks this assertion in checkIndex This is an attempt at that. The check stays; the contract behind it is now written down and enforced where a codec author will see it. ### Verification Every claim the javadoc makes is pinned by a test, since a documented invariant that no test exercises is how the stale `MultiFieldsEnum` comment survived for thirteen years: | claim | test | |---|---| | ascending order from a real reader | `testCheckIndexAcceptsSortedFields` | | `CheckIndex` verifies it | `testCheckIndexDetectsFieldsOutOfOrder` | | `MultiFields` merges with `MergedIterator` | `testMultiFieldsMergesInOrder` | | `PerFieldPostingsFormat` does too | `testPerFieldMergePreservesFieldsExactlyOnce` | | unsorted input stops deduplicating | `testUnsortedInputBreaksDeduplication` | | the write side receives sorted names | `testWriteSideReceivesSortedFields` | | the order is not the insertion order | `testOrderIsIndependentOfInsertionOrder` | | the assertion fires, and only on violations | `TestAssertingPostingsFormat`, `TestAssertingLeafReader` | Each was mutation-checked: inverting the expectation, removing the `CheckIndex` check, bypassing the assertion, or reversing the names the recording consumer observes makes the corresponding test fail. `:lucene:core:test`, `:lucene:test-framework:test`, `:lucene:codecs:test`, `:lucene:backward-codecs:test`, `:lucene:memory:test` pass (11,452 tests), as do `:lucene:core:check` and `tidy`. Also run with `-Ptests.nightly=true -Ptests.iters=3 -Ptests.asserts=true` on the postings tests. ### One thing worth flagging The assertion is in `test-framework`, so a third-party `PostingsFormat` that violates the contract will start failing its own tests. That is what the assertion is for, but it is a behavioural change for downstream, so it is worth a deliberate decision rather than slipping in. I checked what is reachable: Solr has no `Fields` implementation of its own (`SchemaCodecFactory` only resolves format names through SPI). Of the implementations in Elasticsearch and OpenSearch, `DelegatingBloomFilterFieldsProducer` inherits its delegate's order, `TSDBSyntheticIdFieldsProducer` exposes a single field, and `Lucene40BlockTreeTermsReader` sorts explicitly. Two places have no guarantee either way — `XPerFieldMergeState`'s `new ArrayList<>(filterFields)`, and `TermVectorsFields` backed by a `HashMap` from `StreamInput#readMap`, though that one is on the term-vectors path. This is from reading their code, not from running their tests. -- 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]
