javanna commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3201221900
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -99,228 +107,257 @@ public void testOneDoc() throws IOException {
}
public void testDocsWithLongValues() throws IOException {
- try (Directory dir = newDirectory();
- IndexWriter indexWriter = new IndexWriter(dir,
newIndexWriterConfig())) {
- String field = "numeric";
- int numDocs = TestUtil.nextInt(random(), 1, 100);
- long[] docValues = new long[numDocs];
- int nextVal = 1;
- for (int i = 0; i < numDocs; i++) {
- Document doc = new Document();
- if (random().nextBoolean()) { // not all documents have a value
- doc.add(new NumericDocValuesField(field, nextVal));
- doc.add(new StringField("id", "doc" + i, Store.NO));
- docValues[i] = nextVal;
- ++nextVal;
- }
- indexWriter.addDocument(doc);
- }
-
- // 20% of cases delete some docs
- if (random().nextDouble() < 0.2) {
Review Comment:
leave this comment?
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -99,228 +107,257 @@ public void testOneDoc() throws IOException {
}
public void testDocsWithLongValues() throws IOException {
- try (Directory dir = newDirectory();
- IndexWriter indexWriter = new IndexWriter(dir,
newIndexWriterConfig())) {
- String field = "numeric";
- int numDocs = TestUtil.nextInt(random(), 1, 100);
- long[] docValues = new long[numDocs];
- int nextVal = 1;
- for (int i = 0; i < numDocs; i++) {
- Document doc = new Document();
- if (random().nextBoolean()) { // not all documents have a value
- doc.add(new NumericDocValuesField(field, nextVal));
- doc.add(new StringField("id", "doc" + i, Store.NO));
- docValues[i] = nextVal;
- ++nextVal;
- }
- indexWriter.addDocument(doc);
- }
-
- // 20% of cases delete some docs
- if (random().nextDouble() < 0.2) {
+ try (Directory dir = newDirectory()) {
+ IndexWriterConfig config = newIndexWriterConfig();
+ config.setMaxBufferedDocs(10);
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(),
dir, config)) {
+ String field = "numeric";
+ int numDocs = TestUtil.nextInt(random(), 50, 100);
+ long[] docValues = new long[numDocs];
+ int nextVal = 1;
for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
if (random().nextBoolean()) {
- indexWriter.deleteDocuments(new Term("id", "doc" + i));
- docValues[i] = 0;
+ doc.add(new NumericDocValuesField(field, nextVal));
+ doc.add(new StringField("id", "doc" + i, Store.NO));
+ docValues[i] = nextVal;
+ ++nextVal;
+ }
+ if (i != 0 && i % 10 == 0) {
+ indexWriter.commit();
Review Comment:
this is new? Out of curiosity, why do we need it? I was under the impression
that using random index writer may provide enough random behaviours.
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -142,6 +168,45 @@ public final double stdev() {
* might overflow.
*/
public abstract T sum();
+
+ @SuppressWarnings("unchecked")
Review Comment:
I feel like the generics are a bit off. We could leverage them to prevent
from merging mixed docvalues stats types, which is anyways unsupported given we
do the casting within each merge?
Should the signature be `merge(DocValuesStats<T>)` ?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,32 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
Review Comment:
nit: perhaps this should be protected. This class is under misc, so unlikely
that there exist subclasses in the wild, but if there are, they should be able
to override merge with their own implementation? Perhaps add some javadocs as
well?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,32 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
+ count += other.count;
+ missing += other.missing;
+ if (other.min != null && (min == null || compareValues(other.min, min) <
0)) {
+ copyMin(other);
+ }
+ if (other.max != null && (max == null || compareValues(other.max, max) >
0)) {
+ copyMax(other);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ void copyMin(DocValuesStats<?> other) {
+ min = (T) other.min;
+ }
+
+ @SuppressWarnings("unchecked")
+ void copyMax(DocValuesStats<?> other) {
Review Comment:
Does this potentially need to be overridden? I wonder the same for
`copyMin`. Also, given they are one-liner called from a single place , could
they be folded into their caller method?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -173,6 +238,12 @@ protected void doAccumulate(int count) throws IOException {
public Long sum() {
return sum;
}
+
+ @Override
+ void merge(DocValuesStats<?> other) {
+ super.merge(other);
+ sum += ((LongDocValuesStats) other).sum;
Review Comment:
If we go for adding T to the signature, could we instead cast to
`NumericDocValuesStats<Long>`, but most importantly, please call the `sum()`
method instead?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -335,6 +461,12 @@ protected void doAccumulate(int count) throws IOException {
public Double sum() {
return sum;
}
+
+ @Override
+ void merge(DocValuesStats<?> other) {
+ super.merge(other);
+ this.sum += ((SortedDoubleDocValuesStats) other).sum;
Review Comment:
`(SortedNumericDocValuesStats<Long>` ? call the `sum `method instead?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -205,6 +276,12 @@ protected void doAccumulate(int count) throws IOException {
public Double sum() {
return sum;
}
+
+ @Override
+ void merge(DocValuesStats<?> other) {
+ super.merge(other);
+ this.sum += ((DoubleDocValuesStats) other).sum;
Review Comment:
cast to `NumericDocValuesStats<Double>` instead and call the `sum()` method?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -296,6 +416,12 @@ protected void doAccumulate(int count) throws IOException {
public Long sum() {
return sum;
}
+
+ @Override
+ void merge(DocValuesStats<?> other) {
+ super.merge(other);
+ sum += ((SortedLongDocValuesStats) other).sum();
Review Comment:
`SortedNumericDocValuesStats<Long>` ?
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -99,228 +107,257 @@ public void testOneDoc() throws IOException {
}
public void testDocsWithLongValues() throws IOException {
- try (Directory dir = newDirectory();
- IndexWriter indexWriter = new IndexWriter(dir,
newIndexWriterConfig())) {
- String field = "numeric";
- int numDocs = TestUtil.nextInt(random(), 1, 100);
- long[] docValues = new long[numDocs];
- int nextVal = 1;
- for (int i = 0; i < numDocs; i++) {
- Document doc = new Document();
- if (random().nextBoolean()) { // not all documents have a value
- doc.add(new NumericDocValuesField(field, nextVal));
- doc.add(new StringField("id", "doc" + i, Store.NO));
- docValues[i] = nextVal;
- ++nextVal;
- }
- indexWriter.addDocument(doc);
- }
-
- // 20% of cases delete some docs
- if (random().nextDouble() < 0.2) {
+ try (Directory dir = newDirectory()) {
+ IndexWriterConfig config = newIndexWriterConfig();
+ config.setMaxBufferedDocs(10);
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(),
dir, config)) {
+ String field = "numeric";
+ int numDocs = TestUtil.nextInt(random(), 50, 100);
+ long[] docValues = new long[numDocs];
+ int nextVal = 1;
for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
if (random().nextBoolean()) {
- indexWriter.deleteDocuments(new Term("id", "doc" + i));
- docValues[i] = 0;
+ doc.add(new NumericDocValuesField(field, nextVal));
+ doc.add(new StringField("id", "doc" + i, Store.NO));
+ docValues[i] = nextVal;
+ ++nextVal;
+ }
+ if (i != 0 && i % 10 == 0) {
+ indexWriter.commit();
}
+ indexWriter.addDocument(doc);
}
- }
- try (DirectoryReader reader = DirectoryReader.open(indexWriter)) {
- IndexSearcher searcher = new IndexSearcher(reader);
- LongDocValuesStats stats = new LongDocValuesStats(field);
- searcher.search(MatchAllDocsQuery.INSTANCE, new
DocValuesStatsCollector(stats));
+ if (random().nextDouble() < 0.2) {
+ for (int i = 0; i < numDocs; i++) {
+ if (random().nextBoolean()) {
+ indexWriter.deleteDocuments(new Term("id", "doc" + i));
+ docValues[i] = 0;
+ }
+ }
+ }
- int expCount = (int) Arrays.stream(docValues).filter(v -> v >
0).count();
- assertEquals(expCount, stats.count());
- int numDocsWithoutField = (int) getZeroValues(docValues).count();
- assertEquals(computeExpMissing(numDocsWithoutField, numDocs, reader),
stats.missing());
- if (stats.count() > 0) {
- LongSummaryStatistics sumStats =
getPositiveValues(docValues).summaryStatistics();
- assertEquals(sumStats.getMax(), stats.max().longValue());
- assertEquals(sumStats.getMin(), stats.min().longValue());
- assertEquals(sumStats.getAverage(), stats.mean(), 0.00001);
- assertEquals(sumStats.getSum(), stats.sum().longValue());
- double variance = computeVariance(docValues, stats.mean,
stats.count());
- assertEquals(variance, stats.variance(), 0.00001);
- assertEquals(Math.sqrt(variance), stats.stdev(), 0.00001);
+ try (DirectoryReader reader = indexWriter.getReader()) {
+ IndexSearcher searcher = newSearcher(reader);
+ LongDocValuesStats stats =
+ searcher.search(
+ MatchAllDocsQuery.INSTANCE,
+ new DocValuesStatsCollectorManager<>(() -> new
LongDocValuesStats(field)));
+
+ int expCount = (int) Arrays.stream(docValues).filter(v -> v >
0).count();
+ assertEquals(expCount, stats.count());
+ int numDocsWithoutField = (int) getZeroValues(docValues).count();
+ assertEquals(computeExpMissing(numDocsWithoutField, numDocs,
reader), stats.missing());
+ if (stats.count() > 0) {
+ LongSummaryStatistics sumStats =
getPositiveValues(docValues).summaryStatistics();
+ assertEquals(sumStats.getMax(), stats.max().longValue());
+ assertEquals(sumStats.getMin(), stats.min().longValue());
+ assertEquals(sumStats.getAverage(), stats.mean(), 0.00001);
+ assertEquals(sumStats.getSum(), stats.sum().longValue());
+ double variance = computeVariance(docValues, stats.mean,
stats.count());
+ assertEquals(variance, stats.variance(), 0.00001);
+ assertEquals(Math.sqrt(variance), stats.stdev(), 0.00001);
+ }
}
}
}
}
public void testDocsWithDoubleValues() throws IOException {
- try (Directory dir = newDirectory();
- IndexWriter indexWriter = new IndexWriter(dir,
newIndexWriterConfig())) {
- String field = "numeric";
- int numDocs = TestUtil.nextInt(random(), 1, 100);
- double[] docValues = new double[numDocs];
- double nextVal = 1.0;
- for (int i = 0; i < numDocs; i++) {
- Document doc = new Document();
- if (random().nextBoolean()) { // not all documents have a value
- doc.add(new DoubleDocValuesField(field, nextVal));
- doc.add(new StringField("id", "doc" + i, Store.NO));
- docValues[i] = nextVal;
- ++nextVal;
- }
- indexWriter.addDocument(doc);
- }
-
- // 20% of cases delete some docs
- if (random().nextDouble() < 0.2) {
+ try (Directory dir = newDirectory()) {
+ IndexWriterConfig config = newIndexWriterConfig();
+ config.setMaxBufferedDocs(10);
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(),
dir, config)) {
+ String field = "numeric";
+ int numDocs = TestUtil.nextInt(random(), 50, 100);
+ double[] docValues = new double[numDocs];
+ double nextVal = 1.0;
for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
if (random().nextBoolean()) {
- indexWriter.deleteDocuments(new Term("id", "doc" + i));
- docValues[i] = 0;
+ doc.add(new DoubleDocValuesField(field, nextVal));
+ doc.add(new StringField("id", "doc" + i, Store.NO));
+ docValues[i] = nextVal;
+ ++nextVal;
+ }
+ if (i != 0 && i % 10 == 0) {
+ indexWriter.commit();
}
+ indexWriter.addDocument(doc);
}
- }
- try (DirectoryReader reader = DirectoryReader.open(indexWriter)) {
- IndexSearcher searcher = new IndexSearcher(reader);
- DoubleDocValuesStats stats = new DoubleDocValuesStats(field);
- searcher.search(MatchAllDocsQuery.INSTANCE, new
DocValuesStatsCollector(stats));
+ if (random().nextDouble() < 0.2) {
+ for (int i = 0; i < numDocs; i++) {
+ if (random().nextBoolean()) {
+ indexWriter.deleteDocuments(new Term("id", "doc" + i));
+ docValues[i] = 0;
+ }
+ }
+ }
- int expCount = (int) Arrays.stream(docValues).filter(v -> v >
0).count();
- assertEquals(expCount, stats.count());
- int numDocsWithoutField = (int) getZeroValues(docValues).count();
- assertEquals(computeExpMissing(numDocsWithoutField, numDocs, reader),
stats.missing());
- if (stats.count() > 0) {
- DoubleSummaryStatistics sumStats =
getPositiveValues(docValues).summaryStatistics();
- assertEquals(sumStats.getMax(), stats.max().doubleValue(), 0.00001);
- assertEquals(sumStats.getMin(), stats.min().doubleValue(), 0.00001);
- assertEquals(sumStats.getAverage(), stats.mean(), 0.00001);
- assertEquals(sumStats.getSum(), stats.sum(), 0.00001);
- double variance = computeVariance(docValues, stats.mean,
stats.count());
- assertEquals(variance, stats.variance(), 0.00001);
- assertEquals(Math.sqrt(variance), stats.stdev(), 0.00001);
+ try (DirectoryReader reader = indexWriter.getReader()) {
+ IndexSearcher searcher = newSearcher(reader);
+ DoubleDocValuesStats stats =
+ searcher.search(
+ MatchAllDocsQuery.INSTANCE,
+ new DocValuesStatsCollectorManager<>(() -> new
DoubleDocValuesStats(field)));
+
+ int expCount = (int) Arrays.stream(docValues).filter(v -> v >
0).count();
+ assertEquals(expCount, stats.count());
+ int numDocsWithoutField = (int) getZeroValues(docValues).count();
+ assertEquals(computeExpMissing(numDocsWithoutField, numDocs,
reader), stats.missing());
+ if (stats.count() > 0) {
+ DoubleSummaryStatistics sumStats =
getPositiveValues(docValues).summaryStatistics();
+ assertEquals(sumStats.getMax(), stats.max().doubleValue(),
0.00001);
+ assertEquals(sumStats.getMin(), stats.min().doubleValue(),
0.00001);
+ assertEquals(sumStats.getAverage(), stats.mean(), 0.00001);
+ assertEquals(sumStats.getSum(), stats.sum(), 0.00001);
+ double variance = computeVariance(docValues, stats.mean,
stats.count());
+ assertEquals(variance, stats.variance(), 0.00001);
+ assertEquals(Math.sqrt(variance), stats.stdev(), 0.00001);
+ }
}
}
}
}
public void testDocsWithMultipleLongValues() throws IOException {
- try (Directory dir = newDirectory();
- IndexWriter indexWriter = new IndexWriter(dir,
newIndexWriterConfig())) {
- String field = "numeric";
- int numDocs = TestUtil.nextInt(random(), 1, 100);
- long[][] docValues = new long[numDocs][];
- long nextVal = 1;
- for (int i = 0; i < numDocs; i++) {
- Document doc = new Document();
- if (random().nextBoolean()) { // not all documents have a value
- int numValues = TestUtil.nextInt(random(), 1, 5);
- docValues[i] = new long[numValues];
- for (int j = 0; j < numValues; j++) {
- doc.add(new SortedNumericDocValuesField(field, nextVal));
- docValues[i][j] = nextVal;
- ++nextVal;
- }
- doc.add(new StringField("id", "doc" + i, Store.NO));
- }
- indexWriter.addDocument(doc);
- }
-
- // 20% of cases delete some docs
- if (random().nextDouble() < 0.2) {
+ try (Directory dir = newDirectory()) {
+ IndexWriterConfig config = newIndexWriterConfig();
+ config.setMaxBufferedDocs(10);
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(),
dir, config)) {
+ String field = "numeric";
+ int numDocs = TestUtil.nextInt(random(), 50, 100);
+ long[][] docValues = new long[numDocs][];
+ long nextVal = 1;
for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
if (random().nextBoolean()) {
- indexWriter.deleteDocuments(new Term("id", "doc" + i));
- docValues[i] = null;
+ int numValues = TestUtil.nextInt(random(), 1, 5);
+ docValues[i] = new long[numValues];
+ for (int j = 0; j < numValues; j++) {
+ doc.add(new SortedNumericDocValuesField(field, nextVal));
+ docValues[i][j] = nextVal;
+ ++nextVal;
+ }
+ doc.add(new StringField("id", "doc" + i, Store.NO));
}
+ if (i != 0 && i % 10 == 0) {
+ indexWriter.commit();
+ }
+ indexWriter.addDocument(doc);
}
- }
- try (DirectoryReader reader = DirectoryReader.open(indexWriter)) {
- IndexSearcher searcher = new IndexSearcher(reader);
- SortedLongDocValuesStats stats = new SortedLongDocValuesStats(field);
- searcher.search(MatchAllDocsQuery.INSTANCE, new
DocValuesStatsCollector(stats));
+ if (random().nextDouble() < 0.2) {
+ for (int i = 0; i < numDocs; i++) {
+ if (random().nextBoolean()) {
+ indexWriter.deleteDocuments(new Term("id", "doc" + i));
+ docValues[i] = null;
+ }
+ }
+ }
- assertEquals(nonNull(docValues).count(), stats.count());
- int numDocsWithoutField = (int) isNull(docValues).count();
- assertEquals(computeExpMissing(numDocsWithoutField, numDocs, reader),
stats.missing());
- if (stats.count() > 0) {
- LongSummaryStatistics sumStats =
- filterAndFlatValues(docValues, (v) -> v !=
null).summaryStatistics();
- assertEquals(sumStats.getMax(), stats.max().longValue());
- assertEquals(sumStats.getMin(), stats.min().longValue());
- assertEquals(sumStats.getAverage(), stats.mean(), 0.00001);
- assertEquals(sumStats.getSum(), stats.sum().longValue());
- assertEquals(sumStats.getCount(), stats.valuesCount());
- double variance =
- computeVariance(
- filterAndFlatValues(docValues, (v) -> v != null),
stats.mean, stats.count());
- assertEquals(variance, stats.variance(), 0.00001);
- assertEquals(Math.sqrt(variance), stats.stdev(), 0.00001);
+ try (DirectoryReader reader = indexWriter.getReader()) {
+ IndexSearcher searcher = newSearcher(reader);
+ SortedLongDocValuesStats stats =
+ searcher.search(
+ MatchAllDocsQuery.INSTANCE,
+ new DocValuesStatsCollectorManager<>(() -> new
SortedLongDocValuesStats(field)));
+
+ assertEquals(nonNull(docValues).count(), stats.count());
+ int numDocsWithoutField = (int) isNull(docValues).count();
+ assertEquals(computeExpMissing(numDocsWithoutField, numDocs,
reader), stats.missing());
+ if (stats.count() > 0) {
+ LongSummaryStatistics sumStats =
+ filterAndFlatValues(docValues, (v) -> v !=
null).summaryStatistics();
+ assertEquals(sumStats.getMax(), stats.max().longValue());
+ assertEquals(sumStats.getMin(), stats.min().longValue());
+ assertEquals(sumStats.getAverage(), stats.mean(), 0.00001);
+ assertEquals(sumStats.getSum(), stats.sum().longValue());
+ assertEquals(sumStats.getCount(), stats.valuesCount());
+ double variance =
+ computeVariance(
+ filterAndFlatValues(docValues, (v) -> v != null),
stats.mean, stats.count());
+ assertEquals(variance, stats.variance(), 0.00001);
+ assertEquals(Math.sqrt(variance), stats.stdev(), 0.00001);
+ }
}
}
}
}
public void testDocsWithMultipleDoubleValues() throws IOException {
- try (Directory dir = newDirectory();
- IndexWriter indexWriter = new IndexWriter(dir,
newIndexWriterConfig())) {
- String field = "numeric";
- int numDocs = TestUtil.nextInt(random(), 1, 100);
- double[][] docValues = new double[numDocs][];
- double nextVal = 1;
- for (int i = 0; i < numDocs; i++) {
- Document doc = new Document();
- if (random().nextBoolean()) { // not all documents have a value
- int numValues = TestUtil.nextInt(random(), 1, 5);
- docValues[i] = new double[numValues];
- for (int j = 0; j < numValues; j++) {
- doc.add(new SortedNumericDocValuesField(field,
Double.doubleToRawLongBits(nextVal)));
- docValues[i][j] = nextVal;
- ++nextVal;
- }
- doc.add(new StringField("id", "doc" + i, Store.NO));
- }
- indexWriter.addDocument(doc);
- }
-
- // 20% of cases delete some docs
- if (random().nextDouble() < 0.2) {
+ try (Directory dir = newDirectory()) {
+ IndexWriterConfig config = newIndexWriterConfig();
+ config.setMaxBufferedDocs(10);
Review Comment:
why is this called now? We did not call it before?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,32 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
+ count += other.count;
+ missing += other.missing;
+ if (other.min != null && (min == null || compareValues(other.min, min) <
0)) {
+ copyMin(other);
+ }
+ if (other.max != null && (max == null || compareValues(other.max, max) >
0)) {
+ copyMax(other);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ void copyMin(DocValuesStats<?> other) {
+ min = (T) other.min;
+ }
+
+ @SuppressWarnings("unchecked")
+ void copyMax(DocValuesStats<?> other) {
+ max = (T) other.max;
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ int compareValues(Object a, Object b) {
Review Comment:
I wonder if this should be `protected`, in case there are subclasses in the
wild. They should be able to call the method as needed?
Also, do we need to accept objects as arguments and cast? Perhaps we could
leverage generics, and have both T arguments, yet keep the comparable inside
the method.
--
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]
