-
Notifications
You must be signed in to change notification settings - Fork 3.8k
add mechanism to control filter optimization in historical query processing #8209
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.
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
Changes from all commits
4177dc3
d4d6629
383eeb4
cc9c9b0
212c9a8
f7b8a49
ef28052
922c740
9587425
1f65d05
3fbf5e6
6b86b32
bb48537
d0d7f05
1aa5484
359cf4f
2be81b0
a6e6a31
346e8fd
4402160
b8a3cae
cdb63a7
35098b0
9e6a2e0
8fc735a
505b668
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,18 +20,21 @@ | |
| package org.apache.druid.query.filter; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonCreator; | ||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.base.Preconditions; | ||
| import com.google.common.base.Predicate; | ||
| import com.google.common.collect.ImmutableSet; | ||
| import com.google.common.collect.RangeSet; | ||
| import com.google.common.collect.Sets; | ||
| import com.google.common.hash.HashCode; | ||
| import org.apache.druid.java.util.common.StringUtils; | ||
| import org.apache.druid.query.cache.CacheKeyBuilder; | ||
| import org.apache.druid.query.extraction.ExtractionFn; | ||
| import org.apache.druid.segment.filter.DimensionPredicateFilter; | ||
|
|
||
| import java.util.HashSet; | ||
| import javax.annotation.Nullable; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
|
|
||
| /** | ||
| */ | ||
|
|
@@ -41,13 +44,17 @@ public class BloomDimFilter implements DimFilter | |
| private final String dimension; | ||
| private final BloomKFilter bloomKFilter; | ||
| private final HashCode hash; | ||
| @Nullable | ||
| private final ExtractionFn extractionFn; | ||
| @Nullable | ||
| private final FilterTuning filterTuning; | ||
|
|
||
| @JsonCreator | ||
| public BloomDimFilter( | ||
| @JsonProperty("dimension") String dimension, | ||
| @JsonProperty("bloomKFilter") BloomKFilterHolder bloomKFilterHolder, | ||
| @JsonProperty("extractionFn") ExtractionFn extractionFn | ||
| @JsonProperty("extractionFn") @Nullable ExtractionFn extractionFn, | ||
| @JsonProperty("filterTuning") @Nullable FilterTuning filterTuning | ||
| ) | ||
| { | ||
| Preconditions.checkArgument(dimension != null, "dimension must not be null"); | ||
|
|
@@ -56,6 +63,13 @@ public BloomDimFilter( | |
| this.bloomKFilter = bloomKFilterHolder.getFilter(); | ||
| this.hash = bloomKFilterHolder.getFilterHash(); | ||
| this.extractionFn = extractionFn; | ||
| this.filterTuning = filterTuning; | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| public BloomDimFilter(String dimension, BloomKFilterHolder bloomKFilterHolder, @Nullable ExtractionFn extractionFn) | ||
| { | ||
| this(dimension, bloomKFilterHolder, extractionFn, null); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -152,7 +166,8 @@ public boolean applyNull() | |
| }; | ||
| } | ||
| }, | ||
| extractionFn | ||
| extractionFn, | ||
| filterTuning | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -168,20 +183,40 @@ public BloomKFilter getBloomKFilter() | |
| return bloomKFilter; | ||
| } | ||
|
|
||
| @Nullable | ||
| @JsonProperty | ||
| public ExtractionFn getExtractionFn() | ||
| { | ||
| return extractionFn; | ||
| } | ||
|
|
||
| @Nullable | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| @JsonProperty | ||
| public FilterTuning getFilterTuning() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method should be |
||
| { | ||
| return filterTuning; | ||
| } | ||
|
|
||
| @Override | ||
| public RangeSet<String> getDimensionRangeSet(String dimension) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public Set<String> getRequiredColumns() | ||
| { | ||
| return ImmutableSet.of(dimension); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add |
||
| { | ||
| if (extractionFn != null) { | ||
| return StringUtils.format("%s(%s) = %s", extractionFn, dimension, hash.toString()); | ||
| } else { | ||
| return StringUtils.format("%s = %s", dimension, hash.toString()); | ||
| } | ||
| return new DimFilterToStringBuilder().appendDimension(dimension, extractionFn) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoa. This is cool. |
||
| .appendEquals(hash.toString()) | ||
| .appendFilterTuning(filterTuning) | ||
| .build(); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -193,36 +228,16 @@ public boolean equals(Object o) | |
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
|
|
||
| BloomDimFilter that = (BloomDimFilter) o; | ||
|
|
||
| if (!dimension.equals(that.dimension)) { | ||
| return false; | ||
| } | ||
| if (hash != null ? !hash.equals(that.hash) : that.hash != null) { | ||
| return false; | ||
| } | ||
| return extractionFn != null ? extractionFn.equals(that.extractionFn) : that.extractionFn == null; | ||
| } | ||
|
|
||
| @Override | ||
| public RangeSet<String> getDimensionRangeSet(String dimension) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public HashSet<String> getRequiredColumns() | ||
| { | ||
| return Sets.newHashSet(dimension); | ||
| return dimension.equals(that.dimension) && | ||
| hash.equals(that.hash) && | ||
| Objects.equals(extractionFn, that.extractionFn) && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Everything about |
||
| Objects.equals(filterTuning, that.filterTuning); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() | ||
| { | ||
| int result = dimension.hashCode(); | ||
| result = 31 * result + (hash != null ? hash.hashCode() : 0); | ||
| result = 31 * result + (extractionFn != null ? extractionFn.hashCode() : 0); | ||
| return result; | ||
| return Objects.hash(dimension, hash, extractionFn, filterTuning); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,7 +100,8 @@ public DimFilter toDruidFilter( | |
| return new BloomDimFilter( | ||
| druidExpression.getSimpleExtraction().getColumn(), | ||
| holder, | ||
| druidExpression.getSimpleExtraction().getExtractionFn() | ||
| druidExpression.getSimpleExtraction().getExtractionFn(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Constructor annotated
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is true, but I was trying to avoid using these constructors in production code to minimize the chance of errors and make things more obvious. I initially had it more like you are suggesting and was using some of the 'test' constructors where we were always passing null, but this comment suggested the way it is now and I agree, I think the production code explicitly passing in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's anyway strange that there is a chaining constructor and any code is not using an existing constructor with fewer parameters. I think it would be better to create static factory methods in this situation. Some of them may have "InTest" suffix to strongly indicate that they are not for prod code.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but the reason I have a second set of constructors at all was so I didn't have to change hundreds of lines of test code to add a I do think this is worth doing, I'll add this to the ticket i create about refactoring
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mentioned making static test object creators to decouple test filters from json schema in #8256 |
||
| null | ||
| ); | ||
| } else if (virtualColumnRegistry != null) { | ||
| VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression( | ||
|
|
@@ -114,6 +115,7 @@ public DimFilter toDruidFilter( | |
| return new BloomDimFilter( | ||
| virtualColumn.getOutputName(), | ||
| holder, | ||
| null, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here. |
||
| null | ||
| ); | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,7 +204,7 @@ public TimeseriesQueryBuilder filters(String dimensionName, String value) | |
|
|
||
| public TimeseriesQueryBuilder filters(String dimensionName, String value, String... values) | ||
| { | ||
| dimFilter = new InDimFilter(dimensionName, Lists.asList(value, values), null); | ||
| dimFilter = new InDimFilter(dimensionName, Lists.asList(value, values), null, null); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could call the old constructor |
||
| return this; | ||
| } | ||
|
|
||
|
|
@@ -361,7 +361,7 @@ public SearchQueryBuilder dataSource(DataSource d) | |
|
|
||
| public SearchQueryBuilder filters(String dimensionName, String value) | ||
| { | ||
| dimFilter = new SelectorDimFilter(dimensionName, value, null); | ||
| dimFilter = new SelectorDimFilter(dimensionName, value, null, null); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could call the old constructor |
||
| return this; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be
@Nullable, along with the corresponding constructor parameterThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've updated all
DimFilterimplementations to have the appropriate things annotated with@Nullable