Add TaxonomyReader#getBulkOrdinals method (#12180) by epotyom · Pull Request #12769 · apache/lucene

epotyom · 2023-11-06T11:15:28Z

Add TaxonomyReader#getBulkOrdinals method (#12180)

This is the first step for #12180 , next step will be to implement Facets#getSpecificValues (bulk) that calls getBulkOrdinals, will do it in a separate PR.

mikemccand

This looks great @epotyom - I left some small comments.

I wonder what performance impact this might have for heavy Lucene facet use cases... but we don't need to block this nice API with specific benchmarking. It can come later. Maybe we could (later) fix Lucene's nightly benchmark in luceneutil for faceting tasks to use this new API... maybe open an issue there?

mikemccand · 2023-11-06T12:16:35Z

+  private void assertGettingPaths(
+      DirectoryTaxonomyReader reader, FacetLabel[] expectedPaths, int[] sourceOrds)
+      throws IOException {
+    // To exercise mix of cache hit and cache misses for getPath and getBulkPath this method:


mikemccand · 2023-11-06T12:18:12Z

    src.close();
  }

+  private void assertGettingOrdinals(


Could we maybe add a thread safety test as well, if we aren't already testing across threads? Index many FacetLabel -> ord, open reader, spawn threads all doing random lookups and confirming the ords are correct? Maybe with random cache sizes so we sometimes face evictions?

There is testCallingBulkPathReturnsCorrectResult test (I renamed it to testGetPathAndOrdinalsRandomMultithreading) which runs multiple threads. It used to only call getBulkPath, I changed it to use assertPathsAndOrdinals, which means that each thread somewhat randomly calls getPath, getBulkPath, getOrdinal and the new getBulkOrdinals. But maybe we can improve it:

It creates an array of 1K random facets, and the cache size DEFAULT_CACHE_VALUE is 4K, so there are no evictions AFAIU. I can change the array size to something like 10K to sometimes cause evictions.

I'm not sure if calling all 4 methods in test threads is the right thing to do as it reduces chances for "collisions" e.g. calling getBulkPath and getBulkOrdinals in parallel is unlikely to reveal any issues because these 2 methods use different cache instances, etc. I think I can split into 2 tests: one to randomly call getPath+getBulkPath in multiple threads, and another one to do the same for getOrdinal+getBulkOrdinals?

Pushed the branch with the changes, please review!

mikemccand · 2023-11-06T12:20:16Z

+   *     found.
+   */
+  public int[] getBulkOrdinals(FacetLabel... categoryPath) throws IOException {
+    int[] ords = new int[categoryPath.length];


I love the above javadoc, but maybe additionally add comment here that this is the simple & slow base class / default implementation, and note that DirectoryTaxonomyReader has an optimized and more complex version?

epotyom · 2023-11-06T17:46:24Z

Thank you for reviewing @mikemccand !

I wonder what performance impact this might have for heavy Lucene facet use cases... but we don't need to block this nice API with specific benchmarking. It can come later. Maybe we could (later) fix Lucene's nightly benchmark in luceneutil for faceting tasks to use this new API... maybe open an issue there?

I can make sure that we have a task that calls this method (indirectly) in the next step for this issue - adding bulk Facets#getSpecificValues, will that be ok?

mikemccand · 2023-11-09T14:56:40Z

I can make sure that we have a task that calls this method (indirectly) in the next step for this issue - adding bulk Facets#getSpecificValues, will that be ok?

+1, thanks!

mikemccand

Looks great, thank you @epotyom -- I'll merge.

mikemccand · 2023-11-09T15:01:01Z

-    String randomArray[] = new String[RandomizedTest.randomIntBetween(1, 1000)];
+    final int maxNumberOfLabelsToIndex = 1000;
+    final int maxNumberOfUniqueLabelsToIndex = maxNumberOfLabelsToIndex / 2;
+    final int cacheSize = maxNumberOfUniqueLabelsToIndex / 2; // to cause some cache evictions


mikemccand · 2023-11-09T15:28:48Z

I think this is safe to backport to 9.x? I'll do that, and move the CHANGES.txt entry down.

uschindler · 2023-11-09T18:08:32Z

Hi, the commit causes test failures like this from time to time:

org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader > testGetPathAndOrdinalsRandomMultithreading FAILED
    com.carrotsearch.randomizedtesting.UncaughtExceptionError: Captured an uncaught exception in thread: Thread[id=276, name=Thread-152, state=RUNNABLE, group=TGRP-TestDirectoryTaxonomyReader]

        Caused by:
        java.lang.IllegalArgumentException: bound must be positive
            at __randomizedtesting.SeedInfo.seed([EB36AAC7E6947189]:0)
            at java.base/java.util.Random.nextInt(Random.java:322)
            at randomizedtesting.runner@2.8.1/com.carrotsearch.randomizedtesting.Xoroshiro128PlusRandom.nextInt(Xoroshiro128PlusRandom.java:73)
            at randomizedtesting.runner@2.8.1/com.carrotsearch.randomizedtesting.AssertingRandom.nextInt(AssertingRandom.java:87)
            at org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader.assertGettingPaths(TestDirectoryTaxonomyReader.java:519)
            at org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader$4.run(TestDirectoryTaxonomyReader.java:721)

And hundreds of more threads following. Looks like sometimes the ordinals array is initialized by zero length. The followup random then fail because the upper bound passed to random#nextInt(limit) must be limit>=1.

uschindler · 2023-11-09T18:10:14Z

Looks like the ordinals array sizes must be at least 1, so in general the initial setup of the ordinal size must use numOrdinals = random(limit) + 1;

mikemccand · 2023-11-09T18:52:27Z

Thanks Uwe and sorry! I think Egor is digging on this or I’ll revert soon. Mike

…

On Thu, Nov 9, 2023 at 1:17 PM Uwe Schindler ***@***.***> wrote: Assigned #12769 <#12769> to @mikemccand <https://github.com/mikemccand>. — Reply to this email directly, view it on GitHub <#12769 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCOXG67B2SCUM3KOMVQATYDUMWJAVCNFSM6AAAAAA67KSVAGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQHEYTMNJUGE4DIMQ> . You are receiving this because you were assigned.Message ID: ***@***.***>

epotyom · 2023-11-09T19:06:53Z

Hi all, Sorry for the bug, this pull request should fix it: #12790 Kind regards, Egor On Thu, 9 Nov 2023 at 18:52, Michael McCandless ***@***.***> wrote:

…

Thanks Uwe and sorry! I think Egor is digging on this or I’ll revert soon. Mike On Thu, Nov 9, 2023 at 1:17 PM Uwe Schindler ***@***.***> wrote: > Assigned #12769 <#12769> to > @mikemccand <https://github.com/mikemccand>. > > — > Reply to this email directly, view it on GitHub > <#12769 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAGCOXG67B2SCUM3KOMVQATYDUMWJAVCNFSM6AAAAAA67KSVAGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQHEYTMNJUGE4DIMQ> > . > You are receiving this because you were assigned.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub <#12769 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2PTB64YT2MERZA3TGKEKTYDUQ7NAVCNFSM6AAAAAA67KSVAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGM4TMOJWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

xande · 2026-10-06T19:49:03Z

+      }
+    }
+    // all ordinals found in cache
+    if (indexesMissingFromCache.length == 0) {


With AI pocking around the code, I found that this condition never passes.
Fix: #16783

mikemccand reviewed Nov 6, 2023

View reviewed changes

epotyom force-pushed the faceting-getBulOrdinals branch from 6c40956 to 7384fea Compare November 7, 2023 17:44

Add TaxonomyReader#getBulkOrdinals method (apache#12180)

dc943c5

epotyom force-pushed the faceting-getBulOrdinals branch from 7384fea to dc943c5 Compare November 8, 2023 10:52

mikemccand approved these changes Nov 9, 2023

View reviewed changes

mikemccand merged commit fb5f491 into apache:main Nov 9, 2023

mikemccand pushed a commit that referenced this pull request Nov 9, 2023

Add TaxonomyReader#getBulkOrdinals method (#12180) (#12769)

51e271b

uschindler added this to the 9.9.0 milestone Nov 9, 2023

uschindler linked an issue Nov 9, 2023 that may be closed by this pull request

Add Facets#getSpecificValues (bulk) and bulk path -> ordinal lookup for taxonomy faceting #12180

Closed

uschindler removed a link to an issue Nov 9, 2023

Add Facets#getSpecificValues (bulk) and bulk path -> ordinal lookup for taxonomy faceting #12180

Closed

uschindler assigned mikemccand Nov 9, 2023

uschindler added the type:enhancement label Nov 9, 2023

epotyom mentioned this pull request Nov 9, 2023

Fix random test TestDirectoryTaxonomyReader#testGetPathAndOrdinalsRandomMultithreading #12790

Merged

epotyom mentioned this pull request Nov 9, 2023

Add Facets#getSpecificValues (bulk) and bulk path -> ordinal lookup for taxonomy faceting #12180

Closed

xande reviewed Oct 6, 2026

View reviewed changes

Conversation

epotyom commented Nov 6, 2023

Uh oh!

mikemccand left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epotyom commented Nov 6, 2023

Uh oh!

mikemccand commented Nov 9, 2023

Uh oh!

mikemccand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikemccand commented Nov 9, 2023

Uh oh!

uschindler commented Nov 9, 2023

Uh oh!

uschindler commented Nov 9, 2023

Uh oh!

mikemccand commented Nov 9, 2023 via email

Uh oh!

epotyom commented Nov 9, 2023 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mikemccand left a comment •

edited

Loading