Skip to content

Directly rewrite filters on RHS join columns into LHS equivalents #9818

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

Merged
jon-wei merged 7 commits into from
May 9, 2020
Merged

Directly rewrite filters on RHS join columns into LHS equivalents #9818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
937f268
Directly rewrite filters on RHS join columns into LHS equivalents
jon-wei May 1, 2020
c24e612
PR comments
jon-wei May 5, 2020
64bfecb
Fix inspection
jon-wei May 6, 2020
d82c4f9
Revert unnecessary ExprMacroTable change
jon-wei May 6, 2020
c312e05
Merge remote-tracking branch 'upstream/master' into optimize_join_filter
jon-wei May 9, 2020
cce3497
Fix build after merge
jon-wei May 9, 2020
87ad569
Address PR comments
jon-wei May 9, 2020
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
26 changes: 26 additions & 0 deletions processing/src/main/java/org/apache/druid/query/filter/Filter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import java.util.Map;
import java.util.Set;

public interface Filter
Expand Down Expand Up @@ -162,4 +163,29 @@ default boolean canVectorizeMatcher()
* can be expected to have a bitmap index retrievable via {@link BitmapIndexSelector#getBitmapIndex(String)}
*/
Set<String> getRequiredColumns();

/**
* Returns true is this filter is able to return a copy of this filter that is identical to this filter except that it
* operates on different columns, based on a renaming map.
*/
default boolean supportsRequiredColumnRewrite()
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s May 4, 2020

Choose a reason for hiding this comment

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

I think we should add a test that looks for all implementations of Filter and then validate whatsupportsRequiredColumnRewrite() returns.

Since the default is false, we should be explicit in the list of filters that we say do not support re-writes. This way, if someone adds a filter in the future, this test will fail and force the dev to think about what the correct implementation should be.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson May 9, 2020

Choose a reason for hiding this comment

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

Would you elaborate more details of the test you think? I'm not sure how it could help. Since the default returns false, the new filter overrode this method to return true if it had to be. The author should be able to think what it means when it is overridden. If you mean new filters need a design review, that should be done during the design phase rather than testing phase.

Copy link
Copy Markdown
Contributor

@suneet-s suneet-s May 9, 2020

Choose a reason for hiding this comment

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

I think that can be difficult for the author and the reviewer. There are many functions in an interface, and most editors will pull in methods that don't have a definition unless you explicitly tell them to. As a reviewer, nothing is warning me that the implementation/ design did not consider implementing this function.

I have a WIP change that does something similar to validate that the equals and hashCode implementations are not using the implementation provided by Object I suspect we'd want something very similar, except with the ability to provide exceptions. https://github.com/suneet-s/druid/blob/b8bc17551361b20458b073c62b5fe8f5d6a85183/processing/src/test/java/org/apache/druid/query/filter/FilterTest.java#L33

This way, the dev has to add the exception as part of the PR for the tests to pass, and the reviewer can see that an exception was added and ask themselves if that exception makes sense.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson May 9, 2020

Choose a reason for hiding this comment

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

Oh thanks, I see what you are saying. But do we need that kind of tests? If I understand correctly, this interface is for temporarily marking what filters support the rewrite for now since some of filters don't implement it. This interface will be eventually removed in the future. I don't see any reason to not support the rewrite for some particular filter types.

{
return false;
}

/**
* Return a copy of this filter that is identical to the this filter except that it operates on different columns,
* based on a renaming map where the key is the column to be renamed in the filter, and the value is the new
* column name.
*
* For example, if I have a filter (A = hello), and I have a renaming map (A -> B),
* this should return the filter (B = hello)
*
* @param columnRewrites Column rewrite map
* @return Copy of this filter that operates on new columns based on the rewrite map
*/
default Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson May 9, 2020

Choose a reason for hiding this comment

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

Does this interface take a map for future expansion even though all callers pass a map containing only one pair of key and value?

Copy link
Copy Markdown
Contributor Author

@jon-wei jon-wei May 9, 2020

Choose a reason for hiding this comment

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

Yes, that was the intent

{
throw new UnsupportedOperationException("Required column rewrite is not supported by this filter.");
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s May 4, 2020

Choose a reason for hiding this comment

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

note to self: re-think the interface here. Does this need to be de-coupled from the function above? Does this introduce more chances of developer error that's only detected at runtime? Is there a pattern that guarantees that this is only called if supportsRequiredColumnRewrite is true?

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson May 9, 2020

Choose a reason for hiding this comment

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

Good question. A potential alternative could be having just this interface and its default implementation returns this. I haven't thought enough to say what is better yet.

}
}
21 changes: 21 additions & 0 deletions processing/src/main/java/org/apache/druid/query/filter/LikeDimFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,5 +363,26 @@ public SuffixMatch getSuffixMatch()
{
return suffixMatch;
}

