You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "mikemccand (via GitHub)" <gi...@apache.org> on 2023/12/10 11:24:02 UTC
[I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
mikemccand opened a new issue, #12901:
URL: https://github.com/apache/lucene/issues/12901
### Description
Spinoff from #12895 where we inadvertently introduced read-time exceptions in `MultiTermQueries` (e.g. `WildcardQuery` `*fo*`) in 9.9.0 when reading pre-9.9.0 written indices.
Our `TestBackwardsCompatibility` test should have caught this but clearly did not.
Let's:
1. Make a test revealing #12895
2. Confirm the test (and all other tests) pass when applying the proposed PR fix #12900
3. Merge the new test case
4. Merget he fix in #12900
Hopefully in that order?
We could simply snapshot the `wikibigall` index into BWC zip files, but, that's way too massive. Can we instead make a randomized test that indexes fewer-ish terms and reveals the bug? With "just so" set of terms, the bug should reproduce with a tiny index. We could maybe iterate to "just so" by tweaking the set of terms' prefixes until the on-disk file pointers of each block share the last 6 bits of their respective pointers?
### Gradle command to reproduce
_No response_
--
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.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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1849968380
> We should look into more systematically forking the code when we do a file format change, or figuring out other ways to keep testing prior formats with version bumps, e.g. by retaining version checks on the write side and making sure there is a separate `BasePostingsFormatTestCase` impl for every version number?
+1 -- normally forking/duplicating code is a bad thing, but for BWC testing, and for code clarity and our sanity keeping track of code changes that correspond to our on-disk format changes, it is the lesser evil (to fork).
And I think #12908 is a case-in-point showing how risky it is to try to add the right `if` statements on the write side -- the `precommit` failures there are subtle and I think maybe due to a missing `if` statement somewhere in the code changes...
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand closed issue #12901: Improve BWC tests to reveal #12895 and confirm its fix
URL: https://github.com/apache/lucene/issues/12901
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1850041347
Here's the one-off scaffolding I used to find a smallish repro case.
`WriteRandomIndex.java` just opens a pre-existing file with one-term-per-line extracted from a `wikibigall` index. It compiles/runs against 9.8.0 clone.
`TickleBlockTreeBug.java` runs against 9.9.0 clone and opens the randomly created index and then runs random two-ascii-character `WildcardQuery` for N iterations.
`drive.py` is just a `while` loop on these two tools, iterating until a seed tickles the bug.
[12895-repro-tools.zip](https://github.com/apache/lucene/files/13635702/12895-repro-tools.zip)
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1849481891
> A benefit of the latter approach is that we keep existing tests in place, specifically BasePostingsFormatTestCase
Actually, it seems like we lost the test as part of refactorings, I'm adding it back in #12904.
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1849971078
Agreed, I didn't want to go the forking route for `FST`, which I'm not too familiar with, but ideally we'd version it like other file formats: FST90, FST99, etc.
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1849981059
I have a smallish failing test case here! (~49 KB .zip file)
I created it by first extracting all ~5.8 M unique terms from a `wikibigall` index, creating a tool that randomly picks a sequential slice of length [1_000..100_000) of those terms, indexing that, then a read-time test that tries to run random two-letter ascii `WildcardQuery`. It ran multiple iterations until it tickled the bug. I'll post this test-case-discovery-scaffolding code here in case we need to do such things to find future exotic test cases ... and I'll work up a PR to add this "special" index to our BWC tests and confirm it's failing in 9.9.x.
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1849073645
IMO one thing to improve here is that we changed file formats for the MSB vlong optimization by bumping the file format version number, changing the writer to write the new version and adding version checks to the reader. But we sometimes also change file formats by forking the code, like we did with `Lucene90PostingsFormat` -> `Lucene99PostingsFormat`. A benefit of the latter approach is that we keep existing tests in place, specifically `BasePostingsFormatTestCase` which has pretty good coverage, probably more than what `TestBackwardsCompatibility` will ever be able to check? We should look into more systematically forking the code when we do a file format change, or figuring out other ways to keep testing prior formats with version bumps, e.g. by retaining version checks on the write side and making sure there is a separate `BasePostingsFormatTestCase` impl for every version number?
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1850135758
> Actually I can un-@ignore at least in main. I'll go do that.
D'oh! No, I cannot -- it will still fail in main, 9.x and 9.9.x until we get the fix (#12900) in.
--
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: [I] Improve BWC tests to reveal #12895 and confirm its fix [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on issue #12901:
URL: https://github.com/apache/lucene/issues/12901#issuecomment-1850129073
OK this is done -- I pushed the new BWC test case (@Ignore'd) to 9.9.x, 9.x and main.
Actually I can un-@Ignore at least in main. I'll go do that.
--
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