You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "gaobinlong (via GitHub)" <gi...@apache.org> on 2026/02/02 07:31:30 UTC
[PR] Introduce DocValuesStatsCollectorManager [lucene]
gaobinlong opened a new pull request, #15653:
URL: https://github.com/apache/lucene/pull/15653
### Description
This pr introduces DocValuesStatsCollectorManager and switches TestDocValuesStatsCollector.java to use search concurrency and move away from the deprecated search(Query, Collector) method.
Relates to https://github.com/apache/lucene/issues/12892.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3198934385
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,43 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
+ count += other.count;
+ missing += other.missing;
+ if (other.min != null && (min == null || compareMin(other.min, min) < 0)) {
+ copyMin(other);
+ }
+ if (other.max != null && (max == null || compareMax(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 compareMin(Object a, Object b) {
+ if (a instanceof Number numA && b instanceof Number numB) {
+ return Double.compare(numA.doubleValue(), numB.doubleValue());
Review Comment:
No, it's not necessary actually, removed it.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
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: 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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3217989120
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -28,8 +28,8 @@
/** Holds statistics for a DocValues field. */
public abstract class DocValuesStats<T> {
- private int missing = 0;
- private int count = 0;
+ int missing = 0;
+ int count = 0;
Review Comment:
Changed them to protected.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3079675830
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,43 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
+ count += other.count;
+ missing += other.missing;
+ if (other.min != null && (min == null || compareMin(other.min, min) < 0)) {
+ copyMin(other);
+ }
+ if (other.max != null && (max == null || compareMax(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 compareMin(Object a, Object b) {
+ if (a instanceof Number numA && b instanceof Number numB) {
+ return Double.compare(numA.doubleValue(), numB.doubleValue());
Review Comment:
I believe this could lead to precision loss? is the special number case necessary?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
+ * @param <S> the type of {@link DocValuesStats}
+ */
+public class DocValuesStatsCollectorManager<S extends DocValuesStats<?>>
+ implements CollectorManager<DocValuesStatsCollector, S> {
+
+ private final Supplier<S> statsSupplier;
+
+ /**
+ * Creates a new DocValuesStatsCollectorManager.
+ *
+ * @param statsSupplier a supplier that creates new stats instances for each collector
+ */
+ public DocValuesStatsCollectorManager(Supplier<S> statsSupplier) {
+ this.statsSupplier = statsSupplier;
+ }
+
+ @Override
+ public DocValuesStatsCollector newCollector() throws IOException {
+ return new DocValuesStatsCollector(statsSupplier.get());
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public S reduce(Collection<DocValuesStatsCollector> collectors) throws IOException {
+ if (collectors.isEmpty()) {
+ return null;
Review Comment:
This is a potential bug that could cause problems down the line? Could we instead return empty stats? Is it even necessary to have this special case? You could instead just remove this conditional?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,43 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
+ count += other.count;
+ missing += other.missing;
+ if (other.min != null && (min == null || compareMin(other.min, min) < 0)) {
+ copyMin(other);
+ }
+ if (other.max != null && (max == null || compareMax(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 compareMin(Object a, Object b) {
+ if (a instanceof Number numA && b instanceof Number numB) {
+ return Double.compare(numA.doubleValue(), numB.doubleValue());
+ }
+ return ((Comparable) a).compareTo(b);
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ int compareMax(Object a, Object b) {
+ if (a instanceof Number numA && b instanceof Number numB) {
+ return Double.compare(numA.doubleValue(), numB.doubleValue());
+ }
+ return ((Comparable) a).compareTo(b);
Review Comment:
This is the same as compareMin ?
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -55,16 +56,21 @@ public class TestDocValuesStatsCollector extends LuceneTestCase {
Review Comment:
Should we also update `testDocsWithSortedValues` and `testDocsWithSortedSetValues`?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
Review Comment:
Could we expand javadocs to include a usage example for users?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -296,6 +433,14 @@ protected void doAccumulate(int count) throws IOException {
public Long sum() {
return sum;
}
+
+ @Override
+ void merge(DocValuesStats<?> other) {
+ super.merge(other);
+ if (other instanceof SortedLongDocValuesStats o) {
Review Comment:
is this type of conditional necessary? Does it ever happen that we get a different type than `SortedLongDocValuesStats` here? This applies to all merge methods.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15653:
URL: https://github.com/apache/lucene/pull/15653#issuecomment-3911193156
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #15653:
URL: https://github.com/apache/lucene/pull/15653#issuecomment-4358738460
Hey @gaobinlong do you have time to address the comments in this PR? I'd be happy to merge this once ready!
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #15653:
URL: https://github.com/apache/lucene/pull/15653
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3214653576
##########
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:
Yeah, you're right, randomIndexWriter flush or commit documents randomly, this is not necessary.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on PR #15653:
URL: https://github.com/apache/lucene/pull/15653#issuecomment-4395410597
Seems the failed tests are not related to this change.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3198936257
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -296,6 +433,14 @@ protected void doAccumulate(int count) throws IOException {
public Long sum() {
return sum;
}
+
+ @Override
+ void merge(DocValuesStats<?> other) {
+ super.merge(other);
+ if (other instanceof SortedLongDocValuesStats o) {
Review Comment:
Removed all of these unnecessary checks.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3217984257
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
+ * <p>Example usage:
+ *
+ * <pre class="prettyprint">
+ * IndexSearcher searcher = ...; // your IndexSearcher
+ * DocValuesStatsCollectorManager<DocValuesStats.LongDocValuesStats> manager =
+ * new DocValuesStatsCollectorManager<>(() -> new DocValuesStats.LongDocValuesStats("price"));
+ * DocValuesStats.LongDocValuesStats stats = searcher.search(new MatchAllDocsQuery(), manager);
Review Comment:
Changed it to include the `T` type.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15653:
URL: https://github.com/apache/lucene/pull/15653#issuecomment-4340126129
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3198928537
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -73,6 +73,43 @@ final void addMissing() {
++missing;
}
+ void merge(DocValuesStats<?> other) {
+ count += other.count;
+ missing += other.missing;
+ if (other.min != null && (min == null || compareMin(other.min, min) < 0)) {
+ copyMin(other);
+ }
+ if (other.max != null && (max == null || compareMax(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 compareMin(Object a, Object b) {
+ if (a instanceof Number numA && b instanceof Number numB) {
+ return Double.compare(numA.doubleValue(), numB.doubleValue());
+ }
+ return ((Comparable) a).compareTo(b);
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ int compareMax(Object a, Object b) {
+ if (a instanceof Number numA && b instanceof Number numB) {
+ return Double.compare(numA.doubleValue(), numB.doubleValue());
+ }
+ return ((Comparable) a).compareTo(b);
Review Comment:
Combined them into one method.
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
Review Comment:
Add one, thanks!
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -55,16 +56,21 @@ public class TestDocValuesStatsCollector extends LuceneTestCase {
Review Comment:
Updated both.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3198933188
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
+ * @param <S> the type of {@link DocValuesStats}
+ */
+public class DocValuesStatsCollectorManager<S extends DocValuesStats<?>>
+ implements CollectorManager<DocValuesStatsCollector, S> {
+
+ private final Supplier<S> statsSupplier;
+
+ /**
+ * Creates a new DocValuesStatsCollectorManager.
+ *
+ * @param statsSupplier a supplier that creates new stats instances for each collector
+ */
+ public DocValuesStatsCollectorManager(Supplier<S> statsSupplier) {
+ this.statsSupplier = statsSupplier;
+ }
+
+ @Override
+ public DocValuesStatsCollector newCollector() throws IOException {
+ return new DocValuesStatsCollector(statsSupplier.get());
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public S reduce(Collection<DocValuesStatsCollector> collectors) throws IOException {
+ if (collectors.isEmpty()) {
+ return null;
Review Comment:
It's not necessary, removed it, thanks!
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3217274892
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
+ * <p>Example usage:
+ *
+ * <pre class="prettyprint">
+ * IndexSearcher searcher = ...; // your IndexSearcher
+ * DocValuesStatsCollectorManager<DocValuesStats.LongDocValuesStats> manager =
+ * new DocValuesStatsCollectorManager<>(() -> new DocValuesStats.LongDocValuesStats("price"));
+ * DocValuesStats.LongDocValuesStats stats = searcher.search(new MatchAllDocsQuery(), manager);
+ * </pre>
+ *
+ * @param <S> the type of {@link DocValuesStats}
Review Comment:
can you also add javadocs for `T`?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollector.java:
##########
@@ -68,4 +68,9 @@ public void collect(int doc) throws IOException {
public ScoreMode scoreMode() {
return ScoreMode.COMPLETE_NO_SCORES;
}
+
+ /** Returns the statistics computed by this collector. */
+ public DocValuesStats<?> getStats() {
Review Comment:
does this need to be public? Perhaps this could be package private given it's only needed by the collector manager that lives in the same package?
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -99,226 +104,243 @@ 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();
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), dir, config)) {
+ 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()) {
- 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;
}
+ 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));
+ // 20% of cases delete some docs
+ 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();
Review Comment:
this seems a leftover, could we remove it and rely on the default index writer config in all these tests?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStats.java:
##########
@@ -28,8 +28,8 @@
/** Holds statistics for a DocValues field. */
public abstract class DocValuesStats<T> {
- private int missing = 0;
- private int count = 0;
+ int missing = 0;
+ int count = 0;
Review Comment:
I find that using package private here is not consistent with the rest of the members that we just made protected for potential subclasses within other packages to use. I would rather make these also protected, or use the available getter, setter etc methods but I think in this case it is not possible.
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -99,226 +104,243 @@ 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();
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), dir, config)) {
+ 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()) {
- 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;
}
+ indexWriter.addDocument(doc);
}
- }
- try (DirectoryReader reader = DirectoryReader.open(indexWriter)) {
- IndexSearcher searcher = new IndexSearcher(reader);
Review Comment:
isn't using `newSearcher` the only change we should be making to all these tests?
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
+ * <p>Example usage:
+ *
+ * <pre class="prettyprint">
+ * IndexSearcher searcher = ...; // your IndexSearcher
+ * DocValuesStatsCollectorManager<DocValuesStats.LongDocValuesStats> manager =
+ * new DocValuesStatsCollectorManager<>(() -> new DocValuesStats.LongDocValuesStats("price"));
+ * DocValuesStats.LongDocValuesStats stats = searcher.search(new MatchAllDocsQuery(), manager);
Review Comment:
I believe this usage example won't compile.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3218029365
##########
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java:
##########
@@ -99,226 +104,243 @@ 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();
+ try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), dir, config)) {
+ 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()) {
- 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;
}
+ indexWriter.addDocument(doc);
}
- }
- try (DirectoryReader reader = DirectoryReader.open(indexWriter)) {
- IndexSearcher searcher = new IndexSearcher(reader);
Review Comment:
I think no, we also need RandomIndexWriter to generate more segments.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on PR #15653:
URL: https://github.com/apache/lucene/pull/15653#issuecomment-4363500320
> Hey @gaobinlong do you have time to address the comments in this PR? I'd be happy to merge this once ready!
@javanna , sorry for the delay, took some time to address the comments but not fully address them, will get it ready next week.
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3214651413
##########
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:
Added it back.
##########
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:
Changed that, thanks!
##########
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:
This is also unnecessary, removed it.
##########
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:
Yeah, you're right, randomIndexWriter flush documents randomly, this is not necessary.
##########
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:
That's better, changed it to `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) {
+ 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:
Both copyMin and copyMax are folded into the merge method.
##########
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:
Simplify this by folding the compareValues() method into the caller method merge().
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3217976663
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollectorManager.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.misc.search;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A {@link CollectorManager} implementation for {@link DocValuesStatsCollector}.
+ *
+ * <p>Example usage:
+ *
+ * <pre class="prettyprint">
+ * IndexSearcher searcher = ...; // your IndexSearcher
+ * DocValuesStatsCollectorManager<DocValuesStats.LongDocValuesStats> manager =
+ * new DocValuesStatsCollectorManager<>(() -> new DocValuesStats.LongDocValuesStats("price"));
+ * DocValuesStats.LongDocValuesStats stats = searcher.search(new MatchAllDocsQuery(), manager);
+ * </pre>
+ *
+ * @param <S> the type of {@link DocValuesStats}
Review Comment:
Added it, thanks!
--
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] Introduce DocValuesStatsCollectorManager [lucene]
Posted by "gaobinlong (via GitHub)" <gi...@apache.org>.
gaobinlong commented on code in PR #15653:
URL: https://github.com/apache/lucene/pull/15653#discussion_r3217986718
##########
lucene/misc/src/java/org/apache/lucene/misc/search/DocValuesStatsCollector.java:
##########
@@ -68,4 +68,9 @@ public void collect(int doc) throws IOException {
public ScoreMode scoreMode() {
return ScoreMode.COMPLETE_NO_SCORES;
}
+
+ /** Returns the statistics computed by this collector. */
+ public DocValuesStats<?> getStats() {
Review Comment:
Changed it to package private.
--
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