Throw exceptions if nextOrd called more than docValueCount times #17649
Conversation
msfroh
force-pushed
the
explosive_next_ord
branch
from
e185e8f to
c7b46c0
Compare
|
❌ Gradle check result for c7b46c0: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| @@ -365,6 +366,9 @@ public int docValueCount() { | |||
| @Override | |||
| public long nextOrd() throws IOException { | |||
| if (hasOrds) { | |||
| if (++ordCount > values.docValueCount()) { | |||
There was a problem hiding this comment.
@msfroh This is failing due to the check we are doing here.
The values here is MultiOrdinals$MultiOrds ( for TermsAggregatorTests #testBucketInTermsAggregationWithMissingValue ) and its docValueCount decreases with each call to nextOrd
public int docValueCount() {
return Math.toIntExact(currentEndOffset - currentOffset);
}
There was a problem hiding this comment.
This behavior is the same since Lucene 9.2.0 upgrade so shouldn't be a problem. WDYT ?
There was a problem hiding this comment.
Hmm... that implementation in MultiDocs seems wrong. The response for docValueCount() should only change when we move to another doc. If it's changing as a result of nextOrd calls, that's wrong.
|
❌ Gradle check result for 5ff7e4d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for a9da573: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Michael Froh <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
msfroh
force-pushed
the
explosive_next_ord
branch
from
a9da573 to
e2d6961
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17649 +/- ##
============================================
+ Coverage 72.29% 72.44% +0.15%
- Complexity 65900 66011 +111
============================================
Files 5350 5350
Lines 306185 306190 +5
Branches 44373 44375 +2
============================================
+ Hits 221347 221815 +468
+ Misses 66670 66193 -477
- Partials 18168 18182 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msfroh
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2 and
nknize
as code owners
msfroh
requested review from
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
msfroh
changed the title
server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public long nextOrd() throws IOException { | ||
| if (currentOffset == currentEndOffset) { | ||
| return SortedSetDocValues.NO_MORE_DOCS; | ||
| throw new IllegalStateException("nextOrd() called more than docValueCount() times"); |
There was a problem hiding this comment.
I am wondering if this exception and Called nextOrd after docValueCount have different meaning? If not, we should reuse the same message for all instances? Should we create custom exception for this case?
There was a problem hiding this comment.
The Called nextOrd after docValueCount was something generated by IntelliJ's auto-complete. This message is better IMO. I can make it consistent across the different implementations.
In practice, these checks should be short-lived. Lucene doesn't even do checks. The JavaDoc there just says "It is illegal to call this more than docValueCount() times for the currently-positioned doc." Some implementations will just return the same value over and over. Other implementations may do something else.
I added these exceptions to highlight the places where we were doing the wrong thing. The later commits on this PR were where I fixed those places. In theory, before we ship OpenSearch 3.0, we could probably remove these exceptions.
There was a problem hiding this comment.
Sure, let us make these exceptions consistent.
Description
This is a work in progress to try to catch cases where we're calling
nextOrd()more thandocValueCount()times onSortedSetDocValues.Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.