Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-17477: Support custom aggregates in plugin code #2742

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

SOLR-17477: Support custom aggregates in plugin code #2742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7955f2b
SOLR-17477: Support custom aggregates in plugin code
timatbw Oct 4, 2024
90a251c
SOLR-17477: Set private/protected/final on public classes in facet pa…
timatbw Oct 15, 2024
0502e60
SOLR-17477: Revert registration of agg parser changes, not needed
timatbw Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/search/facet/DocValuesAcc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
* org.apache.lucene.index.DocValues}
*/
public abstract class DocValuesAcc extends SlotAcc {
SchemaField sf;
protected final SchemaField sf;

public DocValuesAcc(FacetContext fcontext, SchemaField sf) throws IOException {
super(fcontext);
Expand Down
17 changes: 9 additions & 8 deletions solr/core/src/java/org/apache/solr/search/facet/FacetModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ static class FacetComponentState {
}

// base class for facet functions that can be used in a sort
abstract static class FacetSortableMerger extends FacetMerger {
public abstract static class FacetSortableMerger extends FacetMerger {
public void prepareSort() {}

@Override
Expand All @@ -386,7 +386,7 @@ public void finish(Context mcontext) {
public abstract int compareTo(FacetSortableMerger other, FacetRequest.SortDirection direction);
}

abstract static class FacetDoubleMerger extends FacetSortableMerger {
public abstract static class FacetDoubleMerger extends FacetSortableMerger {
@Override
public abstract void merge(Object facetResult, Context mcontext);

Expand Down Expand Up @@ -422,8 +422,8 @@ public static int compare(double a, double b, FacetRequest.SortDirection directi
}
}

static class FacetLongMerger extends FacetSortableMerger {
long val;
public static class FacetLongMerger extends FacetSortableMerger {
private long val;

@Override
public void merge(Object facetResult, Context mcontext) {
Expand All @@ -442,8 +442,9 @@ public int compareTo(FacetSortableMerger other, FacetRequest.SortDirection direc
}

// base class for facets that create buckets (and can hence have sub-facets)
abstract static class FacetBucketMerger<FacetRequestT extends FacetRequest> extends FacetMerger {
FacetRequestT freq;
public abstract static class FacetBucketMerger<FacetRequestT extends FacetRequest>
extends FacetMerger {
protected final FacetRequestT freq;

public FacetBucketMerger(FacetRequestT freq) {
this.freq = freq;
Expand Down Expand Up @@ -483,8 +484,8 @@ FacetMerger createFacetMerger(String key, Object val) {
}
}

static class FacetQueryMerger extends FacetBucketMerger<FacetQuery> {
FacetBucket bucket;
public static class FacetQueryMerger extends FacetBucketMerger<FacetQuery> {
private FacetBucket bucket;

public FacetQueryMerger(FacetQuery freq) {
super(freq);
Expand Down
40 changes: 25 additions & 15 deletions solr/core/src/java/org/apache/solr/search/facet/FacetParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.SolrParams;
Expand All @@ -33,7 +34,7 @@
import org.apache.solr.search.QParser;
import org.apache.solr.search.SyntaxError;

abstract class FacetParser<T extends FacetRequest> {
public abstract class FacetParser<T extends FacetRequest> {
protected T facet;
protected FacetParser<?> parent;
protected String key;
Expand Down Expand Up @@ -138,21 +139,30 @@ public Object parseFacetOrStat(String key, Object o) throws SyntaxError {
return parseFacetOrStat(key, type, args);
}

public interface ParseHandler {
Object doParse(FacetParser<?> parent, String key, Object args) throws SyntaxError;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, is there an other exemplar of such a naming convention like doSometh ? why not just parse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular strong feelings on this, I tend to use doSomething when it's an inner helper method called from an outer/wrapper method that is the something(..) but in this case it's not really directly called. In practice I think people would use lambda anyway and never actually write this method name. But happy to change it to parse!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, return type is a little bit tricky FacetRequest|AggValueSource I suppose it deserves to be documented via Javadoc, unless we can declare it explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already how it is, the calling code allows the parser to return either a FacetRequest or a AggValueSource hence that Object type was needed here too. It's not ideal, and perhaps in practice most people will want to produce a AggValueSource only. Creating new subclasses of FacetRequest outside the package is harder, and I've not yet looked at how feasible it is to do that from a plugin (it's not the focus of this issue). But it would be interesting if eventually you can define custom types of faceting too.

}

private static final Map<String, ParseHandler> REGISTERED_TYPES = new ConcurrentHashMap<>();

static {
ParseHandler fieldParser = (p, k, a) -> new FacetFieldParser(p, k).parse(a);
REGISTERED_TYPES.put("field", fieldParser);
REGISTERED_TYPES.put("terms", fieldParser);
REGISTERED_TYPES.put("query", (p, k, a) -> new FacetQueryParser(p, k).parse(a));
REGISTERED_TYPES.put("range", (p, k, a) -> new FacetRangeParser(p, k).parse(a));
REGISTERED_TYPES.put("heatmap", (p, k, a) -> new FacetHeatmap.Parser(p, k).parse(a));
REGISTERED_TYPES.put("func", (p, k, a) -> p.parseStat(k, a));
}

public static void registerParseHandler(String type, ParseHandler parseHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the https://issues.apache.org/jira/browse/SOLR-17477 comment w.r.t. how this is called!

Copy link
Member

@mkhludnev mkhludnev Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where to discuss it. But registering anything in static initializer is asking for trouble, imho. I'd rather hookup a component via solrconfig.xml (or a plugin??), which just calls registerParserHandler() for custom handlers.
Anyway it's a matter of taste, and should block this PR, unless someone propose to make FP.registerParserHandler protected. I tried, it was so pity, don't even show it to anyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's perhaps not the best way of hooking things up, especially as the whole process is already kicked off from solrconfig.xml where you declare a valueSourceParser. I was relying on the point at which that custom parser object is instantiated to run a static initialiser to also register it for json parsing too. But maybe it's safer to invert that and have the Solr code register the custom parser if it implements json parsing. I'll have another look and think about the alternatives.

My main reason for suggesting custom code uses a static block to register is when you have many hundreds of SolrCores in your CoreContainer and each one would be instantiated and call the register method, when really you might only need it done once. But now thinking if you run a mixed workload with different solrconfig and they're not homogenous, you don't want static registering maybe (each SolrCore is distinct)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the 'standard set' of registered types in the static block above probably is appropriate as those are built-in and common to all SolrCores.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REGISTERED_TYPES are shared across cores. It means:

  • one core might inject a custom parser, it appears across all cores. If it hooks up some large state it remains in heap after core unloading (it might be called as a leak)
  • registerPH might be called with one of the standard name bringing some surprises to other cores.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes all good points, I'm going to change this to remove the statics and make it per-core which probably fits better with how it's hooked in too

REGISTERED_TYPES.put(type, parseHandler);
}

public Object parseFacetOrStat(String key, String type, Object args) throws SyntaxError {
// TODO: a place to register all these facet types?

switch (type) {
case "field":
case "terms":
return new FacetFieldParser(this, key).parse(args);
case "query":
return new FacetQueryParser(this, key).parse(args);
case "range":
return new FacetRangeParser(this, key).parse(args);
case "heatmap":
return new FacetHeatmap.Parser(this, key).parse(args);
case "func":
return parseStat(key, args);
ParseHandler parseHandler = REGISTERED_TYPES.get(type);
if (parseHandler != null) {
return parseHandler.doParse(this, key, args);
}

throw err("Unknown facet or stat. key=" + key + " type=" + type + " args=" + args);
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/search/facet/UniqueMultiDvSlotAcc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import org.apache.lucene.util.LongValues;
import org.apache.solr.schema.SchemaField;

class UniqueMultiDvSlotAcc extends UniqueSlotAcc {
public class UniqueMultiDvSlotAcc extends UniqueSlotAcc {
SortedSetDocValues topLevel;
SortedSetDocValues[] subDvs;
OrdinalMap ordMap;
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/search/facet/UniqueMultivaluedSlotAcc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import org.apache.solr.schema.SchemaField;
import org.apache.solr.search.SolrIndexSearcher;

class UniqueMultivaluedSlotAcc extends UniqueSlotAcc implements UnInvertedField.Callback {
public class UniqueMultivaluedSlotAcc extends UniqueSlotAcc implements UnInvertedField.Callback {
private UnInvertedField uif;
private UnInvertedField.DocToTerm docToTerm;

Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/search/facet/UniqueSinglevaluedSlotAcc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import org.apache.lucene.util.LongValues;
import org.apache.solr.schema.SchemaField;

class UniqueSinglevaluedSlotAcc extends UniqueSlotAcc {
public class UniqueSinglevaluedSlotAcc extends UniqueSlotAcc {
SortedDocValues topLevel;
SortedDocValues[] subDvs;
OrdinalMap ordMap;
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/search/facet/UniqueSlotAcc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import org.apache.solr.schema.SchemaField;
import org.apache.solr.util.hll.HLL;

abstract class UniqueSlotAcc extends SlotAcc {
public abstract class UniqueSlotAcc extends SlotAcc {
HLLAgg.HLLFactory factory;
SchemaField field;
FixedBitSet[] arr;
Expand Down
33 changes: 17 additions & 16 deletions solr/core/src/test/org/apache/solr/search/function/AggValueSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.solr.SolrTestCase;
import org.apache.solr.search.facet.FacetContext;
import org.apache.solr.search.facet.FacetMerger;
import org.apache.solr.search.facet.FacetModule;
import org.apache.solr.search.facet.SimpleAggValueSource;
import org.apache.solr.search.facet.SlotAcc;
import org.junit.Test;
Expand Down Expand Up @@ -76,22 +77,22 @@ public Object getValue(int slot) {

@Override
public FacetMerger createFacetMerger(Object prototype) {
return new FacetMerger() {
double total = 0.0D;

@Override
public void merge(Object facetResult, Context mcontext) {
total += (Double) facetResult;
}

@Override
public void finish(Context mcontext) {}

@Override
public Object getMergedResult() {
return total;
}
};
// check these inner classes can be referenced by name
FacetModule.FacetSortableMerger merger =
new FacetModule.FacetDoubleMerger() {
double total = 0.0D;

@Override
public void merge(Object facetResult, Context mcontext) {
total += (Double) facetResult;
}

@Override
public double getDouble() {
return total;
}
};
return merger;
}

@Override
Expand Down
Loading