-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Promoting LookupExtractor state and LookupExtractorFactory to be a first class druid state object. #2291
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
Promoting LookupExtractor state and LookupExtractorFactory to be a first class druid state object. #2291
Changes from all commits
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 |
|---|---|---|
|
|
@@ -87,4 +87,5 @@ public List<String> unapply(@NotNull String value) | |
| { | ||
| return reverseExtractionFunction.apply(value); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,235 @@ | ||
| /* | ||
| * Licensed to Metamarkets Group Inc. (Metamarkets) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. Metamarkets licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package io.druid.query.dimension; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JacksonInject; | ||
| import com.fasterxml.jackson.annotation.JsonCreator; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import com.google.common.base.Preconditions; | ||
| import com.google.common.base.Strings; | ||
| import com.metamx.common.StringUtils; | ||
| import io.druid.query.extraction.ExtractionFn; | ||
| import io.druid.query.extraction.LookupExtractionFn; | ||
| import io.druid.query.extraction.LookupExtractor; | ||
| import io.druid.query.extraction.LookupReferencesManager; | ||
| import io.druid.query.filter.DimFilterCacheHelper; | ||
| import io.druid.segment.DimensionSelector; | ||
|
|
||
| import javax.annotation.Nullable; | ||
| import java.nio.ByteBuffer; | ||
|
|
||
| public class LookupDimensionSpec implements DimensionSpec | ||
| { | ||
| private static final byte CACHE_TYPE_ID = 0x4; | ||
|
|
||
| @JsonProperty | ||
| private final String dimension; | ||
|
|
||
| @JsonProperty | ||
| private final String outputName; | ||
|
|
||
| @JsonProperty | ||
| private final LookupExtractor lookup; | ||
|
|
||
| @JsonProperty | ||
| private final boolean retainMissingValue; | ||
|
|
||
| @JsonProperty | ||
| private final String replaceMissingValueWith; | ||
|
|
||
| @JsonProperty | ||
| private final String name; | ||
|
|
||
| @JsonProperty | ||
| private final boolean optimize; | ||
|
|
||
| private final LookupReferencesManager lookupReferencesManager; | ||
|
|
||
| @JsonCreator | ||
| public LookupDimensionSpec( | ||
| @JsonProperty("dimension") String dimension, | ||
| @JsonProperty("outputName") String outputName, | ||
| @JsonProperty("lookup") LookupExtractor lookup, | ||
| @JsonProperty("retainMissingValue") boolean retainMissingValue, | ||
| @JsonProperty("replaceMissingValueWith") String replaceMissingValueWith, | ||
| @JsonProperty("name") String name, | ||
| @JacksonInject LookupReferencesManager lookupReferencesManager, | ||
| @JsonProperty("optimize") Boolean optimize | ||
|
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 is missing from hashcode and equals
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. fixed |
||
| ) | ||
| { | ||
| this.retainMissingValue = retainMissingValue; | ||
| this.optimize = optimize == null ? true : optimize; | ||
| this.replaceMissingValueWith = Strings.emptyToNull(replaceMissingValueWith); | ||
| this.dimension = Preconditions.checkNotNull(dimension, "dimension can not be Null"); | ||
| this.outputName = Preconditions.checkNotNull(outputName, "outputName can not be Null"); | ||
| this.lookupReferencesManager = lookupReferencesManager; | ||
| this.name = name; | ||
| this.lookup = lookup; | ||
| Preconditions.checkArgument( | ||
| Strings.isNullOrEmpty(name) ^ (lookup == null), | ||
| "name [%s] and lookup [%s] are mutually exclusive please provide either a name or a lookup", name, lookup | ||
| ); | ||
|
|
||
| if (!Strings.isNullOrEmpty(name)) { | ||
| Preconditions.checkNotNull( | ||
| this.lookupReferencesManager, | ||
| "The system is not configured to allow for lookups, please read about configuring a lookup manager in the docs" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| @JsonProperty | ||
| public String getDimension() | ||
| { | ||
| return dimension; | ||
| } | ||
|
|
||
| @Override | ||
| @JsonProperty | ||
| public String getOutputName() | ||
| { | ||
| return outputName; | ||
| } | ||
|
|
||
| @JsonProperty | ||
| @Nullable | ||
| public LookupExtractor getLookup() | ||
| { | ||
| return lookup; | ||
| } | ||
|
|
||
| @JsonProperty | ||
| @Nullable | ||
| public String getName() | ||
| { | ||
| return name; | ||
| } | ||
|
|
||
| @Override | ||
| public ExtractionFn getExtractionFn() | ||
| { | ||
| final LookupExtractor lookupExtractor = Strings.isNullOrEmpty(name) | ||
| ? this.lookup | ||
| : Preconditions.checkNotNull( | ||
| this.lookupReferencesManager.get(name).get(), | ||
| "can not find lookup with name [%s]", | ||
| name | ||
| ); | ||
| return new LookupExtractionFn( | ||
| lookupExtractor, | ||
| retainMissingValue, | ||
| replaceMissingValueWith, | ||
| lookupExtractor.isOneToOne(), | ||
| optimize | ||
| ); | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public DimensionSelector decorate(DimensionSelector selector) | ||
| { | ||
| return selector; | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] getCacheKey() | ||
| { | ||
| byte[] dimensionBytes = StringUtils.toUtf8(dimension); | ||
| byte[] dimExtractionFnBytes = Strings.isNullOrEmpty(name) | ||
| ? getLookup().getCacheKey() | ||
| : StringUtils.toUtf8(name); | ||
| byte[] outputNameBytes = StringUtils.toUtf8(outputName); | ||
| byte[] replaceWithBytes = StringUtils.toUtf8(Strings.nullToEmpty(replaceMissingValueWith)); | ||
|
|
||
|
|
||
| return ByteBuffer.allocate(6 | ||
| + dimensionBytes.length | ||
| + outputNameBytes.length | ||
| + dimExtractionFnBytes.length | ||
| + replaceWithBytes.length) | ||
| .put(CACHE_TYPE_ID) | ||
| .put(dimensionBytes) | ||
| .put(DimFilterCacheHelper.STRING_SEPARATOR) | ||
| .put(outputNameBytes) | ||
| .put(DimFilterCacheHelper.STRING_SEPARATOR) | ||
| .put(dimExtractionFnBytes) | ||
| .put(DimFilterCacheHelper.STRING_SEPARATOR) | ||
| .put(replaceWithBytes) | ||
| .put(DimFilterCacheHelper.STRING_SEPARATOR) | ||
| .put(retainMissingValue == true ? (byte) 1 : (byte) 0) | ||
| .array(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean preservesOrdering() | ||
| { | ||
| return getExtractionFn().preservesOrdering(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) | ||
| { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof LookupDimensionSpec)) { | ||
| return false; | ||
| } | ||
|
|
||
| LookupDimensionSpec that = (LookupDimensionSpec) o; | ||
|
|
||
| if (retainMissingValue != that.retainMissingValue) { | ||
| return false; | ||
| } | ||
| if (optimize != that.optimize) { | ||
| return false; | ||
| } | ||
| if (!getDimension().equals(that.getDimension())) { | ||
| return false; | ||
| } | ||
| if (!getOutputName().equals(that.getOutputName())) { | ||
| return false; | ||
| } | ||
| if (getLookup() != null ? !getLookup().equals(that.getLookup()) : that.getLookup() != null) { | ||
| return false; | ||
| } | ||
| if (replaceMissingValueWith != null | ||
| ? !replaceMissingValueWith.equals(that.replaceMissingValueWith) | ||
| : that.replaceMissingValueWith != null) { | ||
| return false; | ||
| } | ||
| return getName() != null ? getName().equals(that.getName()) : that.getName() == null; | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() | ||
| { | ||
| int result = getDimension().hashCode(); | ||
| result = 31 * result + getOutputName().hashCode(); | ||
| result = 31 * result + (getLookup() != null ? getLookup().hashCode() : 0); | ||
| result = 31 * result + (retainMissingValue ? 1 : 0); | ||
| result = 31 * result + (replaceMissingValueWith != null ? replaceMissingValueWith.hashCode() : 0); | ||
| result = 31 * result + (getName() != null ? getName().hashCode() : 0); | ||
| result = 31 * result + (optimize ? 1 : 0); | ||
| return result; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,10 +65,6 @@ public DimensionSelector decorate(final DimensionSelector selector) | |
| return selector; | ||
| } | ||
|
|
||
| if (selector == null) { | ||
| return selector; | ||
| } | ||
|
|
||
| int count = 0; | ||
|
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. why was this removed?
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. it is not needed
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. You can be more clear as in : Code is duplicated immediately prior to here. The duplicate code is hidden in the fold |
||
| final Map<Integer,Integer> forwardMapping = new HashMap<>(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,11 @@ public Map<String, List<String>> unapplyAll(Iterable<String> values) | |
| * @return A byte array that can be used to uniquely identify if results of a prior lookup can use the cached values | ||
| */ | ||
|
|
||
| @Nullable | ||
| public abstract byte[] getCacheKey(); | ||
|
|
||
| // make this abstract again once @drcrallen fix the metmax lookup implementation. | ||
| public boolean isOneToOne() | ||
| { | ||
| return false; | ||
|
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. why is this hard coded to false?
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. it is a default implementation, to avoid breaking up your experimental implementation of lookupExtractor.
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. @drcrallen i dropped a comment
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. ((as per conversation)) suggest adding docs to indicate when and if this should be removed or changed in the future. |
||
| } | ||
| } | ||
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.
this is an incompatible change for configs that had relied on defaulting to false. Can you explain more on why this won't impact configs that were relying on that behavior?
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.
@drcrallen lookups is an experimental feature, so changes like that are expected to happen.
I have set this to true, after had been tested.
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.
LookupExtractor is not listed as experimental, and neither is the "optimize" flag (as far as I can tell).
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.
Looks like it was added in 032d3bf which is in 0.9.0 As such it can change just fine, but the default for an experimental feature should be legacy behavior.
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.
well... it can be changed in 0.9.0 (which is not yet released). Does the default behavior need to change before we release 0.9.0?