mikemccand commented on a change in pull request #2247:
URL: https://github.com/apache/lucene-solr/pull/2247#discussion_r564505813
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -314,7 +316,6 @@ public int getOrdinal(FacetLabel cp) throws IOException {
@Override
public FacetLabel getPath(int ordinal) throws IOException {
- ensureOpen();
Review comment:
Hmm why is this removed?
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
+ FacetLabel[] bulkPath = new FacetLabel[ordinal.length];
+ Map<Integer, Integer> originalPosition = new HashMap<>();
+ for (int i = 0; i < ordinal.length; i++) {
+ if (ordinal[i] < 0 || ordinal[i] >= indexReader.maxDoc()) {
+ return null;
Review comment:
Hmm can we instead throw exception w/ meaningful message?
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
+ FacetLabel[] bulkPath = new FacetLabel[ordinal.length];
+ Map<Integer, Integer> originalPosition = new HashMap<>();
+ for (int i = 0; i < ordinal.length; i++) {
+ if (ordinal[i] < 0 || ordinal[i] >= indexReader.maxDoc()) {
+ return null;
+ }
+ FacetLabel ordinalPath = getPathFromCache(ordinal[i]);
+ if (ordinalPath != null) {
+ bulkPath[i] = ordinalPath;
+ }
+ originalPosition.put(ordinal[i], i);
Review comment:
Instead of a `Map<Integer,Integer>` you could make two parallel arrays,
and sort both arrays in a "matched" manner. Lucene's sort APIs in `oal.util`
make this simple, and there are example in Lucene's sources of "sorting
parallel arrays".
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
+ FacetLabel[] bulkPath = new FacetLabel[ordinal.length];
+ Map<Integer, Integer> originalPosition = new HashMap<>();
+ for (int i = 0; i < ordinal.length; i++) {
+ if (ordinal[i] < 0 || ordinal[i] >= indexReader.maxDoc()) {
+ return null;
+ }
+ FacetLabel ordinalPath = getPathFromCache(ordinal[i]);
+ if (ordinalPath != null) {
+ bulkPath[i] = ordinalPath;
+ }
+ originalPosition.put(ordinal[i], i);
+ }
+
+ Arrays.sort(ordinal);
+ int readerIndex = 0;
+ BinaryDocValues values = null;
+
+ for (int ord : ordinal) {
+ if (bulkPath[originalPosition.get(ord)] == null) {
+ if (values == null
+ || values.advanceExact(ord -
indexReader.leaves().get(readerIndex).docBase) == false) {
+ readerIndex = ReaderUtil.subIndex(ord, indexReader.leaves());
+ LeafReader leafReader =
indexReader.leaves().get(readerIndex).reader();
+ values = leafReader.getBinaryDocValues(Consts.FULL);
+ assert values.advanceExact(ord -
indexReader.leaves().get(readerIndex).docBase);
Review comment:
Hmm, this is dangerous: we are requiring that this `.advanceExact` be
called, since we below use `.binaryValue()`? But if user disables assertions,
this won't happen, and then `.binaryValue()` might be wrong or throw an
exception?
I think you should do it in two steps, e.g.:
```
boolean success = values.advanceExact(ord -
indexReader.leaves().get(readerIndex).docBase);
assert success;
```
?
Also, why didn't the unit test uncover this failure? Lucene's
`test-framework` intentionally, randomly some percentage of the time, disables
assertions precisely because of this situation :) Long ago a Lucene release
had a bug that accidentally "relied" on assertions being enabled in order to
run correctly.
Edit: OK, I see -- the unit test only creates a single segment index, so
this line is not exercised I think (yet).
##########
File path:
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/directory/TestDirectoryTaxonomyReader.java
##########
@@ -569,4 +569,31 @@ public void testAccountable() throws Exception {
taxoReader.close();
dir.close();
}
+
+ public void testBulkPath() throws Exception {
+ Directory src = newDirectory();
+ DirectoryTaxonomyWriter w = new DirectoryTaxonomyWriter(src);
+ String arr[] = new String[] {"a", "b", "c", "d", "e"};
+
+ FacetLabel allpaths[] = new FacetLabel[arr.length];
+ int allords[] = new int[arr.length];
+
+ for (int i = 0; i < arr.length; i++) {
+ allpaths[i] = new FacetLabel(arr[i]);
+ w.addCategory(allpaths[i]);
+ }
+ w.commit();
+ w.close();
+
+ DirectoryTaxonomyReader r1 = new DirectoryTaxonomyReader(src);
+
+ for (int i = 0; i < allpaths.length; i++) {
+ allords[i] = r1.getOrdinal(allpaths[i]);
+ }
+
+ FacetLabel allBulkpath[] = r1.getBulkPath(allords);
+ assertArrayEquals(allpaths, allBulkpath);
Review comment:
This is a good initial basic test case, thanks!
Could you also add a randomized test, indexing random number of facet
labels, hopefully exercising multiple segments randomly, and then doing bulk
lookup of random collections of ordinals in random order? It could help
uncover exciting bugs :)
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
+ FacetLabel[] bulkPath = new FacetLabel[ordinal.length];
+ Map<Integer, Integer> originalPosition = new HashMap<>();
+ for (int i = 0; i < ordinal.length; i++) {
+ if (ordinal[i] < 0 || ordinal[i] >= indexReader.maxDoc()) {
+ return null;
+ }
+ FacetLabel ordinalPath = getPathFromCache(ordinal[i]);
+ if (ordinalPath != null) {
+ bulkPath[i] = ordinalPath;
+ }
+ originalPosition.put(ordinal[i], i);
+ }
+
+ Arrays.sort(ordinal);
+ int readerIndex = 0;
+ BinaryDocValues values = null;
+
+ for (int ord : ordinal) {
+ if (bulkPath[originalPosition.get(ord)] == null) {
Review comment:
Hmm, why wouldn't this entry be `null`? Shouldn't it always be `null`?
I.e. each requested ordinal goes to a unique position?
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
Review comment:
Whoa, why is that!? Oh, this is because it must be `BinaryDocValues`
taxonomy index, OK. If the user messes up and tries to use this API on stored
fields, what happens?
Also, can we enhance the javadoc to state what the method does, link to the
non-bulk API, etc.?
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
+ FacetLabel[] bulkPath = new FacetLabel[ordinal.length];
+ Map<Integer, Integer> originalPosition = new HashMap<>();
+ for (int i = 0; i < ordinal.length; i++) {
+ if (ordinal[i] < 0 || ordinal[i] >= indexReader.maxDoc()) {
+ return null;
+ }
+ FacetLabel ordinalPath = getPathFromCache(ordinal[i]);
+ if (ordinalPath != null) {
+ bulkPath[i] = ordinalPath;
+ }
+ originalPosition.put(ordinal[i], i);
+ }
+
+ Arrays.sort(ordinal);
+ int readerIndex = 0;
+ BinaryDocValues values = null;
+
+ for (int ord : ordinal) {
+ if (bulkPath[originalPosition.get(ord)] == null) {
+ if (values == null
+ || values.advanceExact(ord -
indexReader.leaves().get(readerIndex).docBase) == false) {
Review comment:
Hmm, instead of relying on `advanceExact` returning `false` to know when
to advance to the next reader, could we instead use `.maxDoc` of the reader we
are on?
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
+ FacetLabel[] bulkPath = new FacetLabel[ordinal.length];
+ Map<Integer, Integer> originalPosition = new HashMap<>();
Review comment:
Maybe use `IntIntScatterMap` from HPPC instead, to create less garbage
... `facet` module already depends on HPPC.
But this optimization can come later, and is likely negligible for most
search use cases anyways.
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java
##########
@@ -353,12 +349,65 @@ public FacetLabel getPath(int ordinal) throws IOException
{
}
synchronized (categoryCache) {
- categoryCache.put(catIDInteger, ret);
+ categoryCache.put(ordinal, ret);
}
return ret;
}
+ private FacetLabel getPathFromCache(int ordinal) {
+ ensureOpen();
+
+ // TODO: can we use an int-based hash impl, such as IntToObjectMap,
+ // wrapped as LRU?
+ synchronized (categoryCache) {
+ FacetLabel res = categoryCache.get(ordinal);
+ if (res != null) {
+ return res;
+ }
+ }
+ return null;
+ }
+
+ /* This API is only supported for indexes created with Lucene 8.7+ codec **/
+ public FacetLabel[] getBulkPath(int[] ordinal) throws IOException {
Review comment:
Maybe rename the parameter to `ordinals`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
