You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "gf2121 (via GitHub)" <gi...@apache.org> on 2023/12/10 06:39:12 UTC
[PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
gf2121 opened a new pull request, #12900:
URL: https://github.com/apache/lucene/pull/12900
Relate to : https://github.com/apache/lucene/issues/12895
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1422705408
##########
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();
Review Comment:
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`?
##########
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;
Review Comment:
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.
##########
lucene/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")
Review Comment:
Yay :)
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423704366
##########
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());
Review Comment:
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.
##########
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());
Review Comment:
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423827461
##########
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());
Review Comment:
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!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423808624
##########
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;
Review Comment:
OK we need this because a single frame might traverse multiple FST arcs, so we need to accumulate possibly / often more than one output.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423493276
##########
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();
Review Comment:
> Hmm shouldn't we sometimes set this outputPrefix back to null
We never set `outputPrefix` back to null. But a `BytesRef[0]` here is equivalent, which means all prefix output (including current frame) are all `NO_OUTPUT`.
> 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 ?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #12900:
URL: https://github.com/apache/lucene/pull/12900#issuecomment-1851928485
FWIW I confirmed that this change makes the new test in #12925 pass.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12900:
URL: https://github.com/apache/lucene/pull/12900#issuecomment-1848936948
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!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423822953
##########
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());
Review Comment:
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?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423547781
##########
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;
Review Comment:
I like this idea to avoid the array copy. Implemented in [e387ec9](https://github.com/apache/lucene/pull/12900/commits/e387ec93e92e5a83505419f88e261bc31fb9ab85).
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423806163
##########
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();
Review Comment:
> 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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on PR #12900:
URL: https://github.com/apache/lucene/pull/12900#issuecomment-1851927851
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12900:
URL: https://github.com/apache/lucene/pull/12900#issuecomment-1850151623
I've confirmed the new (failing) BWC test from #12901 now passes with this PR. I'll review ...
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423493276
##########
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();
Review Comment:
> 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 ?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] IntersectTermsEnum should accumulate from output prefix instead of current output [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on code in PR #12900:
URL: https://github.com/apache/lucene/pull/12900#discussion_r1423505676
##########
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;
Review Comment:
> 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 ?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Push and pop OutputAccumulator as IntersectTermsEnumFrames are pushed and popped [lucene]
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 merged PR #12900:
URL: https://github.com/apache/lucene/pull/12900
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org