Skip to content

Commit

Permalink
Merge pull request #669 from FgForrest/668-filter-was-not-yet-called-…
Browse files Browse the repository at this point in the history
…on-selling-price-bitmap-filter-this-is-not-expected

fix(#668): Filter was not yet called on selling price bitmap filter, this is not expected!
  • Loading branch information
novoj authored Sep 13, 2024
2 parents c596a9a + 8f3911b commit d16c33e
Show file tree
Hide file tree
Showing 30 changed files with 281 additions and 183 deletions.
4 changes: 2 additions & 2 deletions documentation/user/en/operate/reference/metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
<dt>catalogName</dt>
<dd><strong>Catalog</strong>: N/A</dd>
<dt>entityType</dt>
<dd><strong>Entity type</strong>: N/A</dd>
<dt>entityType</dt>
<dd><strong>Collection</strong>: N/A</dd>
<dt>entityType</dt>
<dd><strong>Entity type</strong>: N/A</dd>
<dt>fileType</dt>
<dd><strong>File type</strong>: N/A</dd>
<dt>httpMethod</dt>
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 @@ -23,12 +23,14 @@

package io.evitadb.core.cache.payload;

import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.algebra.Formula;
import io.evitadb.core.query.algebra.price.FilteredPriceRecordAccessor;
import io.evitadb.core.query.algebra.price.filteredPriceRecords.FilteredPriceRecords;
import io.evitadb.core.query.algebra.price.termination.PriceEvaluationContext;
import io.evitadb.core.query.algebra.price.termination.PriceWrappingFormula;
import io.evitadb.index.bitmap.Bitmap;
import io.evitadb.utils.Assert;
import io.evitadb.utils.MemoryMeasuringConstants;
import lombok.Getter;

Expand All @@ -47,9 +49,9 @@ public class FlattenedFormulaWithFilteredPrices extends FlattenedFormula impleme
@Serial private static final long serialVersionUID = 29711505428272096L;
/**
* Contains information about price records leading to a computed result.
* Copies {@link FilteredPriceRecordAccessor#getFilteredPriceRecords()}.
* Copies {@link FilteredPriceRecordAccessor#getFilteredPriceRecords(QueryExecutionContext)} .
*/
@Getter private final FilteredPriceRecords filteredPriceRecords;
private final FilteredPriceRecords filteredPriceRecords;
/**
* Price evaluation context. Copies {@link PriceWrappingFormula#getPriceEvaluationContext()}.
*/
Expand All @@ -73,4 +75,15 @@ public FlattenedFormulaWithFilteredPrices(long formulaHash, long transactionalId
this.filteredPriceRecords.prepareForFlattening();
}

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
return this.filteredPriceRecords;
}

@Nonnull
public FilteredPriceRecords getFilteredPriceRecordsOrThrowException() {
Assert.isPremiseValid(this.filteredPriceRecords != null, "Filtered price records are not available.");
return this.filteredPriceRecords;
}
}
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 @@ -24,6 +24,7 @@
package io.evitadb.core.cache.payload;

