clintropolis commented on code in PR #15611:
URL: https://github.com/apache/druid/pull/15611#discussion_r1440177543
##########
processing/src/main/java/org/apache/druid/query/lookup/LookupExtractor.java:
##########
@@ -68,37 +72,30 @@ public Map<String, String> applyAll(Iterable<String> keys)
}
/**
- * Provide the reverse mapping from a given value to a list of keys
+ * Reverse lookup from a given value. Used by the default implementation of
{@link #unapplyAll(Set)}.
+ * Otherwise unused. Implementations that override {@link #unapplyAll(Set)}
may throw
+ * {@link UnsupportedOperationException} from this method.
*
- * @param value the value to apply the reverse lookup
+ * @param value the value to apply the reverse lookup. {@link
NullHandling#nullToEmptyIfNeeded(String)} will have
+ * been applied to the value.
*
- * @return the list of keys that maps to value or empty list.
- * Note that for the case of a none existing value in the lookup we have to
cases either return an empty list OR list with null element.
- * returning an empty list implies that user want to ignore such a lookup
value.
- * In the other hand returning a list with the null element implies user
want to map the none existing value to the key null.
- * Null value maps to empty list.
+ * @return the list of keys that maps to the provided value.
*/
-
- public abstract List<String> unapply(@Nullable String value);
+ protected abstract List<String> unapply(@Nullable String value);
/**
- * @param values Iterable of values for which will perform reverse lookup
+ * Reverse lookup from a given set of values.
*
- * @return Returns {@link Map} whose keys are the contents of {@code values}
and whose values are computed on demand using the reverse lookup function
{@link #unapply(String)}
- * or empty map if {@code values} is `null`
- * User can override this method if there is a better way to perform bulk
reverse lookup
+ * @param values set of values to reverse lookup. {@link
NullHandling#nullToEmptyIfNeeded(String)} will have
+ * been applied to each value.
+ *
+ * @return iterator of keys that map to to the provided set of values. May
contain duplicate keys. Returns null if
+ * this lookup instance does not support reverse lookups.
*/
-
- public Map<String, List<String>> unapplyAll(Iterable<String> values)
+ @Nullable
+ public Iterator<String> unapplyAll(Set<String> values)
Review Comment:
At first I was thinking if we are changing this interface, maybe we should
also consider changing `unapply` itself to also spit out an iterator. However,
looking closer since `MapLookupExtractor` overrides this it probably isn't a
super big deal, and I'm not sure the other two implementations could actually
offer much of an improvement if unapply was lazier, I was mostly just thinking
that it is kind of sad this default implementation spits out an iterator but
has to materialize a list for each `unapply` so it can get an iterator.
tl;dr i think this is fine and nothing needs to change
##########
processing/src/main/java/org/apache/druid/query/filter/InDimFilter.java:
##########
@@ -227,22 +228,32 @@ public byte[] getCacheKey()
}
@Override
- public DimFilter optimize()
+ public DimFilter optimize(final boolean mayIncludeUnknown)
{
- InDimFilter inFilter = optimizeLookup();
+ final ExtractionFn newExtractionFn;
+ ValuesSet newValues = optimizeLookup(this, mayIncludeUnknown);
- if (inFilter.values.isEmpty()) {
- return FalseDimFilter.instance();
+ if (newValues == null) {
+ newValues = values;
+ newExtractionFn = extractionFn;
+ } else {
+ newExtractionFn = null;
}
- if (inFilter.values.size() == 1) {
+
+ if (newValues.isEmpty()) {
+ return FalseDimFilter.instance();
+ } else if (newValues.size() == 1) {
return new SelectorDimFilter(
- inFilter.dimension,
- inFilter.values.iterator().next(),
- inFilter.getExtractionFn(),
+ dimension,
+ newValues.iterator().next(),
+ newExtractionFn,
filterTuning
);
+ } else if (newValues == values) {
+ return this;
Review Comment:
should we just bail early with this if extractionFn is null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
