You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/11/29 15:57:58 UTC
[GitHub] [lucene] iverase opened a new pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
iverase opened a new pull request #486:
URL: https://github.com/apache/lucene/pull/486
Introduces two functional interfaces, `DocValuesVisitor` and `DocIdsVisitor` that are used in the PointTree API instead of using the IntersectVisitor. The IntersectVisitor is now extending those interfaces.
--
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
[GitHub] [lucene] jpountz commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762885177
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
I haven't thought much about it but moving to a long might make sense.
--
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
[GitHub] [lucene] jpountz commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761233070
##########
File path: lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
##########
@@ -505,80 +504,29 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
checkAndThrow();
- in.visitDocIDs(visitor);
+ in.visitDocIDs(
+ docID -> {
+ checkAndThrowWithSampling();
Review comment:
Can we find a better way and not check on every doc/value pair but once per leaf node or something like that?
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
is this cast safe? If yes, can you leave a comment about it and use `Math.toIntExact`?
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
+ pointTree.visitDocValues(visitor::compare, visitor, visitor);
}
break;
default:
throw new IllegalArgumentException("Unreachable code");
}
}
+ private void visitDocIds(IntersectVisitor visitor, PointTree pointTree) throws IOException {
+ final long size = pointTree.size();
+ if (size <= Integer.MAX_VALUE) {
+ visitor.grow((int) size);
+ pointTree.visitDocIDs(visitor::visit);
+ } else {
+ if (pointTree.moveToChild()) {
+ do {
+ visitDocIds(visitor, pointTree);
Review comment:
nit: I wonder if we should try to make it iterative instead of recursive so that it would also work with (weird) implementations of PointValues that could have a high depth
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -323,10 +355,18 @@ default void grow(int count) {}
*/
public final void intersect(IntersectVisitor visitor) throws IOException {
final PointTree pointTree = getPointTree();
- intersect(visitor, pointTree);
+ intersect(wrapIntersectVisitor(visitor), pointTree);
assert pointTree.moveToParent() == false;
}
+ /**
+ * Adds the possibility of wrapping a provided {@link IntersectVisitor} in {@link
+ * #intersect(IntersectVisitor)}.
+ */
+ protected IntersectVisitor wrapIntersectVisitor(IntersectVisitor visitor) throws IOException {
+ return visitor;
+ }
Review comment:
Actually I have a preference for not making intersect final, which has the benefit of not increasing the API surface area (protected methods show up in javadocs).
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -269,22 +270,33 @@ protected PointValues() {}
long size();
/** Visit all the docs below the current node. */
- void visitDocIDs(IntersectVisitor visitor) throws IOException;
+ void visitDocIDs(DocIdsVisitor docIdsVisitor) throws IOException;
/** Visit all the docs and values below the current node. */
- void visitDocValues(IntersectVisitor visitor) throws IOException;
+ default void visitDocValues(DocValuesVisitor docValuesVisitor) throws IOException {
+ visitDocValues((min, max) -> Relation.CELL_CROSSES_QUERY, docID -> {}, docValuesVisitor);
+ }
+
+ /**
+ * Similar to {@link #visitDocValues(DocValuesVisitor)} but in this case it allows adding a
+ * filter that works like {@link IntersectVisitor#compare(byte[], byte[])}.
+ */
+ void visitDocValues(
+ BiFunction<byte[], byte[], Relation> compare,
Review comment:
I'm considuring adding a new functional interface instead of relying on a java.util function. This would help put javadocs about the contract that we expect from this comparison function.
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761838139
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90PointsWriter.java
##########
@@ -147,24 +145,7 @@ public void writeField(FieldInfo fieldInfo, PointsReader reader) throws IOExcept
return;
}
- values.visitDocValues(
- new IntersectVisitor() {
- @Override
- public void visit(int docID) {
- throw new IllegalStateException();
- }
-
- @Override
- public void visit(int docID, byte[] packedValue) throws IOException {
- writer.add(packedValue, docID);
- }
-
- @Override
- public Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
- return Relation.CELL_CROSSES_QUERY;
- }
- });
-
+ values.visitDocValues((docID, packedValue) -> writer.add(packedValue, docID));
Review comment:
sweet, I reversed out and using a method reference is much nicer.
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761251202
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -269,22 +270,33 @@ protected PointValues() {}
long size();
/** Visit all the docs below the current node. */
- void visitDocIDs(IntersectVisitor visitor) throws IOException;
+ void visitDocIDs(DocIdsVisitor docIdsVisitor) throws IOException;
/** Visit all the docs and values below the current node. */
- void visitDocValues(IntersectVisitor visitor) throws IOException;
+ default void visitDocValues(DocValuesVisitor docValuesVisitor) throws IOException {
+ visitDocValues((min, max) -> Relation.CELL_CROSSES_QUERY, docID -> {}, docValuesVisitor);
+ }
+
+ /**
+ * Similar to {@link #visitDocValues(DocValuesVisitor)} but in this case it allows adding a
+ * filter that works like {@link IntersectVisitor#compare(byte[], byte[])}.
+ */
+ void visitDocValues(
+ BiFunction<byte[], byte[], Relation> compare,
Review comment:
I like it as I was considering the same so +1, I will add 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
[GitHub] [lucene] jpountz commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761731533
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90PointsWriter.java
##########
@@ -147,24 +145,7 @@ public void writeField(FieldInfo fieldInfo, PointsReader reader) throws IOExcept
return;
}
- values.visitDocValues(
- new IntersectVisitor() {
- @Override
- public void visit(int docID) {
- throw new IllegalStateException();
- }
-
- @Override
- public void visit(int docID, byte[] packedValue) throws IOException {
- writer.add(packedValue, docID);
- }
-
- @Override
- public Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
- return Relation.CELL_CROSSES_QUERY;
- }
- });
-
+ values.visitDocValues((docID, packedValue) -> writer.add(packedValue, docID));
Review comment:
too bad the order is reversed and we cannot use a method reference, maybe we should change the order of parameters in BKDWriter?
##########
File path: lucene/core/src/java/org/apache/lucene/index/SortingCodecReader.java
##########
@@ -170,45 +168,20 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
- sortingIntersectVisitor.setIntersectVisitor(visitor);
- indexTree.visitDocIDs(sortingIntersectVisitor);
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
+ indexTree.visitDocIDs(docMap::oldToNew);
}
@Override
- public void visitDocValues(PointValues.IntersectVisitor visitor) throws IOException {
- sortingIntersectVisitor.setIntersectVisitor(visitor);
- indexTree.visitDocValues(sortingIntersectVisitor);
- }
- }
-
- private static class SortingIntersectVisitor implements PointValues.IntersectVisitor {
-
- private final Sorter.DocMap docMap;
-
- private PointValues.IntersectVisitor visitor;
-
- SortingIntersectVisitor(Sorter.DocMap docMap) {
- this.docMap = docMap;
- }
-
- private void setIntersectVisitor(PointValues.IntersectVisitor visitor) {
- this.visitor = visitor;
- }
-
- @Override
- public void visit(int docID) throws IOException {
- visitor.visit(docMap.oldToNew(docID));
- }
-
- @Override
- public void visit(int docID, byte[] packedValue) throws IOException {
- visitor.visit(docMap.oldToNew(docID), packedValue);
- }
-
- @Override
- public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
- return visitor.compare(minPackedValue, maxPackedValue);
+ public void visitDocValues(
+ PointValues.NodeComparator nodeComparator,
+ PointValues.DocIdsVisitor docIdsVisitor,
+ PointValues.DocValuesVisitor docValuesVisitor)
+ throws IOException {
+ indexTree.visitDocValues(
+ nodeComparator,
+ docMap::oldToNew,
Review comment:
and likewise here?
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
:+1:
##########
File path: lucene/core/src/java/org/apache/lucene/index/SortingCodecReader.java
##########
@@ -170,45 +168,20 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
- sortingIntersectVisitor.setIntersectVisitor(visitor);
- indexTree.visitDocIDs(sortingIntersectVisitor);
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
+ indexTree.visitDocIDs(docMap::oldToNew);
Review comment:
This doesn't look right: for every doc ID we pass it to `oldToNew` but then don't do anything with the result? We should pass the result to docIdsVisitor?
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
+ pointTree.visitDocValues(visitor::compare, visitor, visitor);
}
break;
default:
throw new IllegalArgumentException("Unreachable code");
}
}
+ private void visitDocIds(IntersectVisitor visitor, PointTree pointTree) throws IOException {
+ final long size = pointTree.size();
+ if (size <= Integer.MAX_VALUE) {
+ visitor.grow((int) size);
+ pointTree.visitDocIDs(visitor::visit);
+ } else {
+ if (pointTree.moveToChild()) {
+ do {
+ visitDocIds(visitor, pointTree);
Review comment:
ok
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -269,22 +269,59 @@ protected PointValues() {}
long size();
/** Visit all the docs below the current node. */
- void visitDocIDs(IntersectVisitor visitor) throws IOException;
+ void visitDocIDs(DocIdsVisitor docIdsVisitor) throws IOException;
/** Visit all the docs and values below the current node. */
- void visitDocValues(IntersectVisitor visitor) throws IOException;
+ default void visitDocValues(DocValuesVisitor docValuesVisitor) throws IOException {
+ visitDocValues((min, max) -> Relation.CELL_CROSSES_QUERY, docID -> {}, docValuesVisitor);
Review comment:
should the doc id visitor throw an assertion error instead?
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761252070
##########
File path: lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
##########
@@ -505,80 +504,29 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
checkAndThrow();
- in.visitDocIDs(visitor);
+ in.visitDocIDs(
+ docID -> {
+ checkAndThrowWithSampling();
Review comment:
yes, we should take a closer look here
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762755043
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
Maybe even better would be something like:
```
visitor.grow((int) Math.min(getDocCount(), pointTree.size()))
```
Which agrees with grow() contract as it refers to number of documents not number of points:
```
/** Notifies the caller that this many documents are about to be visited */
```
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761995196
##########
File path: lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
##########
@@ -505,80 +504,29 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
checkAndThrow();
- in.visitDocIDs(visitor);
+ in.visitDocIDs(
+ docID -> {
+ checkAndThrowWithSampling();
Review comment:
I changed the implementation so we actually only do checks every `DEFAULT_MAX_POINTS_IN_LEAF_NODE`.
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762043648
##########
File path: lucene/core/src/java/org/apache/lucene/util/bkd/BKDReader.java
##########
@@ -495,57 +495,63 @@ private int balanceTreeNodePosition(
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
+ public void visitDocIDs(DocIdsVisitor docIDVisitor) throws IOException {
resetNodeDataPosition();
- addAll(visitor, false);
+ addAll(docIDVisitor);
}
- public void addAll(PointValues.IntersectVisitor visitor, boolean grown) throws IOException {
- if (grown == false) {
- final long size = size();
- if (size <= Integer.MAX_VALUE) {
- visitor.grow((int) size);
- grown = true;
- }
- }
+ public void addAll(DocIdsVisitor docIdsVisitor) throws IOException {
if (isLeafNode()) {
// Leaf node
leafNodes.seek(getLeafBlockFP());
// How many points are stored in this leaf cell:
int count = leafNodes.readVInt();
- // No need to call grow(), it has been called up-front
- DocIdsWriter.readInts(leafNodes, count, visitor);
+ // No need to call grow(), it has been called docIdVisitor-front
Review comment:
Actually I remove it all together. the BKDReader knows nothing about grow() any longer
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762936377
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
I open https://github.com/apache/lucene/pull/520. If that makes sense (and I think it does)then it should make sense to change grow() to a long.
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r758504244
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -323,10 +355,18 @@ default void grow(int count) {}
*/
public final void intersect(IntersectVisitor visitor) throws IOException {
final PointTree pointTree = getPointTree();
- intersect(visitor, pointTree);
+ intersect(wrapIntersectVisitor(visitor), pointTree);
assert pointTree.moveToParent() == false;
}
+ /**
+ * Adds the possibility of wrapping a provided {@link IntersectVisitor} in {@link
+ * #intersect(IntersectVisitor)}.
+ */
+ protected IntersectVisitor wrapIntersectVisitor(IntersectVisitor visitor) throws IOException {
+ return visitor;
+ }
Review comment:
This added this entry point in order to wrap IntersectVisitor with an AssertingIntersectVisitor during testing. I don't really like it but the only other option is to make intersects method not final which I didn't like it either.
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761244824
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
In theory we are on a leaf and the maximum number of points should be given by `maxPointsInLeafNode` which is an int, therefore it should be safe? wdyt?
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761250414
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
+ pointTree.visitDocValues(visitor::compare, visitor, visitor);
}
break;
default:
throw new IllegalArgumentException("Unreachable code");
}
}
+ private void visitDocIds(IntersectVisitor visitor, PointTree pointTree) throws IOException {
+ final long size = pointTree.size();
+ if (size <= Integer.MAX_VALUE) {
+ visitor.grow((int) size);
+ pointTree.visitDocIDs(visitor::visit);
+ } else {
+ if (pointTree.moveToChild()) {
+ do {
+ visitDocIds(visitor, pointTree);
Review comment:
Not sure if that would fix that. This recursion will potentially have the same depth as the one when we visit an intersecting leaf (via #visitDocValues), so it won't be fixing issues for high depth trees?
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762861748
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
Thanks for the explanation, I see that we currently start adding any new docID (duplicated or not) into a int[] until a threshold were we upgrade to a BitSet.
I wonder why grow does not take a long but an int if we can call `visit(int docID)` more than `Integer.MAX_VALUE` times?. Ii is just weird the dance we do here to handle big values when in our implementation for such big values we would have probably already upgraded to a BitSet.
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761839189
##########
File path: lucene/core/src/java/org/apache/lucene/index/SortingCodecReader.java
##########
@@ -170,45 +168,20 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
- sortingIntersectVisitor.setIntersectVisitor(visitor);
- indexTree.visitDocIDs(sortingIntersectVisitor);
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
+ indexTree.visitDocIDs(docMap::oldToNew);
Review comment:
good catch, I fixed 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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761839041
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -269,22 +269,59 @@ protected PointValues() {}
long size();
/** Visit all the docs below the current node. */
- void visitDocIDs(IntersectVisitor visitor) throws IOException;
+ void visitDocIDs(DocIdsVisitor docIdsVisitor) throws IOException;
/** Visit all the docs and values below the current node. */
- void visitDocValues(IntersectVisitor visitor) throws IOException;
+ default void visitDocValues(DocValuesVisitor docValuesVisitor) throws IOException {
+ visitDocValues((min, max) -> Relation.CELL_CROSSES_QUERY, docID -> {}, docValuesVisitor);
Review comment:
👍
--
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
[GitHub] [lucene] romseygeek commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
romseygeek commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761874923
##########
File path: lucene/core/src/java/org/apache/lucene/util/bkd/BKDReader.java
##########
@@ -495,57 +495,63 @@ private int balanceTreeNodePosition(
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
+ public void visitDocIDs(DocIdsVisitor docIDVisitor) throws IOException {
resetNodeDataPosition();
- addAll(visitor, false);
+ addAll(docIDVisitor);
}
- public void addAll(PointValues.IntersectVisitor visitor, boolean grown) throws IOException {
- if (grown == false) {
- final long size = size();
- if (size <= Integer.MAX_VALUE) {
- visitor.grow((int) size);
- grown = true;
- }
- }
+ public void addAll(DocIdsVisitor docIdsVisitor) throws IOException {
if (isLeafNode()) {
// Leaf node
leafNodes.seek(getLeafBlockFP());
// How many points are stored in this leaf cell:
int count = leafNodes.readVInt();
- // No need to call grow(), it has been called up-front
- DocIdsWriter.readInts(leafNodes, count, visitor);
+ // No need to call grow(), it has been called docIdVisitor-front
Review comment:
```suggestion
// No need to call grow(), it has been called by docIdVisitor
```
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -297,42 +339,48 @@ default void visit(DocIdSetIterator iterator) throws IOException {
visit(docID);
}
}
+ }
- /**
- * Called for all documents in a leaf cell that crosses the query. The consumer should
- * scrutinize the packedValue to decide whether to accept it. In the 1D case, values are visited
- * in increasing order, and in the case of ties, in increasing docID order.
- */
+ /**
+ * Collects all documents and values below a tree node by calling {@link
+ * PointTree#visitDocValues(DocValuesVisitor)} (DocIdsVisitor)}
Review comment:
```suggestion
* PointTree#visitDocValues(DocValuesVisitor)}
```
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762043750
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -297,42 +339,48 @@ default void visit(DocIdSetIterator iterator) throws IOException {
visit(docID);
}
}
+ }
- /**
- * Called for all documents in a leaf cell that crosses the query. The consumer should
- * scrutinize the packedValue to decide whether to accept it. In the 1D case, values are visited
- * in increasing order, and in the case of ties, in increasing docID order.
- */
+ /**
+ * Collects all documents and values below a tree node by calling {@link
+ * PointTree#visitDocValues(DocValuesVisitor)} (DocIdsVisitor)}
Review comment:
done
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761251552
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -323,10 +355,18 @@ default void grow(int count) {}
*/
public final void intersect(IntersectVisitor visitor) throws IOException {
final PointTree pointTree = getPointTree();
- intersect(visitor, pointTree);
+ intersect(wrapIntersectVisitor(visitor), pointTree);
assert pointTree.moveToParent() == false;
}
+ /**
+ * Adds the possibility of wrapping a provided {@link IntersectVisitor} in {@link
+ * #intersect(IntersectVisitor)}.
+ */
+ protected IntersectVisitor wrapIntersectVisitor(IntersectVisitor visitor) throws IOException {
+ return visitor;
+ }
Review comment:
agreed I will work on that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762055745
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
@jpountz Is there any reason why we are not using the following construct when calling grow:
```
visitor.grow((int) Math.min(Integer.MAX_VALUE, pointTree.size()));
```
That should simplify a bit and probably is safer?
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
@jpountz Is there any reason why we are not using the following construct when calling grow:
```
visitor.grow((int) Math.min(Integer.MAX_VALUE, pointTree.size()));
```
That should simplify things a bit and probably is safer?
--
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
[GitHub] [lucene] sonatype-lift[bot] commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761358324
##########
File path: lucene/test-framework/src/java/org/apache/lucene/index/BasePointsFormatTestCase.java
##########
@@ -1070,29 +1071,13 @@ private void assertSize(PointValues.PointTree tree) throws IOException {
tree = rarely() ? clone : tree;
final long[] visitDocIDSize = new long[] {0};
final long[] visitDocValuesSize = new long[] {0};
- final IntersectVisitor visitor =
- new IntersectVisitor() {
- @Override
- public void visit(int docID) {
- visitDocIDSize[0]++;
- }
- @Override
- public void visit(int docID, byte[] packedValue) {
- visitDocValuesSize[0]++;
- }
-
- @Override
- public Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
- return Relation.CELL_CROSSES_QUERY;
- }
- };
if (random().nextBoolean()) {
- tree.visitDocIDs(visitor);
- tree.visitDocValues(visitor);
+ tree.visitDocIDs((docID -> visitDocIDSize[0]++));
+ tree.visitDocValues((docID, packedValue) -> visitDocValuesSize[0]++);
} else {
- tree.visitDocValues(visitor);
- tree.visitDocIDs(visitor);
+ tree.visitDocValues((docID, packedValue) -> visitDocValuesSize[0]++);
+ tree.visitDocIDs((docID -> visitDocIDSize[0]++));
Review comment:
*UnnecessaryParentheses:* Unnecessary use of grouping parentheses [(details)](https://errorprone.info/bugpattern/UnnecessaryParentheses)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [lucene] iverase commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r761839280
##########
File path: lucene/core/src/java/org/apache/lucene/index/SortingCodecReader.java
##########
@@ -170,45 +168,20 @@ public long size() {
}
@Override
- public void visitDocIDs(PointValues.IntersectVisitor visitor) throws IOException {
- sortingIntersectVisitor.setIntersectVisitor(visitor);
- indexTree.visitDocIDs(sortingIntersectVisitor);
+ public void visitDocIDs(PointValues.DocIdsVisitor docIdsVisitor) throws IOException {
+ indexTree.visitDocIDs(docMap::oldToNew);
}
@Override
- public void visitDocValues(PointValues.IntersectVisitor visitor) throws IOException {
- sortingIntersectVisitor.setIntersectVisitor(visitor);
- indexTree.visitDocValues(sortingIntersectVisitor);
- }
- }
-
- private static class SortingIntersectVisitor implements PointValues.IntersectVisitor {
-
- private final Sorter.DocMap docMap;
-
- private PointValues.IntersectVisitor visitor;
-
- SortingIntersectVisitor(Sorter.DocMap docMap) {
- this.docMap = docMap;
- }
-
- private void setIntersectVisitor(PointValues.IntersectVisitor visitor) {
- this.visitor = visitor;
- }
-
- @Override
- public void visit(int docID) throws IOException {
- visitor.visit(docMap.oldToNew(docID));
- }
-
- @Override
- public void visit(int docID, byte[] packedValue) throws IOException {
- visitor.visit(docMap.oldToNew(docID), packedValue);
- }
-
- @Override
- public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
- return visitor.compare(minPackedValue, maxPackedValue);
+ public void visitDocValues(
+ PointValues.NodeComparator nodeComparator,
+ PointValues.DocIdsVisitor docIdsVisitor,
+ PointValues.DocValuesVisitor docValuesVisitor)
+ throws IOException {
+ indexTree.visitDocValues(
+ nodeComparator,
+ docMap::oldToNew,
Review comment:
likewise here
--
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
[GitHub] [lucene] jpountz commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762833932
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
The contract that we really care about for `grow()` is the number of times `visit(int docID)` might be called since we use it to resize the `int[]` array that stores matching doc IDs. Making `grow` about the number of unique documents would make it more challenging to deal with the `int[]` in case a leaf has more doc/value pairs than unique docs, since we wouldn't be able to safely grow the array up-front and would have to check upon every doc.
--
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
[GitHub] [lucene] jpountz commented on a change in pull request #486: LUCENE-9619: Remove IntersectVisitor from PointsTree API
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #486:
URL: https://github.com/apache/lucene/pull/486#discussion_r762884878
##########
File path: lucene/core/src/java/org/apache/lucene/index/PointValues.java
##########
@@ -361,14 +405,29 @@ private void intersect(IntersectVisitor visitor, PointTree pointTree) throws IOE
// TODO: we can assert that the first value here in fact matches what the pointTree
// claimed?
// Leaf node; scan and filter all points in this block:
- pointTree.visitDocValues(visitor);
+ visitor.grow((int) pointTree.size());
Review comment:
I suspect we made it take an `int` because we thought of using it for growing arrays which are addressed by integers, and thought that it would be good enough to only grow at the leaf level, where we would always have a small number of doc/value pairs.
--
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