import io.evitadb.api.query.require.QueryPriceMode;
import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.algebra.Formula;
import io.evitadb.core.query.algebra.price.FilteredOutPriceRecordAccessor;
import io.evitadb.core.query.algebra.price.FilteredPriceRecordAccessor;
Expand All @@ -34,6 +35,7 @@
import io.evitadb.core.query.algebra.price.termination.PriceWrappingFormula;
import io.evitadb.index.bitmap.Bitmap;
import io.evitadb.index.bitmap.RoaringBitmapBackedBitmap;
import io.evitadb.utils.Assert;
import io.evitadb.utils.MemoryMeasuringConstants;
import io.evitadb.utils.NumberUtils;
import lombok.Getter;
Expand All @@ -55,9 +57,9 @@ public class FlattenedFormulaWithFilteredPricesAndFilteredOutRecords extends Fla
@Serial private static final long serialVersionUID = -6052882250380556441L;
/**
* Contains information about price records leading to a computed result.
* Copies {@link FilteredPriceRecordAccessor#getFilteredPriceRecords()}.
* Copies {@link FilteredPriceRecordAccessor#getFilteredPriceRecords(QueryExecutionContext)}.
*/
@Getter @Nonnull private final FilteredPriceRecords filteredPriceRecords;
@Nonnull private final FilteredPriceRecords filteredPriceRecords;
/**
* Records that has been filtered out by the original formula.
* Copies {@link FilteredOutPriceRecordAccessor#getCloneWithPricePredicateFilteredOutResults()}.
Expand Down Expand Up @@ -167,4 +169,15 @@ public Formula getCloneWithPricePredicateFilteredOutResults() {
return this.memoizedClone;
}

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
return this.filteredPriceRecords;
}

@Nonnull
public FilteredPriceRecords getFilteredPriceRecordsOrThrowException() {
Assert.isPremiseValid(this.filteredPriceRecords != null, "Filtered price records must not be null.");
return this.filteredPriceRecords;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.evitadb.api.query.require.EntityRequire;
import io.evitadb.api.requestResponse.EvitaRequest;
import io.evitadb.api.requestResponse.schema.dto.GlobalAttributeSchema;
import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.algebra.AbstractFormula;
import io.evitadb.core.query.algebra.Formula;
import io.evitadb.core.query.algebra.price.FilteredPriceRecordAccessor;
Expand Down Expand Up @@ -90,11 +91,10 @@ protected Bitmap computeInternal() {

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords() {
Assert.isPremiseValid(this.executionContext != null, "The formula hasn't been initialized!");
Assert.isTrue(this.executionContext.getPrefetchedEntities() != null, () -> new EntityCollectionRequiredException("matching entities"));
return alternative instanceof FilteredPriceRecordAccessor ?
((FilteredPriceRecordAccessor) alternative).getFilteredPriceRecords() :
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
Assert.isTrue(context.getPrefetchedEntities() != null, () -> new EntityCollectionRequiredException("matching entities"));
return this.alternative instanceof FilteredPriceRecordAccessor fpra ?
fpra.getFilteredPriceRecords(context) :
new ResolvedFilteredPriceRecords();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,18 +219,20 @@ public Formula getCloneWithPricePredicateFilteredOutResults() {

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords() {
Assert.isPremiseValid(this.executionContext != null, "The formula hasn't been initialized!");
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
// if the entities were prefetched we passed the "is it worthwhile" check
return Optional.ofNullable(executionContext.getPrefetchedEntities())
return Optional.ofNullable(this.executionContext.getPrefetchedEntities())
// ask the alternative solution for filtered price records
.map(it ->
alternative instanceof FilteredPriceRecordAccessor ?
((FilteredPriceRecordAccessor) alternative).getFilteredPriceRecords() :
new ResolvedFilteredPriceRecords()
.map(it -> {
if (this.alternative instanceof FilteredPriceRecordAccessor fpra) {
return fpra.getFilteredPriceRecords(context);
} else {
return new ResolvedFilteredPriceRecords();
}
}
)
// otherwise collect the filtered records from the delegate
.orElseGet(() -> FilteredPriceRecords.createFromFormulas(this, this.compute()));
.orElseGet(() -> FilteredPriceRecords.createFromFormulas(this, this.compute(), this.executionContext));
}

@Override
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 @@ -23,6 +23,7 @@

package io.evitadb.core.query.algebra.price;

import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.algebra.Formula;
import io.evitadb.core.query.algebra.price.filteredPriceRecords.FilteredPriceRecords;
import io.evitadb.index.price.PriceListAndCurrencyPriceIndex;
Expand All @@ -46,6 +47,6 @@ public interface FilteredPriceRecordAccessor {
* additional logic that needs to work with the prices (mainly sorting) could perform quickly.
*/
@Nonnull
FilteredPriceRecords getFilteredPriceRecords();
FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context);

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

package io.evitadb.core.query.algebra.price.filteredPriceRecords;