Copy link
Copy Markdown
Contributor

@suneet-s suneet-s May 4, 2020

Choose a reason for hiding this comment

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

Why can't a like dim filter be re-written?

Copy link
Copy Markdown
Contributor Author

@jon-wei jon-wei May 5, 2020

Choose a reason for hiding this comment

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

Like filters can, the new interface is on Filter, not DimFilter

@Override
public boolean equals(Object o)
{
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
LikeMatcher that = (LikeMatcher) o;
return getSuffixMatch() == that.getSuffixMatch() &&
Objects.equals(getPrefix(), that.getPrefix()) &&
Objects.equals(pattern.toString(), that.pattern.toString());
}

@Override
public int hashCode()
{
return Objects.hash(getSuffixMatch(), getPrefix(), pattern.toString());
}
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s May 4, 2020

Choose a reason for hiding this comment

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

Missing EqualsVerifierTest for this LikeDimFilter.

Thanks for this adding an equals test for LikeMatcher 👍

Copy link
Copy Markdown
Contributor Author

@jon-wei jon-wei May 5, 2020

Choose a reason for hiding this comment

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

Hm, it doesn't look like we have EqualsVerifierTest for any of the DimFilter implementations; since this patch is only adding equals implementations for Filter classes, I think that should be done in a separate PR

}
}
35 changes: 35 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/BoundFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import it.unimi.dsi.fastutil.ints.IntList;
import org.apache.druid.collections.bitmap.ImmutableBitmap;
import org.apache.druid.common.config.NullHandling;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.Pair;
import org.apache.druid.query.BitmapResultFactory;
import org.apache.druid.query.extraction.ExtractionFn;
Expand All @@ -47,6 +48,7 @@
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import java.util.Comparator;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

Expand Down Expand Up @@ -172,6 +174,39 @@ public Set<String> getRequiredColumns()
return boundDimFilter.getRequiredColumns();
}

@Override
public boolean supportsRequiredColumnRewrite()
{
return true;
}

@Override
public Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
{
String rewriteDimensionTo = columnRewrites.get(boundDimFilter.getDimension());

if (rewriteDimensionTo == null) {
throw new IAE(
"Received a non-applicable rewrite: %s, filter's dimension: %s",
columnRewrites,
boundDimFilter.getDimension()
);
}
BoundDimFilter newDimFilter = new BoundDimFilter(
rewriteDimensionTo,
boundDimFilter.getLower(),
boundDimFilter.getUpper(),
boundDimFilter.isLowerStrict(),
boundDimFilter.isUpperStrict(),
null,
boundDimFilter.getExtractionFn(),
boundDimFilter.getOrdering()
);
return new BoundFilter(
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s May 4, 2020

Choose a reason for hiding this comment

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

note to self: What is the overhead of creating a new object for the filter?

newDimFilter
);
}

private static Pair<Integer, Integer> getStartEndIndexes(
final BoundDimFilter boundDimFilter,
final BitmapIndex bitmapIndex
Expand Down
33 changes: 28 additions & 5 deletions processing/src/main/java/org/apache/druid/segment/filter/DimensionPredicateFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,18 @@
import org.apache.druid.segment.DimensionHandlerUtils;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import java.util.Objects;
import java.util.Set;

/**
*/
public class DimensionPredicateFilter implements Filter
{
private final String dimension;
private final DruidPredicateFactory predicateFactory;
private final String basePredicateString;
private final ExtractionFn extractionFn;
private final FilterTuning filterTuning;
protected final String dimension;
protected final DruidPredicateFactory predicateFactory;
protected final String basePredicateString;
protected final ExtractionFn extractionFn;
protected final FilterTuning filterTuning;

public DimensionPredicateFilter(
final String dimension,
Expand Down Expand Up @@ -218,4 +219,26 @@ public String toString()
return StringUtils.format("%s = %s", dimension, basePredicateString);
}
}

@Override
public boolean equals(Object o)
{
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
DimensionPredicateFilter that = (DimensionPredicateFilter) o;
return Objects.equals(dimension, that.dimension) &&
Objects.equals(basePredicateString, that.basePredicateString) &&
Objects.equals(extractionFn, that.extractionFn) &&
Objects.equals(filterTuning, that.filterTuning);
}

@Override
public int hashCode()
{
return Objects.hash(dimension, basePredicateString, extractionFn, filterTuning);
}
}
7 changes: 7 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/ExpressionFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,11 @@ public Set<String> getRequiredColumns()
{
return requiredBindings.get();
}

@Override
public boolean supportsRequiredColumnRewrite()
{
// We could support this, but need a good approach to rewriting the identifiers within an expression.
return false;
}
}
13 changes: 13 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/FalseFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import java.util.Collections;
import java.util.Map;
import java.util.Set;

