jpountz commented on code in PR #15161:
URL: https://github.com/apache/lucene/pull/15161#discussion_r2333265563
##########
lucene/core/src/java/org/apache/lucene/util/BitSet.java:
##########
@@ -24,7 +24,8 @@
*
* @lucene.internal
*/
-public abstract class BitSet implements Bits, Accountable {
+public abstract sealed class BitSet implements Bits, Accountable
Review Comment:
Can you add a small comment explaining that we're doing this to help call
sites of methods on the `BitSet` class be bimorphic at most, to help with
performance?
##########
lucene/CHANGES.txt:
##########
@@ -215,6 +215,8 @@ Improvements
* GITHUB#15063: Use optimistic-with-checking KNN Query strategy for
`DiversifyingChildren*` queries.
(Ben Trent)
+* GITHUB#15038: BitSet is now a sealed abstract class permitting only
FixedBitSet and SparseFixedBitSet.
+
Review Comment:
Let's move this to 11.0?
##########
lucene/test-framework/src/java/org/apache/lucene/tests/util/BaseBitSetTestCase.java:
##########
@@ -65,6 +61,8 @@ static java.util.BitSet randomSet(int numBits, float
percentSet) {
return randomSet(numBits, (int) (percentSet * numBits));
}
+ protected abstract T fromJavaUtilBitSet(java.util.BitSet set, int numBits);
Review Comment:
I worry that copying from a `java.util.BitSet` defeats the purpose of the
test, since most tests would be comparing two same implementations of `BitSet`
instead of `JavaUtilBitSet` vs. a `BitSet` implementation. Can we try to
refactor tests to work directly against a `java.util.BitSet` without copying to
a `BitSet`?
##########
lucene/test-framework/src/java/org/apache/lucene/tests/util/BaseBitSetTestCase.java:
##########
@@ -75,7 +73,7 @@ protected void assertEquals(BitSet set1, T set2, int maxDoc) {
public void testCardinality() throws IOException {
final int numBits = 1 + random().nextInt(100000);
for (float percentSet : new float[] {0, 0.01f, 0.1f, 0.5f, 0.9f, 0.99f,
1f}) {
- BitSet set1 = new JavaUtilBitSet(randomSet(numBits, percentSet),
numBits);
+ BitSet set1 = fromJavaUtilBitSet(randomSet(numBits, percentSet),
numBits);
Review Comment:
E.g. doing just `java.util.BitSet set1 = randomSet(numBits, percentSet);`
should be good enough here since we're only comparing cardinalities?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
