Utility classes to make it easier to use sandbox facet API for most common cases #14237

epotyom · 2025-02-13T21:58:33Z

In the initial sandbox facet module PR @gsmiller suggested adding helpers to make common tasks easier.

This implementation uses existing type FacetResult to return results. The type has limitations, e.g. it supports only one value per label. At the same time, it is simple, users are already familiar with it, and it is probably good enough for the most common tasks. Users who need more complex results can use the lower level API FacetCutter, FacetRecorder, etc to get multiple aggregations per label, to implement custom sort order or any other non-trivial faceting task.

It's a draft, if people are ok with the approach overall I will add more unit tests and javadocs.

Simplifies requesting taxonomy and long range facets Makes sure we don't count docs two times for the same index field when there are multiple requests Returns FacetResult - to make it easier to migrate for classic facets module users TODOs: - better names - maybe use names suggested in TK - implement for all existing facet cutters - add comprehensive tests - fix total value where possible in tests

…s: reuse some code, make more abstract

stefanvodita

I had a glance at everything and didn't see anything objectionable. It's great if we're making this module easier to use. I have some non-blocking concerns that this is a lot of new code, with more abstractions, and it doesn't have a user waiting to use it. There are arguments against those concerns as well and I personally like the new aggregations, but I thought it was worth raising the point.

stefanvodita · 2025-02-14T16:56:23Z

lucene/demo/src/java/org/apache/lucene/demo/facet/SandboxFacetsExample.java

@@ -130,6 +135,88 @@ void index() throws IOException {
    IOUtils.close(indexWriter, taxoWriter);
  }

