You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "stefanvodita (via GitHub)" <gi...@apache.org> on 2024/01/22 15:04:26 UTC
[PR] Fix TaxonomyIndexArray construction from empty index [lucene]
stefanvodita opened a new pull request, #13028:
URL: https://github.com/apache/lucene/pull/13028
In #12995, we introduced `allocateChunkedArray`, but had a misaligned assumption in the constructor for cases where the allocation size is 0. @msfroh - sorry I missed this check when we refactored `allocateChunkedArray`.
Credit to @Shradha26 for discovering this while applying #12995 to the Amazon fork.
--
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] Fix issues with chunked TaxonomyIndexArray [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on code in PR #13028:
URL: https://github.com/apache/lucene/pull/13028#discussion_r1463869892
##########
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/directory/TestTaxonomyIndexArrays.java:
##########
@@ -59,4 +74,88 @@ public void testMultiplesOfChunkSize() {
ordinal = newOrdinal;
}
}
+
+ public void testConstructFromEmptyIndex() throws IOException {
+ Directory dir = newDirectory();
+
+ // Produce empty index
+ new IndexWriter(dir, newIndexWriterConfig(null)).close();
+
+ IndexReader reader = DirectoryReader.open(dir);
+
+ TaxonomyIndexArrays tia = new TaxonomyIndexArrays(reader);
+ assertEquals(0, tia.parents().length());
+
+ tia = new TaxonomyIndexArrays(reader, tia);
+ assertEquals(0, tia.parents().length());
+
+ reader.close();
+ dir.close();
+ }
+
+ public void testRefresh() throws IOException {
+ Directory dir = newDirectory();
+
+ // Write two chunks worth of ordinals whose parents are ordinals 1 or 2
+ TaxonomyWriter taxoWriter = new DirectoryTaxonomyWriter(dir);
+ taxoWriter.addCategory(new FacetLabel("a")); // ordinal 1
+ taxoWriter.addCategory(new FacetLabel("b")); // ordinal 2
+ for (int i = 0; i < 2 * TaxonomyIndexArrays.CHUNK_SIZE; i++) {
Review Comment:
If you start this with `i = 3`, you should end up with exactly two `CHUNK_SIZE` worth of ordinals.
I believe you would hit the condition that I called out above where `initParents` won't get called.
##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/TaxonomyIndexArrays.java:
##########
@@ -80,7 +81,8 @@ public int length() {
public TaxonomyIndexArrays(IndexReader reader) throws IOException {
int[][] parentArray = allocateChunkedArray(reader.maxDoc(), 0);
- if (parentArray.length > 0) {
+ assert parentArray.length > 0;
+ if (parentArray[parentArray.length - 1].length > 0) {
Review Comment:
Does this condition work when we grow to a multiple of `CHUNK_SIZE`?
Let's say the total size was 8190, then we add two more ordinals. Now we have a chunk of size 8192, followed by "tail chunk" of size 0. With this condition, I think we would fail to read the two new elements from `reader`.
I think you might be able to verify it with `testRefresh`, where you should find that the `parents` arrays are always:
```
parents[0][0] == INVALID
parents[0][1] == 0
parents[0][2] == 0
parents[0][3] == 1
parents[0][4] == 2
parents[0][5] == 1
parents[0][6] == 2
...
parents[0][n] == 1 (if n%2 == 1)
parents[0][n] == 2 (if n%2 == 0)
```
--
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] Fix TaxonomyIndexArray construction from empty index [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on PR #13028:
URL: https://github.com/apache/lucene/pull/13028#issuecomment-1904540802
Hey -- I was just telling a colleague about this change and he asked about the synchronization on `initChildrenSiblings`. I talked through the logic in `computeChildrenSiblings` and realized that we very likely have another bug:
```
for (int i = first; i < length; i++) {
int parent = parents.get(i);
// The existing youngest child of the parent is the next older sibling of i.
// note that parents[i] is always < i, so the right-hand-side of
// the following line is already set when we get here
siblings.set(i, children.get(parent));
// The new youngest child of the parent is i.
children.set(parent, i);
}
```
Since `first` is the index of the first new element, and `parent < i` always, we can guarantee that at least one `parent` is an "old" ordinal, likely shared with the old `TaxonomyIndexArray` (unless the parent is also in the last chunk). If we add a new top-level category, then `parent` will be `0`, which is always going to be from one of our old, supposedly "immutable" chunks.
I think we need to deep-copy any `children` chunks for parents of the newly-added 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.
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] Fix issues with chunked TaxonomyIndexArray [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on code in PR #13028:
URL: https://github.com/apache/lucene/pull/13028#discussion_r1463878454
##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/TaxonomyIndexArrays.java:
##########
@@ -95,7 +97,8 @@ public TaxonomyIndexArrays(IndexReader reader, TaxonomyIndexArrays copyFrom) thr
// NRT reader was obtained, even though nothing was changed. this is not very likely
// to happen.
int[][] parentArray = allocateChunkedArray(reader.maxDoc(), copyFrom.parents.values.length - 1);
- if (parentArray.length > 0) {
+ assert parentArray.length > 0;
+ if (parentArray[parentArray.length - 1].length > 0) {
Review Comment:
Actually, I guess my concern above was about the case where we start with a multiple of `CHUNK_SIZE`. This one kicks in when we grow to a multiple of `CHUNK_SIZE`.
Either way, I think the condition is going to hit us when we have that trailing empty array.
Could the condition be `if (parentArray[0].length > 0)`, since we have the assert to say that `parentArray` always has at least one element (which could be empty)?
--
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] Fix TaxonomyIndexArray construction from empty index [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #13028:
URL: https://github.com/apache/lucene/pull/13028#discussion_r1462177725
##########
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/directory/TestTaxonomyIndexArrays.java:
##########
@@ -59,4 +64,22 @@ public void testMultiplesOfChunkSize() {
ordinal = newOrdinal;
}
}
+
+ public void testConstructFromEmptyIndex() throws IOException {
+ Directory dir = newDirectory();
+
+ // Produce empty index
+ new IndexWriter(dir, newIndexWriterConfig(null)).close();
+
+ IndexReader reader = DirectoryReader.open(dir);
+
+ TaxonomyIndexArrays tia = new TaxonomyIndexArrays(reader);
+ assertEquals(0, tia.parents().length());
Review Comment:
I don't think it is a valid use-case, but I think there is something to tweak with the checks in the constructors. The previous conditions were trying to check for an empty array, but with the way `allocateChunkedArray` turned out in the end, that was no longer the correct condition to check for.
--
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] Fix issues with chunked TaxonomyIndexArray [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #13028:
URL: https://github.com/apache/lucene/pull/13028#discussion_r1464835176
##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/TaxonomyIndexArrays.java:
##########
@@ -95,7 +97,8 @@ public TaxonomyIndexArrays(IndexReader reader, TaxonomyIndexArrays copyFrom) thr
// NRT reader was obtained, even though nothing was changed. this is not very likely
// to happen.
int[][] parentArray = allocateChunkedArray(reader.maxDoc(), copyFrom.parents.values.length - 1);
- if (parentArray.length > 0) {
+ assert parentArray.length > 0;
+ if (parentArray[parentArray.length - 1].length > 0) {
Review Comment:
In this constructor we can't use `parentArray[0].length > 0` because `parentArray[0]` could be `null` if we had already filled up the first chunk, but I actually think we don't need this if statement at all - what are we guarding against?
##########
lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/TaxonomyIndexArrays.java:
##########
@@ -80,7 +81,8 @@ public int length() {
public TaxonomyIndexArrays(IndexReader reader) throws IOException {
int[][] parentArray = allocateChunkedArray(reader.maxDoc(), 0);
- if (parentArray.length > 0) {
+ assert parentArray.length > 0;
+ if (parentArray[parentArray.length - 1].length > 0) {
Review Comment:
You're right that this condition is not correct. I'll revert back to `parentArray[0].length > 0`.
--
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] Fix TaxonomyIndexArray construction from empty index [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on code in PR #13028:
URL: https://github.com/apache/lucene/pull/13028#discussion_r1462133413
##########
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/directory/TestTaxonomyIndexArrays.java:
##########
@@ -59,4 +64,22 @@ public void testMultiplesOfChunkSize() {
ordinal = newOrdinal;
}
}
+
+ public void testConstructFromEmptyIndex() throws IOException {
+ Directory dir = newDirectory();
+
+ // Produce empty index
+ new IndexWriter(dir, newIndexWriterConfig(null)).close();
+
+ IndexReader reader = DirectoryReader.open(dir);
+
+ TaxonomyIndexArrays tia = new TaxonomyIndexArrays(reader);
+ assertEquals(0, tia.parents().length());
Review Comment:
Oh -- interesting. I didn't realize this was a valid use-case.
If you open a TaxonomyWriter, you're currently guaranteed to get at least one (root) document.
--
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] Fix TaxonomyIndexArray construction from empty index [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13028:
URL: https://github.com/apache/lucene/pull/13028#issuecomment-1904887153
Thank you for finding this @msfroh! I pushed a commit that should fix the issue.
--
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] Fix issues with chunked TaxonomyIndexArray [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita merged PR #13028:
URL: https://github.com/apache/lucene/pull/13028
--
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