Skip to content

Commit

Permalink
Merge pull request #599 from FgForrest/598-selecting-facet-combinatio…
Browse files Browse the repository at this point in the history
…n-from-different-groups-enables-hassense-for-invalid-facets

fix(#598): Selecting facet combination from different groups enables …
  • Loading branch information
novoj committed Jun 6, 2024
2 parents b710305 + b9bd4d3 commit 97c4729
Show file tree
Hide file tree
Showing 14 changed files with 356 additions and 138 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ public String prettyPrint(
statistics
.getFacetStatistics()
.stream()
.map(facet -> "\t\t[" + (facet.isRequested() ? "X" : " ") + "] " +
.map(facet -> "\t\t[" + (facet.isRequested() ? "X" : (ofNullable(facet.getImpact()).map(RequestImpact::hasSense).orElse(true) ? " " : "-")) + "] " +
ofNullable(facetRenderer.apply(facet)).filter(it -> !it.isBlank()).orElseGet(() -> String.valueOf(facet.getFacetEntity().getPrimaryKey())) +
" (" + facet.getCount() + ")" +
ofNullable(facet.getImpact()).map(RequestImpact::toString).map(it -> " " + it).orElse(""))
Expand Down Expand Up @@ -361,8 +361,12 @@ public int hashCode() {
* @param difference Projected number of entities that are added or removed from result if the query is altered by adding this
* facet to filtering query in comparison to current result.
* @param matchCount Projected number of filtered entities if the query is altered by adding this facet to filtering query.
* @param hasSense Selection has sense - TRUE if there is at least one entity still present in the result if
* the query is altered by adding this facet to filtering query. In case of OR relation between
* facets it's also true only if there is at least one entity present in the result when all other
* facets in the same group are removed and only this facet is requested.
*/
public record RequestImpact(int difference, int matchCount) implements Serializable {
public record RequestImpact(int difference, int matchCount, boolean hasSense) implements Serializable {
@Serial private static final long serialVersionUID = 8332603848272953977L;

/**
Expand All @@ -373,24 +377,16 @@ public int difference() {
return difference;
}

/**
* Selection has sense - TRUE if there is at least one entity still present in the result if the query is
* altered by adding this facet to filtering query.
*/
public boolean hasSense() {
return matchCount > 0;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof RequestImpact that)) return false;
return difference() == that.difference() && matchCount() == that.matchCount();
return difference() == that.difference() && this.matchCount() == that.matchCount() && this.hasSense() == that.hasSense();
}

@Override
public int hashCode() {
return Objects.hash(difference(), matchCount());
return Objects.hash(difference(), matchCount(), hasSense());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ public void visit(@Nonnull Formula formula) {
* @return The base entity IDs as a Bitmap.
*/
@Nonnull
protected Bitmap getBaseEntityIds(@Nonnull Bitmap[] facetEntityIds) {
protected static Bitmap getBaseEntityIds(@Nonnull Bitmap[] facetEntityIds) {
if (facetEntityIds.length == 0) {
return EmptyBitmap.INSTANCE;
} else if (facetEntityIds.length == 1) {
Expand Down Expand Up @@ -677,18 +677,27 @@ protected static class MutableFormulaFinderAndReplacer implements FormulaVisitor
*/
private final Deque<Formula> formulaStack = new ArrayDeque<>(16);
/**
* Flag that is set to true if the visitor found the target {@link MutableFormula}.
* Reference to {@link MutableFormula} found.
*/
@Getter private boolean targetFound;
@Getter
private MutableFormula target;

/**
* Returns true if the target {@link MutableFormula} has been found.
* @return True if the target {@link MutableFormula} has been found.
*/
public boolean isTargetFound() {
return target != null;
}

@Override
public void visit(@Nonnull Formula formula) {
if (!targetFound) {
if (target == null) {
if (formula instanceof MutableFormula mutableFormula) {
if (targetFound) {
if (target != null) {
throw new GenericEvitaInternalError("Expected single MutableFormula in the formula tree!");
} else {
targetFound = true;
target = mutableFormula;
mutableFormula.setDelegate(formulaToReplaceSupplier.get());
for (Formula parentFormula : formulaStack) {
parentFormula.clearMemory();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* | __/\ V /| | || (_| | |_| | |_) |
* \___| \_/ |_|\__\__,_|____/|____/
*
* Copyright (c) 2023
* Copyright (c) 2023-2024
*
* Licensed under the Business Source License, Version 1.1 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -34,6 +34,7 @@
import io.evitadb.core.query.algebra.facet.FacetGroupOrFormula;
import io.evitadb.index.bitmap.BaseBitmap;
import io.evitadb.index.bitmap.Bitmap;
import io.evitadb.utils.Assert;
import io.evitadb.utils.CollectionUtils;

import javax.annotation.Nonnull;
Expand Down Expand Up @@ -170,6 +171,39 @@ protected boolean handleUserFilter(@Nonnull Formula formula, @Nonnull Formula[]
}
}

/**
* We need to calculate whether the `facetId` returns any results when other facets in the same group are removed.
*
* @param hypotheticalFormula the current formula including this facet and all other facets
* @param referenceSchema the reference schema of the facet group
* @param facetGroupId the facet group id
* @param facetId the examined facet id
* @param facetEntityIds the examined facet entity ids
* @return true when there is at least one result when the formula is altered in a way, that the `facetId` is requested
* on its own in the facet group OR formula
*/
public boolean hasSenseAlone(
@Nonnull Formula hypotheticalFormula,
@Nonnull ReferenceSchemaContract referenceSchema,
@Nullable Integer facetGroupId,
int facetId,
@Nonnull Bitmap[] facetEntityIds
) {
if (this.isFacetGroupConjunction.test(referenceSchema, facetGroupId)) {
return !hypotheticalFormula.compute().isEmpty();
} else {
final Bitmap facetEntityIdsBitmap = getBaseEntityIds(facetEntityIds);
final MutableFormulaFinderAndReplacer mutableFormulaFinderAndReplacer = new MutableFormulaFinderAndReplacer(
() -> new FacetGroupOrFormula(referenceSchema.getName(), facetGroupId, new BaseBitmap(facetId), facetEntityIdsBitmap)
);
hypotheticalFormula.accept(mutableFormulaFinderAndReplacer);
Assert.isPremiseValid(mutableFormulaFinderAndReplacer.isTargetFound(), "Expected single MutableFormula in the formula tree!");
return mutableFormulaFinderAndReplacer.getTarget().suppressPivot(
() -> !hypotheticalFormula.compute().isEmpty()
);
}
}

/**
* Represents a cache key used for caching formula generation results. The cache key contains all key information
* needed to distinguish the situation when we need to analyze and create new formula composition and we can reuse
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* | __/\ V /| | || (_| | |_| | |_) |
* \___| \_/ |_|\__\__,_|____/|____/
*
* Copyright (c) 2023
* Copyright (c) 2023-2024
*
* Licensed under the Business Source License, Version 1.1 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -60,9 +60,9 @@ public class MemoizingFacetCalculator implements FacetCalculator, ImpactCalculat
*/
private final Formula baseFormulaWithoutUserFilter;
/**
* Contains "no-impact" result for all facets that are already selected in {@link EvitaRequest}.
* Contains current match count (the base-line).
*/
private final RequestImpact base;
private final int baseMatchCount;
/**
* Contains instance of {@link FacetFormulaGenerator} that is reused for all calls. Visitors instances are usually
* created for single use and then thrown away but here we expect a lot of repeated computations for facets and
Expand All @@ -86,7 +86,7 @@ public MemoizingFacetCalculator(
// now replace common parts of the formula with cached counterparts
this.baseFormula = queryContext.analyse(optimizedFormula);
this.baseFormulaWithoutUserFilter = baseFormulaWithoutUserFilter;
this.base = new RequestImpact(0, baseFormula.compute().size());
this.baseMatchCount = baseFormula.compute().size();
this.facetFormulaGenerator = new FacetFormulaGenerator(
queryContext::isFacetGroupConjunction,
queryContext::isFacetGroupDisjunction,
Expand All @@ -102,32 +102,29 @@ public MemoizingFacetCalculator(
@Nullable
@Override
public RequestImpact calculateImpact(@Nonnull ReferenceSchemaContract referenceSchema, int facetId, @Nullable Integer facetGroupId, boolean required, @Nonnull Bitmap[] facetEntityIds) {
if (required) {
// facet is already selected in request - return "no impact" result quickly
return base;
} else {
// create formula that would capture the requested facet selected
final Formula hypotheticalFormula = impactFormulaGenerator.generateFormula(
baseFormula, baseFormulaWithoutUserFilter, referenceSchema, facetGroupId, facetId, facetEntityIds
);
// compute the hypothetical result
final int hypotheticalCount = hypotheticalFormula.compute().size();
// and return computed impact
return new RequestImpact(
hypotheticalCount - base.matchCount(),
hypotheticalCount
);
}
// create formula that would capture the requested facet selected
final Formula hypotheticalFormula = impactFormulaGenerator.generateFormula(
baseFormula, baseFormulaWithoutUserFilter, referenceSchema, facetGroupId, facetId, facetEntityIds
);
// compute the hypothetical result
final int hypotheticalCount = hypotheticalFormula.compute().size();
// and return computed impact
final int difference = hypotheticalCount - this.baseMatchCount;
return new RequestImpact(
difference,
hypotheticalCount,
hypotheticalCount > 0 &&
(difference != 0 || impactFormulaGenerator.hasSenseAlone(hypotheticalFormula, referenceSchema, facetGroupId, facetId, facetEntityIds))
);
}

@Nonnull
@Override
public Formula createCountFormula(@Nonnull ReferenceSchemaContract referenceSchema, int facetId, @Nullable Integer facetGroupId, @Nonnull Bitmap[] facetEntityIds) {
// create formula that would capture all mandatory filtering constraints plus this single facet selected
final Formula formula = facetFormulaGenerator.generateFormula(
return facetFormulaGenerator.generateFormula(
baseFormula, baseFormulaWithoutUserFilter, referenceSchema, facetGroupId, facetId, facetEntityIds
);
return formula;
}

@Nonnull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@
import io.evitadb.core.query.algebra.FormulaVisitor;
import io.evitadb.core.query.algebra.facet.FacetGroupFormula;
import io.evitadb.index.bitmap.Bitmap;
import io.evitadb.utils.Assert;

import javax.annotation.Nonnull;
import java.util.Objects;
import java.util.function.BooleanSupplier;

/**
* This formula is a HACK and don't use it!!!
Expand All @@ -46,6 +48,7 @@
* @author Jan Novotný (novotny@fg.cz), FG Forrest a.s. (c) 2024
*/
class MutableFormula implements Formula {
private boolean suppressPivot;
private FacetGroupFormula pivot;
private FacetGroupFormula delegate;
private FacetGroupFormula result;
Expand Down Expand Up @@ -188,14 +191,31 @@ public String toStringVerbose() {
return getInnerFormula().toStringVerbose();
}

/**
* Method allows to calculate {@link #compute()} result based only on {@link #delegate}, suppressing combination
* with {@link #pivot} formula even if this is set.
*
* @param lambda the lambda to be executed
* @return the result of the lambda
*/
public boolean suppressPivot(@Nonnull BooleanSupplier lambda) {
try {
Assert.isPremiseValid(this.result == null, "Cannot suppress pivot when the result is already computed!");
this.suppressPivot = true;
return lambda.getAsBoolean();
} finally {
this.suppressPivot = false;
}
}

/**
* Returns the inner formula. If the pivot is set, the result is merged with the pivot.
* @return the inner formula
*/
@Nonnull
private FacetGroupFormula getInnerFormula() {
if (this.result == null) {
if (this.pivot != null) {
if (this.pivot != null && !this.suppressPivot) {
this.result = this.pivot.mergeWith(this.delegate);
} else {
this.result = this.delegate;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* | __/\ V /| | || (_| | |_| | |_) |
* \___| \_/ |_|\__\__,_|____/|____/
*
* Copyright (c) 2023
* Copyright (c) 2023-2024
*
* Licensed under the Business Source License, Version 1.1 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -28,6 +28,7 @@
import io.evitadb.api.requestResponse.data.structure.EntityReference;
import io.evitadb.api.requestResponse.extraResult.FacetSummary;
import io.evitadb.api.requestResponse.extraResult.FacetSummary.FacetGroupStatistics;
import io.evitadb.api.requestResponse.extraResult.FacetSummary.RequestImpact;
import io.evitadb.externalApi.grpc.generated.GrpcEntityReference;
import io.evitadb.externalApi.grpc.generated.GrpcExtraResults.Builder;
import io.evitadb.externalApi.grpc.generated.GrpcFacetGroupStatistics;
Expand Down Expand Up @@ -75,9 +76,12 @@ public static void buildFacetSummary(@Nonnull Builder extraResults, @Nonnull Fac
statisticsBuilder.setFacetEntity(EntityConverter.toGrpcSealedEntity(entity));
}

if (facetStatistic.getImpact() != null) {
statisticsBuilder.setImpact(Int32Value.newBuilder().setValue(facetStatistic.getImpact().difference()).build());
statisticsBuilder.setMatchCount(Int32Value.newBuilder().setValue(facetStatistic.getImpact().matchCount()).build());
final RequestImpact impact = facetStatistic.getImpact();
if (impact != null) {
statisticsBuilder.
setImpact(Int32Value.newBuilder().setValue(impact.difference()).build())
.setMatchCount(Int32Value.newBuilder().setValue(impact.matchCount()).build())
.setHasSense(impact.hasSense());
}

final GrpcFacetStatistics statistics = statisticsBuilder.build();
Expand Down

0 comments on commit 97c4729

Please sign in to comment.