Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped #12900

gf2121 · 2023-12-10T06:39:10Z

Relate to : #12895

mikemccand · 2023-12-10T11:25:45Z

OK I opened #12901 to create a better BWC test revealing these read-time exceptions -- can we do that, first, and then confirm #12900 indeed fixes it, then push the test, then push the fix? I can try to work on creating that test case, but won't have much time until early tomorrow AM. If anyone else wants to crack it, feel free!

mikemccand · 2023-12-11T14:06:59Z

I've confirmed the new (failing) BWC test from #12901 now passes with this PR. I'll review ...

mikemccand

Thanks @gf2121! I left a few head scratching comments ...

mikemccand · 2023-12-11T15:49:38Z

...ne/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java

@@ -2245,7 +2244,6 @@ public void testFailOpenOldIndex() throws IOException {
  // #12895: test on a carefully crafted 9.8.0 index (from a small contiguous subset
  // of wikibigall unique terms) that shows the read-time exception of
  // IntersectTermsEnum (used by WildcardQuery)
-  @Ignore("re-enable once we merge #12900")


mikemccand · 2023-12-11T15:51:26Z

lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/IntersectTermsEnumFrame.java

@@ -89,6 +89,9 @@ final class IntersectTermsEnumFrame {

  final ByteArrayDataInput bytesReader = new ByteArrayDataInput();

+  // Cumulative outputs so far
+  BytesRef[] outputPrefix;


It's sort of strange to have a separate array for tracking the accumulated BytesRef outputs when this is the purpose of the outputAccumulator? Wouldn't we be able to push/pop as we push/pop the frames, into the single accumulator, so we don't need make extra arrays as we push deeper into the FST?

We could assert when we pop that the output we are popping exactly matches (.equals()) the FST.Arc's output from that frame.

Wouldn't we be able to push/pop as we push/pop the frames, into the single accumulator, so we don't need make extra arrays as we push deeper into the FST?

I'm not sure. Looking at pushFrame, it seems like one single frame could be related to more than one FST#Arc. Maybe we need to record the number of outputs pushed into the accumulator when pushing a frame ?

I like this idea to avoid the array copy. Implemented in e387ec9.

mikemccand · 2023-12-11T15:52:54Z

lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/IntersectTermsEnum.java

@@ -198,6 +204,7 @@ private IntersectTermsEnumFrame pushFrame(int state) throws IOException {
    }

    f.arc = arc;
+    f.outputPrefix = outputAccumulator.bytesRefs();


Hmm shouldn't we sometimes set this outputPrefix back to null? The frames are re-used, so we could push deep into the FST, where there is an outputPrefix, then pop back out and push into a new part of the FST that does not have an outputPrefix and illegally share / use the old leftover stale outputPrefix?

push into a new part of the FST that does not have an outputPrefix and illegally share / use the old leftover stale outputPrefix?

I thought the outputPrefix will be always overwritten by pushFrame when we push into a new part of the FST. Did i miss something ?

I thought the outputPrefix will be always overwritten by pushFrame when we push into a new part of the FST. Did i miss something ?

Ahh OK you are right -- and we could simply rewrite to empty BytesRef array which means the same thing as null. Good.

gf2121 · 2023-12-12T09:23:05Z

lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/SegmentTermsEnum.java

@@ -495,7 +495,7 @@ public boolean seekExact(BytesRef target) throws IOException {
      targetUpto = 0;
      outputAccumulator.push(arc.nextFinalOutput());
      currentFrame = pushFrame(arc, 0);
-      outputAccumulator.pop();
+      outputAccumulator.pop(arc.nextFinalOutput());


This seems like another potential bug : If arc.nextFinalOutput() == NO_OUTPUT, we will not add the output into the accumulator but we pop one out.

Not sure why we have not seen any bug report for this. Maybe our output format protect it implicitly? This is in SegmentTermsEnum so it could possibly affect merge. I'll dig to see if it is a real bug.

Good catch! And the new check you added in pop will prevent this bug, right?

But I also think this was (luckily!?) not a bug, because this case is when the STE is not yet seek'd to any term, so we are starting at the FST entry arc / root block, and the empty string FST input must always have a final output (the "root block"), which will never be NO_OUTPUT. Are there any other places in STE where we were blindly popping like this?

It looks like the other places where we "blindly" pop are fine -- we conditionalize by arc.isFinal() check, and we know FST for BlockTree terms index will always have a nextFinalOutput on final nodes since it holds the floor data for that terms block. And there there is one other place in seekCeil where we also rely on empty string having a final output, which is fine. So net/net I don't think this was a bug, but it smelled close to one!

mikemccand · 2023-12-12T10:42:47Z

lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/IntersectTermsEnum.java

@@ -198,9 +199,11 @@ private IntersectTermsEnumFrame pushFrame(int state) throws IOException {
    }

    f.arc = arc;
+    f.outputNum = outputAccumulator.outputCount() - initOutputCount;


OK we need this because a single frame might traverse multiple FST arcs, so we need to accumulate possibly / often more than one output.

mikemccand

This looks good to me! I like the added safety of confirming on pop that we are in fact poping what we think we are (except when we pop multiple items). And it's cleaner than the first cut by simple using the output accumulator consistently.

I think we should merge this to main, 9x and 99x and let's let CI chew on it for a bit (day or so) before cutting 9.9.1?

gf2121 · 2023-12-12T12:18:38Z

Thanks for review and great advice @mikemccand !

I think we should merge this to main, 9x and 99x and let's let CI chew on it for a bit (day or so) before cutting 9.9.1?

+1. I'll merge and backport soon.

jpountz · 2023-12-12T12:19:05Z

FWIW I confirmed that this change makes the new test in #12925 pass.

… and popped (#12900)

bug fix

26e166c

gf2121 mentioned this pull request Dec 10, 2023

Corruption read on term dictionaries in Lucene 9.9 #12895

Closed

mikemccand mentioned this pull request Dec 10, 2023

Improve BWC tests to reveal #12895 and confirm its fix #12901

Closed

gf2121 added 2 commits December 10, 2023 20:12

add an assert for OutputAccumulator#push

722a96b

lazy load

85f8c96

javanna mentioned this pull request Dec 11, 2023

Revert "Optimize outputs accumulating for SegmentTermsEnum and InterssectTermsEnum " #12899

Closed

uschindler added this to the 9.9.1 milestone Dec 11, 2023

gf2121 mentioned this pull request Dec 11, 2023

Add BWC test to reveal #12895 #12912

Closed

gf2121 added 3 commits December 11, 2023 22:10

Merge remote-tracking branch 'origin/main' into fix_accumulator_bug

fe1e0fe

remove ignore

eba6fe6

tidy

2e33baf

mikemccand reviewed Dec 11, 2023

View reviewed changes

gf2121 added 3 commits December 12, 2023 15:23

push and pop outputs with frames

e387ec9

rename

f00f604

outputCount no need public

bf54d6f

gf2121 commented Dec 12, 2023

View reviewed changes

mikemccand reviewed Dec 12, 2023

View reviewed changes

mikemccand approved these changes Dec 12, 2023

View reviewed changes

gf2121 changed the title ~~IntersectTermsEnum should accumulate from output prefix instead of current output~~ Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped Dec 12, 2023

gf2121 merged commit 05b14e2 into apache:main Dec 12, 2023
4 checks passed

asfgit pushed a commit that referenced this pull request Dec 12, 2023

Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed…

82f3f06

… and popped (#12900)

asfgit pushed a commit that referenced this pull request Dec 12, 2023

Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed…

ae6e1de

… and popped (#12900)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped #12900

Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped #12900

gf2121 commented Dec 10, 2023

mikemccand commented Dec 10, 2023

mikemccand commented Dec 11, 2023

mikemccand left a comment

mikemccand Dec 11, 2023

mikemccand Dec 11, 2023

gf2121 Dec 12, 2023

gf2121 Dec 12, 2023

mikemccand Dec 11, 2023

gf2121 Dec 12, 2023 •

edited

Loading

mikemccand Dec 12, 2023

gf2121 Dec 12, 2023 •

edited

Loading

mikemccand Dec 12, 2023

mikemccand Dec 12, 2023

mikemccand Dec 12, 2023

mikemccand left a comment

gf2121 commented Dec 12, 2023

jpountz commented Dec 12, 2023

Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped #12900

Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped #12900

Conversation

gf2121 commented Dec 10, 2023

mikemccand commented Dec 10, 2023

mikemccand commented Dec 11, 2023

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf2121 Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf2121 Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemccand left a comment

Choose a reason for hiding this comment

gf2121 commented Dec 12, 2023

jpountz commented Dec 12, 2023

gf2121 Dec 12, 2023 •

edited

Loading

gf2121 Dec 12, 2023 •

edited

Loading