+  /**
+   * Example for {@link FacetBuilder} usage - simple API that provides results in a format very
+   * similar to classic facets module. It doesn't give all flexibility available with {@link


This really does look more familiar! Can we highlight it somewhere more prominent as well? Maybe in the faceting guide.

Good idea, I'll do that.

Would it be a good idea to have a bidirectional link for the shared examples in the classic and new aggregation modules?
Edit: nvm, I think the examples are a bit different.

Thanks for creating the FacetBuilder and FacetOrchestrator abstractions - they do really simplify the usage of the new API.

stefanvodita · 2025-02-14T17:02:07Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/utils/CommonFacetBuilder.java

+  }
+
+  @Override
+  FacetCutter createFacetCutter() {


Would it be a problem that this is called create, giving the impression that we get a new cutter? Reusing the old one could be problematic, couldn't it, if the user expects a new cutter?

Ah you are right, this method doesn't have to create FacetCutter, e.g. FacetCutter might be created in advance and returned by the method. Renamed to getFacetCutter

stefanvodita · 2025-02-14T17:07:43Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/utils/BaseFacetBuilder.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.sandbox.facet.utils;


Should we have a package for the facet builders?

I want to keep *FacetOrchstrators in the same package as FacetBuilders, so that we can keep some FacetBuilder method package-private. So the only file that maybe doesn't belong in the same package is ComparableUtils.java. At the same time the idea of ComparableUtils is similar - make it easier for users to use the new module, so I thought we could reduce number of packages and keep meaningful package structure at the same time... But I don't feel strongly about it. WDYT?

I get your argument. Let's keep it as is then.

epotyom · 2025-02-17T11:11:38Z

Thanks for reviewing @stefanvodita !

I have some non-blocking concerns that this is a lot of new code, with more abstractions, and it doesn't have a user waiting to use it. There are arguments against those concerns as well and I personally like the new aggregations, but I thought it was worth raising the point.

I agree with the concern, at the same time it might be one of these chicken and egg cases when users avoid using the feature just because it doesn't have simple enough API for simple use cases?

One use case for the new utils I have in mind is luceneutil perf tests - it puts me off that we need 90 lines to test performance of the new API, while for existing facets module it is only two lines.

stefanvodita

Thank you for addressing my comments! I'm up for merging this, while keeping in mind the concerns we discussed.

Shradha26 · 2025-02-18T12:56:05Z

lucene/demo/src/java/org/apache/lucene/demo/facet/SandboxFacetsExample.java

+            + topDocs.scoreDocs.length);
+
+    //// Results
+    FacetResult autorResults = authorFacetBuilder.getResult();


[nit] authorResults instead of autorResults?

Shradha26 · 2025-02-18T14:34:12Z

lucene/demo/src/java/org/apache/lucene/demo/facet/SandboxFacetsExample.java

+        .collect(query, ds);
+
+    //// Results
+    FacetResult autorResults = authorFacetBuilder.getResult();


[nit] authorResults instead of autorResults?

Shradha26 · 2025-02-18T14:37:57Z

lucene/demo/src/java/org/apache/lucene/demo/facet/SandboxFacetsExample.java

+            "Price",
+            new LongRange("0-10", 0, true, 10, true),
+            new LongRange("10-20", 10, true, 20, true));
+


To maintain output consistency between different examples, we could add something equivalent to what we have in simpleFacetsWithSearch ? Or, we could remove it from there.

System.out.println( "Search results: totalHits: " + topDocs.totalHits + ", collected hits: " + topDocs.scoreDocs.length);

I did this intentionally, to demonstrate that there are methods to do facet only and facets with search (examples that haveWithSearch suffix also compute/print TopDocs). We can rename this example to simpleFacetsOnlyWithDrillSideways maybe?

Shradha26 · 2025-02-18T14:47:12Z

lucene/demo/src/java/org/apache/lucene/demo/facet/SandboxFacetsExample.java

+
+    //// Search and collect
+    TopDocs topDocs =
+        new FacetOrchestrator()


How do we handle cases where more than one FacetBuilder instances specify topN?

Each FacetBuilder can have its own topN value set by withTopN(), which is used to compute FacetBuilder#getResult

Shradha26 · 2025-02-18T14:53:59Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/utils/LongRangeFacetBuilder.java

+import org.apache.lucene.sandbox.facet.labels.RangeOrdToLabel;
+
+/** {@link FacetBuilder} factory for long range facets. */
+public final class LongRangeFacetBuilder {


Why don't we make LongRangeFacetBuilder extend CommonFacetBuilder? The create method returning something of a different type is a bit confusing to me.

The idea is that CommonFacetBuilder can be used to compute results for long range facets, so there is no need to create a new FacetBuilder subclass. But I thought it would be nice to have a utility class (I should have added a private constructor to make it static-only, will do) that helps to get CommonFacetBuilder instance.

I can remove the class and let users rely on examples, or rename it to RangeFacetBuilder and add similar methods for double ranges. WDYT?

Shradha26 · 2025-02-18T14:56:42Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/utils/CommonFacetBuilder.java

+ * Common {@link FacetBuilder} that works with any {@link FacetCutter} and {@link OrdToLabel}
+ * provided by user.
+ */
+public final class CommonFacetBuilder extends BaseFacetBuilder<CommonFacetBuilder> {


I'm not sure when I should be using CommonFacetBuilder vs BaseFacetBuilder? Some examples, even in the documentation would help? For e.g, why can't TaxonomyFacetBuilder extend CommonFacetBuilder?

BaseFacetBuilder is abstract package-private class that implements common functionality that different FacetBuilder implementations can reuse.

CommonFacetBuilder is a final class, kind of generic facet builder that can be used with any FacetCutter, but as a consequence it lacks some facet type specific functionality. E.g. using it for taxonomy facets makes sense only if we don't need to specify parent path and don't need to compute overall value. The idea is that creating FacetBuilder should not be required for every facet type, just for the types that require some custom logic, such as taxonomy facets.

WDYT?

Shradha26 · 2025-02-18T14:57:58Z

lucene/demo/src/java/org/apache/lucene/demo/facet/SandboxFacetsExample.java

@@ -130,6 +135,88 @@ void index() throws IOException {
    IOUtils.close(indexWriter, taxoWriter);
  }

+  /**
+   * Example for {@link FacetBuilder} usage - simple API that provides results in a format very
+   * similar to classic facets module. It doesn't give all flexibility available with {@link


Thanks for creating the FacetBuilder and FacetOrchestrator abstractions - they do really simplify the usage of the new API.

epotyom added 8 commits February 13, 2025 09:57

TMP: DrillSidewaysFacetOrchestrator.

222a172

TMP: DrillSidewaysFacetOrchestrator part 2

4b1875b

Make FacetBuilder abstract class

6462b9d

tidy, licence and other small changes to pass gradlew check

d58ee7c

Some FacetOrchestrator and DrillSidewaysFacetOrchestrator improvement…

3a4a753

…s: reuse some code, make more abstract

add CommonFacetBuilder, some unit tests and small canges

0dd7161

Minor changes

41d5905

github-actions bot added module:demo module:sandbox labels Feb 13, 2025

stefanvodita reviewed Feb 14, 2025

View reviewed changes

private LongRangeFacetBuilder ctor

b4594ed

epotyom added 2 commits February 17, 2025 11:53

Mention sandbox facet module in facets userguide

cd719db

Rename BaseFacetBuilder#createFacetCutter to #getFacetCutter

0dcb7c6

stefanvodita approved these changes Feb 17, 2025

View reviewed changes

This was referenced Feb 18, 2025

Use in progress lucene PR #14237 epotyom/luceneutil#1

Draft

Enable comparing main facets module agains sandbox facets implementation mikemccand/luceneutil#325

Open

Shradha26 reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utility classes to make it easier to use sandbox facet API for most common cases #14237

Utility classes to make it easier to use sandbox facet API for most common cases #14237

epotyom commented Feb 13, 2025

stefanvodita left a comment

stefanvodita Feb 14, 2025

epotyom Feb 17, 2025

Shradha26 Feb 18, 2025 •

edited

Loading

Shradha26 Feb 18, 2025

stefanvodita Feb 14, 2025

epotyom Feb 17, 2025

stefanvodita Feb 14, 2025

epotyom Feb 17, 2025

stefanvodita Feb 17, 2025

epotyom commented Feb 17, 2025

stefanvodita left a comment

Shradha26 Feb 18, 2025

Shradha26 Feb 18, 2025

Shradha26 Feb 18, 2025

epotyom Feb 18, 2025

Shradha26 Feb 18, 2025

epotyom Feb 18, 2025

Shradha26 Feb 18, 2025

epotyom Feb 18, 2025

Shradha26 Feb 18, 2025

epotyom Feb 18, 2025

Shradha26 Feb 18, 2025

Utility classes to make it easier to use sandbox facet API for most common cases #14237

Are you sure you want to change the base?

Utility classes to make it easier to use sandbox facet API for most common cases #14237

Conversation

epotyom commented Feb 13, 2025

stefanvodita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shradha26 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epotyom commented Feb 17, 2025

stefanvodita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shradha26 Feb 18, 2025 •

edited

Loading