public class FalseFilter implements Filter
Expand Down Expand Up @@ -99,6 +100,18 @@ public Set<String> getRequiredColumns()
return Collections.emptySet();
}

@Override
public boolean supportsRequiredColumnRewrite()
{
return true;
}

@Override
public Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
{
return this;
}

@Override
public String toString()
{
Expand Down
23 changes: 23 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/Filters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,4 +498,27 @@ public static Filter and(List<Filter> filterList)

return new AndFilter(filterList);
}

/**
* Create a filter representing an OR relationship across a set of filters.
*
* @param filterSet Set of filters
*
* @return If filterSet has more than one element, return an OR filter composed of the filters from filterSet
* If filterSet has a single element, return that element alone
* If filterSet is empty, return null
*/
@Nullable
public static Filter or(Set<Filter> filterSet)
{
if (filterSet.isEmpty()) {
return null;
}

if (filterSet.size() == 1) {
return filterSet.iterator().next();
}

return new OrFilter(filterSet);
}
}
27 changes: 27 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/InFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import it.unimi.dsi.fastutil.ints.IntIterable;
import it.unimi.dsi.fastutil.ints.IntIterator;
import org.apache.druid.collections.bitmap.ImmutableBitmap;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.query.BitmapResultFactory;
import org.apache.druid.query.extraction.ExtractionFn;
import org.apache.druid.query.filter.BitmapIndexSelector;
Expand All @@ -45,6 +46,7 @@
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import java.util.Iterator;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

Expand Down Expand Up @@ -184,6 +186,31 @@ public Set<String> getRequiredColumns()
return ImmutableSet.of(dimension);
}

@Override
public boolean supportsRequiredColumnRewrite()
{
return true;
}

@Override
public Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
{
String rewriteDimensionTo = columnRewrites.get(dimension);
if (rewriteDimensionTo == null) {
throw new IAE("Received a non-applicable rewrite: %s, filter's dimension: %s", columnRewrites, dimension);
}

return new InFilter(
rewriteDimensionTo,
values,
longPredicateSupplier,
floatPredicateSupplier,
doublePredicateSupplier,
extractionFn,
filterTuning
);
}

@Override
public boolean supportsBitmapIndex(BitmapIndexSelector selector)
{
Expand Down
52 changes: 52 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/LikeFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import it.unimi.dsi.fastutil.ints.IntIterator;
import org.apache.druid.collections.bitmap.ImmutableBitmap;
import org.apache.druid.common.config.NullHandling;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.query.BitmapResultFactory;
import org.apache.druid.query.extraction.ExtractionFn;
import org.apache.druid.query.filter.BitmapIndexSelector;
Expand All @@ -44,7 +45,9 @@

import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.Set;

public class LikeFilter implements Filter
Expand Down Expand Up @@ -107,6 +110,33 @@ public Set<String> getRequiredColumns()
return ImmutableSet.of(dimension);
}

@Override
public boolean supportsRequiredColumnRewrite()
{
return true;
}

@Override
public Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
{
String rewriteDimensionTo = columnRewrites.get(dimension);

if (rewriteDimensionTo == null) {
throw new IAE(
"Received a non-applicable rewrite: %s, filter's dimension: %s",
columnRewrites,
dimension
);
}

return new LikeFilter(
rewriteDimensionTo,
extractionFn,
likeMatcher,
filterTuning
);
}

@Override
public boolean supportsBitmapIndex(BitmapIndexSelector selector)
{
Expand Down Expand Up @@ -253,4 +283,26 @@ public int nextInt()
}
};
}

@Override
public boolean equals(Object o)
{
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
LikeFilter that = (LikeFilter) o;
return Objects.equals(dimension, that.dimension) &&
Objects.equals(extractionFn, that.extractionFn) &&
Objects.equals(likeMatcher, that.likeMatcher) &&
Objects.equals(filterTuning, that.filterTuning);
}

@Override
public int hashCode()
{
return Objects.hash(dimension, extractionFn, likeMatcher, filterTuning);
}
}
13 changes: 13 additions & 0 deletions processing/src/main/java/org/apache/druid/segment/filter/NotFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.druid.segment.ColumnSelectorFactory;
import org.apache.druid.segment.vector.VectorColumnSelectorFactory;

import java.util.Map;
import java.util.Objects;
import java.util.Set;

Expand Down Expand Up @@ -111,6 +112,18 @@ public Set<String> getRequiredColumns()
return baseFilter.getRequiredColumns();
}

@Override
public boolean supportsRequiredColumnRewrite()
{
return baseFilter.supportsRequiredColumnRewrite();
}

@Override
public Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
{
return new NotFilter(baseFilter.rewriteRequiredColumns(columnRewrites));
}

@Override
public boolean supportsBitmapIndex(BitmapIndexSelector selector)
{
Expand Down
Loading