gautamworah96 commented on PR #12465: URL: https://github.com/apache/lucene/pull/12465#issuecomment-1655036794
> I was adding the assert to guarantee the consistency between disi.doc and disi.exists. Hmm. To me it feels like we are changing the current behavior of the (admittedly undefined) API for very little benefit. I agree that we should not have `IndexedDISI#advanceExactWithinBlock` return false while `disi.exists = true` but maybe we could handle this in a different way? For example, what if we assigned `disi.exists` to false here? Changing it this way would fix the behavior (of the inconsistency between disi.advance returning false and disi.exists being true) and would also not break the behavior of the API? What is happening with this assert is that clients who relied on earlier calls that requested backwards docids were earlier getting false as the answer and were working on that assumption. But with this assert, tests fail in a somewhat obstruse way.. > I'm not very sure if we need to maintain the undefined behavior to keep consistent with before ? IMO, it would be better to try to maintain the behavior of the API unless we see a strong compelling benefit (like much better code structure or user experience benefit) in changing it? > So it will make sense to me to add similar java doc on IndexedDISI#advanceExact if it is causing confusion. I definitely want to add some javadoc here. It took me quite some time to understand what the API did.. :) -- 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: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
