-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Javascript filter with multiple columns #2210
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
dc24bce
cfd185b
98e8c10
a910396
24df1cd
82d873f
55f25d8
0bdf01d
aa0df8a
f0498cc
5ac3d81
4d205b8
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 |
|---|---|---|
|
|
@@ -21,32 +21,36 @@ | |
|
|
||
| import com.fasterxml.jackson.annotation.JsonCreator; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import com.google.common.base.Joiner; | ||
| import com.google.common.base.Preconditions; | ||
| import com.metamx.common.StringUtils; | ||
|
|
||
| import java.nio.ByteBuffer; | ||
| import java.util.Arrays; | ||
|
|
||
| public class JavaScriptDimFilter implements DimFilter | ||
| { | ||
| private final String dimension; | ||
| private final String[] dimensions; | ||
| private final String function; | ||
|
|
||
| @JsonCreator | ||
| public JavaScriptDimFilter( | ||
|
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. How about to make two constructor? But his will need opinions from others because I don't know well on guava.
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. Guice, not guava, I mean.
Contributor
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'm also not familiar with Jackson, |
||
| // for backwards compatibility | ||
| @JsonProperty("dimension") String dimension, | ||
|
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. let's mark the old one only for backwards compatibility
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. let's remove dimension and only have dimensions Also, can we make dimensions a List of String?
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. @fjy Can we ignore backward compatibility?
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. This is going into 0.9.0 with other API changes so I think we can in this case But would be good to get some other feedback here |
||
| @JsonProperty("dimensions") String[] dimensions, | ||
| @JsonProperty("function") String function | ||
| ) | ||
| { | ||
| Preconditions.checkArgument(dimension != null, "dimension must not be null"); | ||
| Preconditions.checkArgument(dimension != null ^ dimensions != null, "dimensions(xor dimension) must not be null"); | ||
| Preconditions.checkArgument(function != null, "function must not be null"); | ||
| this.dimension = dimension; | ||
| this.dimensions = (dimensions != null) ? dimensions : new String[]{dimension}; | ||
| this.function = function; | ||
| } | ||
|
|
||
| @JsonProperty | ||
| public String getDimension() | ||
| public String[] getDimensions() | ||
|
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. i think this may cause serde errors given how Jackson works need a serde check to make sure
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. @sirpkt made a test for it in JavaScriptDimFilterSerDesrTest |
||
| { | ||
| return dimension; | ||
| return dimensions; | ||
| } | ||
|
|
||
| @JsonProperty | ||
|
|
@@ -58,14 +62,22 @@ public String getFunction() | |
| @Override | ||
| public byte[] getCacheKey() | ||
| { | ||
| final byte[] dimensionBytes = StringUtils.toUtf8(dimension); | ||
| final byte[] functionBytes = StringUtils.toUtf8(function); | ||
| byte[][] dimensionsBytes = new byte[dimensions.length][]; | ||
| int totalDimensionsBytes = 0; | ||
|
|
||
| return ByteBuffer.allocate(2 + dimensionBytes.length + functionBytes.length) | ||
| .put(DimFilterCacheHelper.JAVASCRIPT_CACHE_ID) | ||
| .put(dimensionBytes) | ||
| .put(DimFilterCacheHelper.STRING_SEPARATOR) | ||
| .put(functionBytes) | ||
| for (int idx = 0; idx < dimensions.length; idx++) { | ||
| dimensionsBytes[idx] = StringUtils.toUtf8(dimensions[idx]); | ||
| totalDimensionsBytes += dimensionsBytes[idx].length; | ||
| } | ||
|
|
||
| ByteBuffer byteBuffer = ByteBuffer.allocate(2 + dimensions.length + totalDimensionsBytes + functionBytes.length) | ||
| .put(DimFilterCacheHelper.JAVASCRIPT_CACHE_ID); | ||
| for (byte[] dimBytes: dimensionsBytes) { | ||
| byteBuffer.put(dimBytes) | ||
| .put(DimFilterCacheHelper.STRING_SEPARATOR); | ||
| } | ||
| return byteBuffer.put(functionBytes) | ||
| .array(); | ||
| } | ||
|
|
||
|
|
@@ -79,8 +91,26 @@ public DimFilter optimize() | |
| public String toString() | ||
| { | ||
| return "JavaScriptDimFilter{" + | ||
| "dimension='" + dimension + '\'' + | ||
| "dimensions=['" + Joiner.on("', '").join(dimensions) + "']" + | ||
| ", function='" + function + '\'' + | ||
| '}'; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) | ||
| { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof JavaScriptDimFilter)) { | ||
| return false; | ||
| } | ||
|
|
||
| JavaScriptDimFilter that = (JavaScriptDimFilter) o; | ||
|
|
||
| if (!function.equals(that.function)) { | ||
| return false; | ||
| } | ||
| return Arrays.equals(that.dimensions, dimensions); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,6 @@ | |
| import com.google.common.base.Preconditions; | ||
| import com.google.common.base.Predicate; | ||
| import com.metamx.collections.bitmap.ImmutableBitmap; | ||
| import com.metamx.common.guava.FunctionalIterable; | ||
| import io.druid.query.filter.BitmapIndexSelector; | ||
| import io.druid.query.filter.Filter; | ||
| import io.druid.query.filter.ValueMatcher; | ||
|
|
@@ -33,14 +32,14 @@ | |
| import org.mozilla.javascript.Function; | ||
| import org.mozilla.javascript.ScriptableObject; | ||
|
|
||
| import javax.annotation.Nullable; | ||
| import java.util.Iterator; | ||
|
|
||
| public class JavaScriptFilter implements Filter | ||
| { | ||
| private final JavaScriptPredicate predicate; | ||
| private final String dimension; | ||
| private final String[] dimension; | ||
|
|
||
| public JavaScriptFilter(String dimension, final String script) | ||
| public JavaScriptFilter(String[] dimension, final String script) | ||
| { | ||
| this.dimension = dimension; | ||
| this.predicate = new JavaScriptPredicate(script); | ||
|
|
@@ -51,34 +50,57 @@ public ImmutableBitmap getBitmapIndex(final BitmapIndexSelector selector) | |
| { | ||
| final Context cx = Context.enter(); | ||
| try { | ||
| final Indexed<String> dimValues = selector.getDimensionValues(dimension); | ||
| ImmutableBitmap bitmap; | ||
| if (dimValues == null) { | ||
| bitmap = selector.getBitmapFactory().makeEmptyImmutableBitmap(); | ||
| } else { | ||
| bitmap = selector.getBitmapFactory().union( | ||
| FunctionalIterable.create(dimValues) | ||
| .filter( | ||
| new Predicate<String>() | ||
| { | ||
| @Override | ||
| public boolean apply(@Nullable String input) | ||
| { | ||
| return predicate.applyInContext(cx, input); | ||
| } | ||
| } | ||
| ) | ||
| .transform( | ||
| new com.google.common.base.Function<String, ImmutableBitmap>() | ||
| { | ||
| @Override | ||
| public ImmutableBitmap apply(@Nullable String input) | ||
| { | ||
| return selector.getBitmapIndex(dimension, input); | ||
| } | ||
| } | ||
| ) | ||
| ); | ||
|
|
||
| boolean hasEmptyDimension = false; | ||
| Indexed<String> [] dimValuesList = new Indexed[dimension.length]; | ||
| Iterator<String> [] dimValuesIterator = new Iterator[dimension.length]; | ||
| String[] currentDim = new String[dimension.length]; | ||
| for(int idx = 0; idx < dimension.length; idx++) { | ||
| dimValuesList[idx] = selector.getDimensionValues(dimension[idx]); | ||
| if (dimValuesList[idx].size() == 0) { | ||
| hasEmptyDimension = true; | ||
| break; | ||
| } | ||
| dimValuesIterator[idx] = dimValuesList[idx].iterator(); | ||
| if (idx != 0) { | ||
| currentDim[idx] = dimValuesIterator[idx].next(); | ||
| } | ||
| } | ||
|
|
||
| bitmap = selector.getBitmapFactory().makeEmptyImmutableBitmap(); | ||
| if (!hasEmptyDimension) { | ||
| int iteratingIndex = 0; | ||
| while(true) { | ||
| // advance iterator | ||
| Iterator<String> iterator = dimValuesIterator[iteratingIndex]; | ||
| if (iterator.hasNext()) { | ||
| currentDim[iteratingIndex] = iterator.next(); | ||
| if (iteratingIndex > 0) { | ||
| // reset inner loop iterators | ||
| for (int idx = 0; idx < iteratingIndex; idx++) { | ||
| dimValuesIterator[idx] = dimValuesList[idx].iterator(); | ||
| currentDim[idx] = dimValuesIterator[idx].next(); | ||
| } | ||
| // move to the most inner loop | ||
| iteratingIndex = 0; | ||
| } | ||
| if (predicate.applyInContext(cx, currentDim)) | ||
|
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. this should be called only when iteratingIndex == 0.
Contributor
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. @navis you're right. I'll fix it. |
||
| { | ||
| // update bitmap | ||
| ImmutableBitmap overlap = null; | ||
| for (int idx = 0; idx < dimension.length; idx++) { | ||
| ImmutableBitmap dimBitMap = selector.getBitmapIndex(dimension[idx], currentDim[idx]); | ||
| overlap = (overlap == null) ? dimBitMap : overlap.intersection(dimBitMap); | ||
| } | ||
| bitmap = bitmap.union(overlap); | ||
| } | ||
|
|
||
| } else { | ||
| iteratingIndex++; | ||
| if (iteratingIndex == dimension.length) break; | ||
| } | ||
| } | ||
| } | ||
| return bitmap; | ||
| } | ||
|
|
@@ -94,7 +116,7 @@ public ValueMatcher makeMatcher(ValueMatcherFactory factory) | |
| return factory.makeValueMatcher(dimension, predicate); | ||
| } | ||
|
|
||
| static class JavaScriptPredicate implements Predicate<String> | ||
| static class JavaScriptPredicate implements Predicate<String[]> | ||
| { | ||
| final ScriptableObject scope; | ||
| final Function fnApply; | ||
|
|
@@ -118,7 +140,7 @@ public JavaScriptPredicate(final String script) | |
| } | ||
|
|
||
| @Override | ||
| public boolean apply(final String input) | ||
| public boolean apply(final String[] input) | ||
| { | ||
| // one and only one context per thread | ||
| final Context cx = Context.enter(); | ||
|
|
@@ -131,9 +153,9 @@ public boolean apply(final String input) | |
|
|
||
| } | ||
|
|
||
| public boolean applyInContext(Context cx, String input) | ||
| public boolean applyInContext(Context cx, String[] input) | ||
| { | ||
| return Context.toBoolean(fnApply.call(cx, scope, scope, new String[]{input})); | ||
| return Context.toBoolean(fnApply.call(cx, scope, scope, input)); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
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.
we should remove the old form from the documentation and only maintain it for backwards compatibility