import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.SharedBufferPool;
import io.evitadb.core.query.algebra.Formula;
import io.evitadb.core.query.algebra.price.FilteredPriceRecordAccessor;
Expand Down Expand Up @@ -83,7 +84,8 @@ public interface FilteredPriceRecords extends Serializable {
@Nonnull
static FilteredPriceRecords createFromFormulas(
@Nonnull Formula parentFormula,
@Nullable Bitmap narrowToEntityIds
@Nullable Bitmap narrowToEntityIds,
@Nonnull QueryExecutionContext context
) {
// collect all FilteredPriceRecordAccessor that were involved in computing delegate result
final Collection<FilteredPriceRecordAccessor> filteredPriceRecordAccessors = FormulaFinder.findAmongChildren(
Expand All @@ -92,7 +94,7 @@ static FilteredPriceRecords createFromFormulas(

final List<FilteredPriceRecords> filteredPriceRecords = filteredPriceRecordAccessors
.stream()
.map(FilteredPriceRecordAccessor::getFilteredPriceRecords)
.map(it -> it.getFilteredPriceRecords(context))
.map(it -> it instanceof NonResolvedFilteredPriceRecords ? ((NonResolvedFilteredPriceRecords) it).toResolvedFilteredPriceRecords() : it)
.toList();

Expand All @@ -104,7 +106,7 @@ static FilteredPriceRecords createFromFormulas(
return filteredPriceRecords.get(0);
// all price records are resolved
} else {
final Optional<LazyEvaluatedEntityPriceRecords> lazyEvaluatedEntityPriceRecords = getLazyEvaluatedEntityPriceRecords(filteredPriceRecordAccessors);
final Optional<LazyEvaluatedEntityPriceRecords> lazyEvaluatedEntityPriceRecords = getLazyEvaluatedEntityPriceRecords(filteredPriceRecordAccessors, context);
final Optional<ResolvedFilteredPriceRecords> resolvedFilteredPriceRecords;
if (narrowToEntityIds == null) {
// and no filtering is known (all contents combined should be returned)
Expand Down Expand Up @@ -133,10 +135,11 @@ static FilteredPriceRecords createFromFormulas(

@Nonnull
private static Optional<LazyEvaluatedEntityPriceRecords> getLazyEvaluatedEntityPriceRecords(
@Nonnull Collection<FilteredPriceRecordAccessor> filteredPriceRecordAccessors
@Nonnull Collection<FilteredPriceRecordAccessor> filteredPriceRecordAccessors,
@Nonnull QueryExecutionContext context
) {
final PriceListAndCurrencyPriceIndex[] priceIndexes = filteredPriceRecordAccessors.stream()
.map(FilteredPriceRecordAccessor::getFilteredPriceRecords)
final PriceListAndCurrencyPriceIndex<?, ?>[] priceIndexes = filteredPriceRecordAccessors.stream()
.map(it -> it.getFilteredPriceRecords(context))
.filter(LazyEvaluatedEntityPriceRecords.class::isInstance)
.map(LazyEvaluatedEntityPriceRecords.class::cast)
.flatMap(it -> Arrays.stream(it.getPriceIndexes()))
Expand Down Expand Up @@ -234,12 +237,13 @@ private static Optional<ResolvedFilteredPriceRecords> getResolvedFilteredPriceRe
@Nonnull
static FilteredPriceRecordsLookupResult collectFilteredPriceRecordsFromPriceRecordAccessors(
@Nonnull Collection<FilteredPriceRecordAccessor> filteredPriceRecordAccessors,
@Nonnull RoaringBitmap filterTo
@Nonnull RoaringBitmap filterTo,
@Nonnull QueryExecutionContext context
) {
final CompositeObjectArray<PriceRecordContract> collectedPriceRecords = new CompositeObjectArray<>(PriceRecordContract.class, false);
final List<PriceRecordLookup> priceRecordIterators = filteredPriceRecordAccessors
.stream()
.map(it -> it.getFilteredPriceRecords().getPriceRecordsLookup())
.map(it -> it.getFilteredPriceRecords(context).getPriceRecordsLookup())
.toList();

final int[] buffer = SharedBufferPool.INSTANCE.obtain();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

package io.evitadb.core.query.algebra.price.priceIndex;

import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.algebra.AbstractFormula;
import io.evitadb.core.query.algebra.Formula;
import io.evitadb.core.query.algebra.price.filteredPriceRecords.FilteredPriceRecords;
Expand Down Expand Up @@ -69,9 +70,9 @@ public Formula getDelegate() {

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords() {
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
return new ResolvedFilteredPriceRecords(
priceIndex.getPriceRecords(compute()),
this.priceIndex.getPriceRecords(compute()),
SortingForm.NOT_SORTED
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

package io.evitadb.core.query.algebra.price.priceIndex;

import io.evitadb.core.query.QueryExecutionContext;
import io.evitadb.core.query.algebra.AbstractCacheableFormula;
import io.evitadb.core.query.algebra.CacheableFormula;
import io.evitadb.core.query.algebra.Formula;
Expand Down Expand Up @@ -99,13 +100,13 @@ public Formula getDelegate() {

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords() {
return new LazyEvaluatedEntityPriceRecords(priceIndex);
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
return new LazyEvaluatedEntityPriceRecords(this.priceIndex);
}

@Override
public String toString() {
return "DO WITH PRICE INDEX: " + priceIndex.getPriceIndexKey();
return "DO WITH PRICE INDEX: " + this.priceIndex.getPriceIndexKey();
}

@Nonnull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public FlattenedFormula toSerializableFormula(long formulaHash, @Nonnull LongHas
.sorted()
.toArray(),
computationalResult,
FilteredPriceRecords.createFromFormulas(this, computationalResult),
FilteredPriceRecords.createFromFormulas(this, computationalResult, this.executionContext),
priceEvaluationContext
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
* It may also filter out entity ids which don't pass {@link #pricePredicate} predicate test.
*
* This formula consumes and produces {@link Formula} of {@link PriceRecord#entityPrimaryKey() entity ids}. It uses
* information from underlying formulas that implement {@link FilteredPriceRecordAccessor#getFilteredPriceRecords()}
* information from underlying formulas that implement {@link FilteredPriceRecordAccessor#getFilteredPriceRecords(QueryExecutionContext)}
* to access the lowest price of each entity/inner record id combination for filtering purposes.
*
* @author Jan Novotný (novotny@fg.cz), FG Forrest a.s. (c) 2021
Expand Down Expand Up @@ -222,14 +222,17 @@ public CacheableFormula getCloneWithComputationCallback(@Nonnull Consumer<Cachea

@Nonnull
@Override
public FilteredPriceRecords getFilteredPriceRecords() {
Assert.notNull(filteredPriceRecords, "Call #compute() method first!");
public FilteredPriceRecords getFilteredPriceRecords(@Nonnull QueryExecutionContext context) {
if (filteredPriceRecords == null) {
// init the records first
compute();
}
return filteredPriceRecords;
}

@Override
public String toString() {
return pricePredicate.toString();
return this.pricePredicate.toString();
}

@Override
Expand All @@ -242,13 +245,13 @@ public FlattenedFormula toSerializableFormula(long formulaHash, @Nonnull LongHas
.sorted()
.toArray(),
compute(),
getFilteredPriceRecords(),
getFilteredPriceRecords(this.executionContext),
Objects.requireNonNull(getRecordsFilteredOutByPredicate()),
getPriceEvaluationContext(),
pricePredicate.getQueryPriceMode(),
pricePredicate.getFrom(),
pricePredicate.getTo(),
pricePredicate.getIndexedPricePlaces()
this.pricePredicate.getQueryPriceMode(),
this.pricePredicate.getFrom(),
this.pricePredicate.getTo(),
this.pricePredicate.getIndexedPricePlaces()
);
}

Expand Down Expand Up @@ -277,7 +280,7 @@ protected Bitmap computeInternal() {
// collect price iterators ordered by price list importance
final PriceRecordLookup[] priceRecordIterators = filteredPriceRecordAccessors
.stream()
.map(FilteredPriceRecordAccessor::getFilteredPriceRecords)
.map(it -> it.getFilteredPriceRecords(this.executionContext))
.map(FilteredPriceRecords::getPriceRecordsLookup)
.toArray(PriceRecordLookup[]::new);
// create array for the lowest prices by entity
Expand Down
Loading

0 comments on commit d16c33e

Please sign in to comment.