diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupBenchmarkUtil.java b/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupBenchmarkUtil.java index 780e58296d1d..95957fdd3bda 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupBenchmarkUtil.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupBenchmarkUtil.java @@ -23,7 +23,9 @@ import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.Pair; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.query.extraction.MapLookupExtractor; +import org.apache.druid.query.lookup.ImmutableLookupMap; import org.apache.druid.query.lookup.LookupExtractor; import java.util.HashMap; @@ -35,6 +37,11 @@ */ public class LookupBenchmarkUtil { + /** + * Length of keys. They are zero-padded to this size. + */ + private static final int KEY_LENGTH = 20; + public enum LookupType { HASHMAP { @@ -69,6 +76,17 @@ public LookupExtractor build(Iterable> keyValuePairs) } return new MapLookupExtractor(map, false); } + }, + REVERSIBLE { + @Override + public LookupExtractor build(Iterable> keyValuePairs) + { + final Map map = new HashMap<>(); + for (final Pair keyValuePair : keyValuePairs) { + map.put(keyValuePair.lhs, keyValuePair.rhs); + } + return ImmutableLookupMap.fromMap(map).asLookupExtractor(false, () -> new byte[0]); + } }; public abstract LookupExtractor build(Iterable> keyValuePairs); @@ -91,9 +109,14 @@ public static LookupExtractor makeLookupExtractor(final LookupType lookupType, f final Iterable> keys = () -> IntStream.range(0, numKeys) - .mapToObj(i -> Pair.of(String.valueOf(i), String.valueOf(i % numValues))) + .mapToObj(i -> Pair.of(makeKeyOrValue(i), makeKeyOrValue(i % numValues))) .iterator(); return lookupType.build(keys); } + + public static String makeKeyOrValue(final int i) + { + return StringUtils.format("%0" + KEY_LENGTH + "d", i); + } } diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupExtractorBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupExtractorBenchmark.java index ddc57dfc0d34..4f9e7f140979 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupExtractorBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/lookup/LookupExtractorBenchmark.java @@ -20,7 +20,9 @@ package org.apache.druid.benchmark.lookup; import com.google.common.base.Preconditions; +import com.google.common.collect.Iterators; import org.apache.druid.common.config.NullHandling; +import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.query.lookup.LookupExtractor; import org.openjdk.jmh.annotations.Benchmark; @@ -58,7 +60,7 @@ public class LookupExtractorBenchmark /** * Type of lookup to benchmark. All are members of enum {@link LookupBenchmarkUtil.LookupType}. */ - @Param({"hashmap", "guava", "fastutil"}) + @Param({"reversible"}) private String lookupType; /** @@ -74,6 +76,7 @@ public class LookupExtractorBenchmark private int keysPerValue; private LookupExtractor lookup; + private String oneValue; private Set oneThousandValues; @Setup(Level.Trial) @@ -86,12 +89,15 @@ public void setup() numValues ); - Preconditions.checkArgument(lookup.keySet().size() == numKeys); + Preconditions.checkArgument(lookup.asMap().size() == numKeys); + + // Values to unapply for the benchmark lookupUnapplyOne. + oneValue = LookupBenchmarkUtil.makeKeyOrValue(0); // Set of values to unapply for the benchmark lookupUnapplyOneThousand. oneThousandValues = new HashSet<>(); for (int i = 0; i < 1000; i++) { - oneThousandValues.add(String.valueOf(i)); + oneThousandValues.add(LookupBenchmarkUtil.makeKeyOrValue(i)); } } @@ -105,17 +111,23 @@ public void lookupApply(Blackhole blackhole) @Benchmark @BenchmarkMode(Mode.AverageTime) - @OutputTimeUnit(TimeUnit.MICROSECONDS) - public void lookupUnapplyOne(Blackhole blackhole) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + public void lookupUnapplyOne() { - blackhole.consume(lookup.unapplyAll(Collections.singleton("0"))); + final int numKeys = Iterators.size(lookup.unapplyAll(Collections.singleton(oneValue))); + if (numKeys != keysPerValue) { + throw new ISE("Expected [%s] keys, got[%s]", keysPerValue, numKeys); + } } @Benchmark @BenchmarkMode(Mode.AverageTime) - @OutputTimeUnit(TimeUnit.MICROSECONDS) - public void lookupUnapplyOneThousand(Blackhole blackhole) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + public void lookupUnapplyOneThousand() { - blackhole.consume(lookup.unapplyAll(oneThousandValues)); + final int numKeys = Iterators.size(lookup.unapplyAll(oneThousandValues)); + if (numKeys != keysPerValue * 1000) { + throw new ISE("Expected [%s] keys, got[%s]", keysPerValue * 1000, numKeys); + } } } diff --git a/extensions-core/lookups-cached-global/pom.xml b/extensions-core/lookups-cached-global/pom.xml index 7ed1de772fd5..92e81b1329d7 100644 --- a/extensions-core/lookups-cached-global/pom.xml +++ b/extensions-core/lookups-cached-global/pom.xml @@ -49,6 +49,11 @@ org.mapdb mapdb + + it.unimi.dsi + fastutil-core + provided + com.google.code.findbugs jsr305 diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactory.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactory.java index ce3121679ad8..40621cde3cb0 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactory.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactory.java @@ -28,18 +28,17 @@ import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.logger.Logger; -import org.apache.druid.query.extraction.MapLookupExtractor; import org.apache.druid.query.lookup.namespace.ExtractionNamespace; import org.apache.druid.server.lookup.namespace.cache.CacheScheduler; import javax.annotation.Nullable; import java.nio.ByteBuffer; -import java.util.Map; import java.util.UUID; import java.util.concurrent.TimeoutException; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; @JsonTypeName("cachedNamespace") public class NamespaceLookupExtractorFactory implements LookupExtractorFactory @@ -225,23 +224,18 @@ public LookupExtractor get() throw new ISE("%s: %s, extractorID = %s", entry, noCacheReason, extractorID); } CacheScheduler.VersionedCache versionedCache = (CacheScheduler.VersionedCache) cacheState; - Map map = versionedCache.getCache(); final byte[] v = StringUtils.toUtf8(versionedCache.getVersion()); final byte[] id = StringUtils.toUtf8(extractorID); - return new MapLookupExtractor(map, isInjective()) - { - @Override - public byte[] getCacheKey() - { - return ByteBuffer + final byte injectiveByte = isInjective() ? (byte) 1 : (byte) 0; + final Supplier cacheKey = () -> + ByteBuffer .allocate(CLASS_CACHE_KEY.length + id.length + 1 + v.length + 1 + 1) .put(CLASS_CACHE_KEY) .put(id).put((byte) 0xFF) .put(v).put((byte) 0xFF) - .put(isOneToOne() ? (byte) 1 : (byte) 0) + .put(injectiveByte) .array(); - } - }; + return versionedCache.asLookupExtractor(isInjective(), cacheKey); } finally { readLock.unlock(); diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupIntrospectHandler.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupIntrospectHandler.java index 7508f16b674c..889467324da3 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupIntrospectHandler.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/query/lookup/NamespaceLookupIntrospectHandler.java @@ -22,7 +22,6 @@ import com.google.common.collect.ImmutableMap; import org.apache.druid.common.utils.ServletResourceUtils; import org.apache.druid.java.util.common.ISE; -import org.apache.druid.query.extraction.MapLookupExtractor; import org.apache.druid.server.lookup.namespace.cache.CacheScheduler; import javax.ws.rs.GET; @@ -95,6 +94,6 @@ public Response getMap() private Map getLatest() { - return ((MapLookupExtractor) factory.get()).getMap(); + return factory.get().asMap(); } } diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheHandler.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheHandler.java index 58a4a991be41..5874086498e9 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheHandler.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheHandler.java @@ -20,9 +20,11 @@ package org.apache.druid.server.lookup.namespace.cache; import org.apache.druid.java.util.common.logger.Logger; +import org.apache.druid.query.lookup.LookupExtractor; import java.io.Closeable; import java.util.Map; +import java.util.function.Supplier; public final class CacheHandler implements AutoCloseable, Closeable { @@ -45,6 +47,14 @@ public Map getCache() return cache; } + /** + * Returns a {@link LookupExtractor} view of the cached data. + */ + public LookupExtractor asLookupExtractor(final boolean isOneToOne, final Supplier cacheKeySupplier) + { + return cacheManager.asLookupExtractor(this, isOneToOne, cacheKeySupplier); + } + @Override public void close() { diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java index 30fa710a4b54..786916cf97ce 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/CacheScheduler.java @@ -30,6 +30,7 @@ import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.emitter.service.ServiceEmitter; import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; +import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.query.lookup.namespace.CacheGenerator; import org.apache.druid.query.lookup.namespace.ExtractionNamespace; @@ -46,6 +47,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; /** * Usage: @@ -409,6 +411,14 @@ public Map getCache() return cacheHandler.getCache(); } + /** + * Returns a {@link LookupExtractor} view of the cached data. + */ + public LookupExtractor asLookupExtractor(final boolean isOneToOne, final Supplier cacheKeySupplier) + { + return cacheHandler.asLookupExtractor(isOneToOne, cacheKeySupplier); + } + public String getVersion() { return version; diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/NamespaceExtractionCacheManager.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/NamespaceExtractionCacheManager.java index 268a19371b57..52be300604f9 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/NamespaceExtractionCacheManager.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/NamespaceExtractionCacheManager.java @@ -25,10 +25,12 @@ import org.apache.druid.java.util.common.lifecycle.Lifecycle; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.emitter.service.ServiceEmitter; +import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.server.lookup.namespace.NamespaceExtractionConfig; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; /** * Usage: @@ -112,6 +114,16 @@ boolean waitForServiceToEnd(long time, TimeUnit unit) throws InterruptedExceptio */ public abstract CacheHandler attachCache(CacheHandler cache); + /** + * Given a cache from {@link #createCache()} or {@link #allocateCache()}, return a {@link LookupExtractor} + * view of it. + */ + public abstract LookupExtractor asLookupExtractor( + CacheHandler cacheHandler, + boolean isOneToOne, + Supplier cacheKeySupplier + ); + abstract void disposeCache(CacheHandler cacheHandler); abstract int cacheCount(); diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OffHeapNamespaceExtractionCacheManager.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OffHeapNamespaceExtractionCacheManager.java index 7eb1637ebaf0..0b31868a4b86 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OffHeapNamespaceExtractionCacheManager.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OffHeapNamespaceExtractionCacheManager.java @@ -27,6 +27,8 @@ import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.emitter.service.ServiceEmitter; import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; +import org.apache.druid.query.extraction.MapLookupExtractor; +import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.server.lookup.namespace.NamespaceExtractionConfig; import org.mapdb.DB; import org.mapdb.DBMaker; @@ -38,6 +40,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Supplier; /** * @@ -82,7 +85,8 @@ public void run() try { doDispose(); // Log statement goes after doDispose(), because logging may fail (e. g. if we are in shutdownHooks). - log.error("OffHeapNamespaceExtractionCacheManager.disposeCache() was not called, disposed resources by the JVM"); + log.error( + "OffHeapNamespaceExtractionCacheManager.disposeCache() was not called, disposed resources by the JVM"); } catch (Throwable t) { try { @@ -251,6 +255,23 @@ public CacheHandler attachCache(CacheHandler cache) return cache; } + @Override + public LookupExtractor asLookupExtractor( + final CacheHandler cacheHandler, + final boolean isOneToOne, + final Supplier cacheKeySupplier + ) + { + return new MapLookupExtractor(cacheHandler.getCache(), isOneToOne) + { + @Override + public byte[] getCacheKey() + { + return cacheKeySupplier.get(); + } + }; + } + @Override void disposeCache(CacheHandler cacheHandler) { diff --git a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java index 8cb7a3a5439f..224ffc5cf00b 100644 --- a/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java +++ b/extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OnHeapNamespaceExtractionCacheManager.java @@ -20,30 +20,30 @@ package org.apache.druid.server.lookup.namespace.cache; import com.google.inject.Inject; +import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.lifecycle.Lifecycle; -import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.emitter.service.ServiceEmitter; import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; import org.apache.druid.query.extraction.MapLookupExtractor; +import org.apache.druid.query.lookup.ImmutableLookupMap; +import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.server.lookup.namespace.NamespaceExtractionConfig; import java.lang.ref.WeakReference; import java.util.Collections; -import java.util.HashMap; import java.util.Iterator; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.function.Supplier; /** * */ public class OnHeapNamespaceExtractionCacheManager extends NamespaceExtractionCacheManager { - private static final Logger LOG = new Logger(OnHeapNamespaceExtractionCacheManager.class); - /** * Weak collection of caches is "the second level of defence". Normally all users of {@link #createCache()} must call * {@link CacheHandler#close()} on the returned CacheHandler instance manually. But if they don't do this for @@ -94,7 +94,8 @@ public CacheHandler createCache() @Override public CacheHandler allocateCache() { - Map cache = new HashMap<>(); + // Object2ObjectOpenHashMap has a bit smaller footprint than HashMap + Map cache = new Object2ObjectOpenHashMap<>(); // untracked, but disposing will explode if we don't create a weak reference here return new CacheHandler(this, cache, new WeakReference<>(cache)); } @@ -105,14 +106,34 @@ public CacheHandler attachCache(CacheHandler cache) if (caches.contains((WeakReference>) cache.id)) { throw new ISE("cache [%s] is already attached", cache.id); } - // this cache is not thread-safe, make sure nothing ever writes to it - Map immutable = Collections.unmodifiableMap(cache.getCache()); + // replace Object2ObjectOpenHashMap with ImmutableLookupMap + final ImmutableLookupMap immutable = ImmutableLookupMap.fromMap(cache.getCache()); WeakReference> cacheRef = new WeakReference<>(immutable); expungeCollectedCaches(); caches.add(cacheRef); return new CacheHandler(this, immutable, cacheRef); } + @Override + public LookupExtractor asLookupExtractor( + final CacheHandler cache, + final boolean isOneToOne, + final Supplier cacheKeySupplier + ) + { + if (cache.getCache() instanceof ImmutableLookupMap) { + return ((ImmutableLookupMap) cache.getCache()).asLookupExtractor(isOneToOne, cacheKeySupplier); + } else { + return new MapLookupExtractor(cache.getCache(), isOneToOne) { + @Override + public byte[] getCacheKey() + { + return cacheKeySupplier.get(); + } + }; + } + } + @Override void disposeCache(CacheHandler cacheHandler) { @@ -140,7 +161,7 @@ void monitor(ServiceEmitter serviceEmitter) if (cache != null) { numEntries += cache.size(); - heapSizeInBytes += MapLookupExtractor.estimateHeapFootprint(cache); + heapSizeInBytes += MapLookupExtractor.estimateHeapFootprint(cache.entrySet()); } } diff --git a/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactoryTest.java b/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactoryTest.java index 1aa4ef1dfe4b..36c88cf10e81 100644 --- a/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactoryTest.java +++ b/extensions-core/lookups-cached-global/src/test/java/org/apache/druid/query/lookup/NamespaceLookupExtractorFactoryTest.java @@ -37,6 +37,7 @@ import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.jackson.JacksonUtils; +import org.apache.druid.query.extraction.MapLookupExtractor; import org.apache.druid.query.lookup.namespace.ExtractionNamespace; import org.apache.druid.query.lookup.namespace.UriExtractionNamespace; import org.apache.druid.server.DruidNode; @@ -47,8 +48,10 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.mockito.ArgumentMatchers; import javax.ws.rs.core.Response; +import java.util.Collection; import java.util.HashMap; import java.util.Map; @@ -70,7 +73,7 @@ public class NamespaceLookupExtractorFactoryTest static { NullHandling.initializeForTests(); } - + private final ObjectMapper mapper = new DefaultObjectMapper(); @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); @@ -183,7 +186,7 @@ public void testStartReturnsImmediatelyAndFails() throws InterruptedException { final ExtractionNamespace extractionNamespace = () -> 0; when(scheduler.scheduleAndWait(extractionNamespace, 1L)) - .thenReturn(null); + .thenReturn(null); final NamespaceLookupExtractorFactory namespaceLookupExtractorFactory = new NamespaceLookupExtractorFactory( extractionNamespace, @@ -239,8 +242,8 @@ public void testSimpleStartGetStop() throws Exception final ExtractionNamespace extractionNamespace = () -> 0; expectScheduleAndWaitOnce(extractionNamespace); when(entry.getCacheState()).thenReturn(versionedCache); - when(entry.getCache()).thenReturn(new HashMap()); - when(versionedCache.getCache()).thenReturn(new HashMap<>()); + when(versionedCache.asLookupExtractor(ArgumentMatchers.eq(false), ArgumentMatchers.any())) + .thenReturn(new MapLookupExtractor(new HashMap<>(), false)); when(versionedCache.getVersion()).thenReturn("0"); final NamespaceLookupExtractorFactory namespaceLookupExtractorFactory = new NamespaceLookupExtractorFactory( @@ -256,7 +259,7 @@ public void testSimpleStartGetStop() throws Exception verify(entry).getCacheState(); verify(entry).close(); verify(versionedCache).getVersion(); - verify(versionedCache, atLeastOnce()).getCache(); + verify(versionedCache, atLeastOnce()).asLookupExtractor(ArgumentMatchers.eq(false), ArgumentMatchers.any()); verifyNoMoreInteractions(scheduler, entry, versionedCache); } @@ -455,8 +458,8 @@ public void testSimpleIntrospectionHandler() throws Exception Assert.assertNotNull(clazz.getMethod("getVersion").invoke(handler)); Assert.assertEquals(ImmutableSet.of("foo"), ((Response) clazz.getMethod("getKeys").invoke(handler)).getEntity()); Assert.assertEquals( - ImmutableSet.of("bar"), - ((Response) clazz.getMethod("getValues").invoke(handler)).getEntity() + ImmutableList.of("bar"), + ImmutableList.copyOf((Collection) ((Response) clazz.getMethod("getValues").invoke(handler)).getEntity()) ); Assert.assertEquals( ImmutableMap.builder().put("foo", "bar").build(), diff --git a/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java b/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java index 2bffc36ee808..508b07b9330e 100644 --- a/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java +++ b/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java @@ -30,7 +30,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; @@ -107,27 +106,15 @@ public List unapply(@Nullable final String value) } @Override - public boolean canIterate() + public boolean supportsAsMap() { return false; } @Override - public boolean canGetKeySet() + public Map asMap() { - return false; - } - - @Override - public Iterable> iterable() - { - throw new UnsupportedOperationException("Cannot iterate"); - } - - @Override - public Set keySet() - { - throw new UnsupportedOperationException("Cannot get key set"); + throw new UnsupportedOperationException("Cannot get map view"); } @Override diff --git a/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/PollingLookup.java b/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/PollingLookup.java index a59891254c00..95e7fb14e469 100644 --- a/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/PollingLookup.java +++ b/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/PollingLookup.java @@ -37,7 +37,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -169,27 +168,15 @@ public List unapply(@Nullable final String value) } @Override - public boolean canIterate() + public boolean supportsAsMap() { return false; } @Override - public boolean canGetKeySet() + public Map asMap() { - return false; - } - - @Override - public Iterable> iterable() - { - throw new UnsupportedOperationException("Cannot iterate"); - } - - @Override - public Set keySet() - { - throw new UnsupportedOperationException("Cannot get key set"); + throw new UnsupportedOperationException("Cannot get map view"); } @Override diff --git a/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/polling/OnHeapPollingCache.java b/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/polling/OnHeapPollingCache.java index 8f93cf39ff67..18e6be1d2b34 100644 --- a/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/polling/OnHeapPollingCache.java +++ b/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/polling/OnHeapPollingCache.java @@ -88,13 +88,7 @@ public List getKeys(final V value) @SuppressWarnings("unchecked") public long estimateHeapFootprint() { - for (final Map.Entry entry : immutableMap.entrySet()) { - if (!(entry.getKey() instanceof String) || !(entry.getValue() instanceof String)) { - return 0; - } - } - - return MapLookupExtractor.estimateHeapFootprint((Map) immutableMap); + return MapLookupExtractor.estimateHeapFootprint(((Map) immutableMap).entrySet()); } @Override diff --git a/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java b/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java index 0d5eb67ab63c..43588bf8c476 100644 --- a/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java +++ b/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java @@ -134,15 +134,15 @@ public void testGetCacheKey() } @Test - public void testCanGetKeySet() + public void testSupportsAsMap() { - Assert.assertFalse(loadingLookup.canGetKeySet()); + Assert.assertFalse(loadingLookup.supportsAsMap()); } @Test - public void testKeySet() + public void testAsMap() { expectedException.expect(UnsupportedOperationException.class); - loadingLookup.keySet(); + loadingLookup.asMap(); } } diff --git a/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupTest.java b/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupTest.java index 209b86dcde2c..41f0b6de2592 100644 --- a/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupTest.java +++ b/extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupTest.java @@ -211,16 +211,16 @@ public void testGetCacheKey() } @Test - public void testCanGetKeySet() + public void testSupportsAsMap() { - Assert.assertFalse(pollingLookup.canGetKeySet()); + Assert.assertFalse(pollingLookup.supportsAsMap()); } @Test - public void testKeySet() + public void testAsMap() { expectedException.expect(UnsupportedOperationException.class); - pollingLookup.keySet(); + pollingLookup.asMap(); } @Test diff --git a/processing/src/main/java/org/apache/druid/query/extraction/MapLookupExtractor.java b/processing/src/main/java/org/apache/druid/query/extraction/MapLookupExtractor.java index fd6647adffd5..763e5bc706ae 100644 --- a/processing/src/main/java/org/apache/druid/query/extraction/MapLookupExtractor.java +++ b/processing/src/main/java/org/apache/druid/query/extraction/MapLookupExtractor.java @@ -28,6 +28,7 @@ import com.google.common.collect.Iterators; import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.lookup.ImmutableLookupMap; import org.apache.druid.query.lookup.LookupExtractor; import javax.annotation.Nullable; @@ -37,8 +38,14 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; +/** + * Lookup extractor backed by any kind of map. + * + * When the map is immutable, use {@link ImmutableLookupMap} instead. + */ @JsonTypeName("map") public class MapLookupExtractor extends LookupExtractor { @@ -62,22 +69,20 @@ public MapLookupExtractor( /** * Estimate the heap footprint of a Map. * - * Important note: the implementation accepts any kind of Map, but estimates zero footprint for keys and values of - * types other than String. + * Important note: the implementation accepts any kind of map entries, but estimates zero footprint for keys and + * values of types other than String. */ - public static long estimateHeapFootprint(@Nullable final Map map) + public static long estimateHeapFootprint(final Iterable> entries) { - if (map == null) { - return 0; - } - - final int numEntries = map.size(); + int numEntries = 0; long numChars = 0; - for (Map.Entry sEntry : map.entrySet()) { + for (Map.Entry sEntry : entries) { final K key = sEntry.getKey(); final V value = sEntry.getValue(); + numEntries++; + if (key instanceof String) { numChars += ((String) key).length(); } @@ -124,9 +129,9 @@ public Iterator unapplyAll(Set values) Iterators.filter( map.entrySet().iterator(), entry -> { - if (entry.getKey() == null && NullHandling.sqlCompatible()) { - // apply always maps null to null in SQL-compatible mode. - return values.contains(null); + if (NullHandling.sqlCompatible() && entry.getKey() == null) { + // Null keys are omitted in SQL-compatible mode. + return false; } else { return values.contains(NullHandling.emptyToNullIfNeeded(entry.getValue())); } @@ -169,33 +174,21 @@ public byte[] getCacheKey() } @Override - public boolean canIterate() + public boolean supportsAsMap() { return true; } @Override - public boolean canGetKeySet() + public Map asMap() { - return true; - } - - @Override - public Iterable> iterable() - { - return map.entrySet(); - } - - @Override - public Set keySet() - { - return Collections.unmodifiableSet(map.keySet()); + return Collections.unmodifiableMap(map); } @Override public long estimateHeapFootprint() { - return estimateHeapFootprint(map); + return estimateHeapFootprint(map.entrySet()); } @Override @@ -210,13 +203,12 @@ public boolean equals(Object o) MapLookupExtractor that = (MapLookupExtractor) o; - return map.equals(that.map); + return isOneToOne == that.isOneToOne && map.equals(that.map); } @Override public int hashCode() { - return map.hashCode(); + return Objects.hash(isOneToOne, map); } - } diff --git a/processing/src/main/java/org/apache/druid/query/lookup/ImmutableLookupMap.java b/processing/src/main/java/org/apache/druid/query/lookup/ImmutableLookupMap.java new file mode 100644 index 000000000000..520f3d55521b --- /dev/null +++ b/processing/src/main/java/org/apache/druid/query/lookup/ImmutableLookupMap.java @@ -0,0 +1,261 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.query.lookup; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ForwardingMap; +import com.google.common.collect.Maps; +import it.unimi.dsi.fastutil.objects.Object2IntMap; +import it.unimi.dsi.fastutil.objects.Object2IntOpenHashMap; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.java.util.common.Pair; +import org.apache.druid.java.util.common.guava.Comparators; +import org.apache.druid.query.extraction.MapLookupExtractor; + +import javax.annotation.Nullable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.function.Supplier; + +/** + * Similar to {@link MapLookupExtractor}, but immutable, and also reversible without iterating the entire map. + * + * Forward lookup, {@link ImmutableLookupExtractor#apply(String)}, is implemented using an {@link Object2IntOpenHashMap} + * with load factor {@link #LOAD_FACTOR}. The value of the map is an index into {@link #keys} and {@link #values}. + * + * Reverse lookup, {@link ImmutableLookupExtractor#unapply(String)}, is implemented using binary search through + * {@link #values}. The {@link #keys} and {@link #values} lists are both sorted by value using {@link #VALUE_COMPARATOR}. + * + * Relative to {@link MapLookupExtractor} backed by Java {@link HashMap}, this map has been observed to have + * somewhat lower footprint, same performance for {@link ImmutableLookupExtractor#apply(String)}, and significantly + * faster for {@link ImmutableLookupExtractor#unapply(String)}. It should be used whenever the map does not need to + * be mutated. + */ +public final class ImmutableLookupMap extends ForwardingMap +{ + /** + * Default value for {@link #keyToEntry}. + */ + private static final int NOT_FOUND = -1; + + /** + * Load factor lower than default {@link it.unimi.dsi.fastutil.Hash#DEFAULT_LOAD_FACTOR} to speed up performance + * a bit for {@link ImmutableLookupExtractor#apply(String)}. + */ + private static final float LOAD_FACTOR = 0.6f; + + private static final Comparator> VALUE_COMPARATOR = + Comparator.comparing(pair -> pair.rhs, Comparators.naturalNullsFirst()); + /** + * Key to index in {@link #keys} and {@link #values}. + */ + private final Object2IntMap keyToEntry; + // Store keys and values as separate lists to avoid storing Entry objects (saves some memory). + private final List keys; + private final List values; + private final Map asMap; + + private ImmutableLookupMap( + final Object2IntMap keyToEntry, + final List keys, + final List values + ) + { + this.keyToEntry = Preconditions.checkNotNull(keyToEntry, "keyToEntry"); + this.keys = Preconditions.checkNotNull(keys, "keys"); + this.values = Preconditions.checkNotNull(values, "values"); + this.asMap = Collections.unmodifiableMap(Maps.transformValues(keyToEntry, values::get)); + } + + /** + * Create an {@link ImmutableLookupMap} from a particular map. The provided map will not be stored in the + * returned {@link ImmutableLookupMap}. + */ + public static ImmutableLookupMap fromMap(final Map srcMap) + { + final List> entriesList = new ArrayList<>(srcMap.size()); + for (final Entry entry : srcMap.entrySet()) { + entriesList.add(Pair.of(entry.getKey(), entry.getValue())); + } + entriesList.sort(VALUE_COMPARATOR); + + final List keys = new ArrayList<>(entriesList.size()); + final List values = new ArrayList<>(entriesList.size()); + + for (final Pair entry : entriesList) { + keys.add(entry.lhs); + values.add(entry.rhs); + } + + entriesList.clear(); // save memory + + // Populate keyToEntries map. + final Object2IntMap keyToEntry = new Object2IntOpenHashMap<>(keys.size(), LOAD_FACTOR); + keyToEntry.defaultReturnValue(NOT_FOUND); + for (int i = 0; i < keys.size(); i++) { + keyToEntry.put(keys.get(i), i); + } + + return new ImmutableLookupMap(keyToEntry, keys, values); + } + + @Override + protected Map delegate() + { + return asMap; + } + + public LookupExtractor asLookupExtractor(final boolean isOneToOne, final Supplier cacheKey) + { + return new ImmutableLookupExtractor(isOneToOne, cacheKey); + } + + public class ImmutableLookupExtractor extends LookupExtractor + { + private final boolean isOneToOne; + private final Supplier cacheKeySupplier; + + private ImmutableLookupExtractor(final boolean isOneToOne, final Supplier cacheKeySupplier) + { + this.isOneToOne = isOneToOne; + this.cacheKeySupplier = Preconditions.checkNotNull(cacheKeySupplier, "cacheKeySupplier"); + } + + @Nullable + @Override + public String apply(@Nullable String key) + { + String keyEquivalent = NullHandling.nullToEmptyIfNeeded(key); + if (keyEquivalent == null) { + // keyEquivalent is null only for SQL-compatible null mode + // Otherwise, null will be replaced with empty string in nullToEmptyIfNeeded above. + return null; + } + + final int entryId = keyToEntry.getInt(keyEquivalent); + if (entryId == NOT_FOUND) { + return null; + } else { + return NullHandling.emptyToNullIfNeeded(values.get(entryId)); + } + } + + @Override + protected List unapply(@Nullable String value) + { + final List unapplied = unapplyInternal(value, !NullHandling.sqlCompatible()); + + if (NullHandling.replaceWithDefault() && value == null) { + // Also check empty string, if the value was null. + final List emptyStringUnapplied = unapplyInternal("", true); + if (!emptyStringUnapplied.isEmpty()) { + final List combined = new ArrayList<>(unapplied.size() + emptyStringUnapplied.size()); + combined.addAll(unapplied); + combined.addAll(emptyStringUnapplied); + return combined; + } + } + + return unapplied; + } + + @Override + public boolean supportsAsMap() + { + return true; + } + + @Override + public Map asMap() + { + return ImmutableLookupMap.this.asMap; + } + + @Override + public boolean isOneToOne() + { + return isOneToOne; + } + + @Override + public long estimateHeapFootprint() + { + return MapLookupExtractor.estimateHeapFootprint(asMap().entrySet()); + } + + @Override + public byte[] getCacheKey() + { + return cacheKeySupplier.get(); + } + + /** + * Unapply a single value, without null-handling-based transformation. Just look for entries in the map that + * have the provided value. + * + * @param value value to search for + * @param includeNullKeys whether to include null keys in the returned list + */ + private List unapplyInternal(@Nullable final String value, boolean includeNullKeys) + { + final int index = Collections.binarySearch(values, value, Comparators.naturalNullsFirst()); + if (index < 0) { + return Collections.emptyList(); + } + + // Found the value at "index". The value may appear multiple times, and "index" isn't guaranteed to be any + // particular appearance. So we need to expand the search in both directions to find all the matching entries. + int minIndex = index /* min is inclusive */, maxIndex = index + 1 /* max is exclusive */; + + while (minIndex > 0 && Objects.equals(values.get(minIndex - 1), value)) { + minIndex--; + } + + while (maxIndex < values.size() && Objects.equals(values.get(maxIndex), value)) { + maxIndex++; + } + + if (minIndex + 1 == maxIndex) { + // Only found one entry for this value. + final String key = keys.get(index); + if (key == null && !includeNullKeys) { + return Collections.emptyList(); + } else { + return Collections.singletonList(keys.get(index)); + } + } else { + // Found multiple entries. + final List retVal = new ArrayList<>(maxIndex - minIndex); + for (int i = minIndex; i < maxIndex; i++) { + final String key = keys.get(i); + if (key != null || includeNullKeys) { + retVal.add(key); + } + } + return retVal; + } + } + } +} diff --git a/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractor.java b/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractor.java index 2ed69f04f5f5..183007c41a4b 100644 --- a/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractor.java +++ b/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractor.java @@ -79,7 +79,8 @@ public Map applyAll(Iterable keys) * @param value the value to apply the reverse lookup. {@link NullHandling#emptyToNullIfNeeded(String)} will have * been applied to the value. * - * @return the list of keys that maps to the provided value. + * @return the list of keys that maps to the provided value. In SQL-compatible null handling mode, null keys + * are omitted. */ protected abstract List unapply(@Nullable String value); @@ -90,7 +91,8 @@ public Map applyAll(Iterable keys) * 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. + * this lookup instance does not support reverse lookups. In SQL-compatible null handling mode, null keys are + * omitted. */ @Nullable public Iterator unapplyAll(Set values) @@ -99,28 +101,16 @@ public Iterator unapplyAll(Set values) } /** - * Returns true if this lookup extractor's {@link #iterable()} method will return a valid iterator. + * Returns whether this lookup extractor's {@link #asMap()} will return a valid map. */ - public abstract boolean canIterate(); + public abstract boolean supportsAsMap(); /** - * Returns true if this lookup extractor's {@link #keySet()} method will return a valid set. - */ - public abstract boolean canGetKeySet(); - - /** - * Returns an Iterable that iterates over the keys and values in this lookup extractor. - * - * @throws UnsupportedOperationException if {@link #canIterate()} returns false. - */ - public abstract Iterable> iterable(); - - /** - * Returns a Set of all keys in this lookup extractor. The returned Set will not change. + * Returns a Map view of this lookup extractor. The map may change along with the underlying lookup data. * - * @throws UnsupportedOperationException if {@link #canGetKeySet()} returns false. + * @throws UnsupportedOperationException if {@link #supportsAsMap()} returns false. */ - public abstract Set keySet(); + public abstract Map asMap(); /** * Create a cache key for use in results caching @@ -137,8 +127,8 @@ public boolean isOneToOne() /** * Estimated heap footprint of this object. Not guaranteed to be accurate. For example, some implementations return - * zero even though they do use on-heap structures. However, the most common class, {@link MapLookupExtractor}, - * does have a reasonable implementation. + * zero even though they do use on-heap structures. However, the most common classes, {@link MapLookupExtractor} + * and {@link ImmutableLookupMap}, do have reasonable implementations. * * This API is provided for best-effort memory management and monitoring. */ diff --git a/processing/src/main/java/org/apache/druid/query/lookup/LookupSegment.java b/processing/src/main/java/org/apache/druid/query/lookup/LookupSegment.java index 51d8f8994c50..4e841455b6be 100644 --- a/processing/src/main/java/org/apache/druid/query/lookup/LookupSegment.java +++ b/processing/src/main/java/org/apache/druid/query/lookup/LookupSegment.java @@ -34,7 +34,7 @@ /** * A {@link org.apache.druid.segment.Segment} that is based on a {@link LookupExtractor}. Allows direct - * querying of lookups. The lookup must support {@link LookupExtractor#iterable()}. + * querying of lookups. The lookup must support {@link LookupExtractor#asMap()}. */ public class LookupSegment extends RowBasedSegment> { @@ -51,11 +51,11 @@ public LookupSegment(final String lookupName, final LookupExtractorFactory looku Sequences.simple(() -> { final LookupExtractor extractor = lookupExtractorFactory.get(); - if (!extractor.canIterate()) { - throw new ISE("Cannot iterate lookup[%s]", lookupExtractorFactory); + if (!extractor.supportsAsMap()) { + throw new ISE("Cannot retrieve map view from lookup[%s]", lookupExtractorFactory); } - return extractor.iterable().iterator(); + return extractor.asMap().entrySet().iterator(); }), new RowAdapter>() { diff --git a/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinMatcher.java b/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinMatcher.java index 319d4a81ef1b..4f2c7e873f00 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinMatcher.java +++ b/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinMatcher.java @@ -184,8 +184,8 @@ private LookupJoinMatcher( // Verify that extractor can be iterated when needed. if (condition.isAlwaysTrue() || remainderNeeded) { Preconditions.checkState( - extractor.canIterate(), - "Cannot iterate lookup, but iteration is required for this join" + extractor.supportsAsMap(), + "Cannot read lookup as Map, which is required for this join" ); } } @@ -231,7 +231,7 @@ public void matchCondition() if (condition.isAlwaysFalse()) { currentEntry.set(null); } else if (condition.isAlwaysTrue()) { - currentIterator = extractor.iterable().iterator(); + currentIterator = extractor.asMap().entrySet().iterator(); nextMatch(); } else { // Not always true, not always false, it's a normal condition. @@ -285,13 +285,13 @@ public void matchRemainder() matchingRemainder = true; if (condition.isAlwaysFalse()) { - currentIterator = extractor.iterable().iterator(); + currentIterator = extractor.asMap().entrySet().iterator(); } else if (condition.isAlwaysTrue()) { currentIterator = Collections.emptyIterator(); } else { //noinspection ConstantConditions - entry can not be null because extractor.iterable() prevents this currentIterator = Iterators.filter( - extractor.iterable().iterator(), + extractor.asMap().entrySet().iterator(), entry -> !matchedKeys.contains(entry.getKey()) ); } diff --git a/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinable.java b/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinable.java index de66016ba7eb..638506e5901b 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinable.java +++ b/processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinable.java @@ -100,8 +100,8 @@ public JoinMatcher makeJoinMatcher( @Override public ColumnValuesWithUniqueFlag getMatchableColumnValues(String columnName, boolean includeNull, int maxNumValues) { - if (LookupColumnSelectorFactory.KEY_COLUMN.equals(columnName) && extractor.canGetKeySet()) { - final Set keys = extractor.keySet(); + if (LookupColumnSelectorFactory.KEY_COLUMN.equals(columnName) && extractor.supportsAsMap()) { + final Set keys = extractor.asMap().keySet(); final Set nonMatchingValues; diff --git a/processing/src/test/java/org/apache/druid/query/extraction/MapBasedLookupExtractorTest.java b/processing/src/test/java/org/apache/druid/query/extraction/MapBasedLookupExtractorTest.java new file mode 100644 index 000000000000..578feae41c71 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/extraction/MapBasedLookupExtractorTest.java @@ -0,0 +1,234 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.query.extraction; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Sets; +import org.apache.commons.compress.utils.Lists; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.query.lookup.ImmutableLookupMap; +import org.apache.druid.query.lookup.LookupExtractor; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * Base test class for {@link MapLookupExtractor} and {@link ImmutableLookupMap.ImmutableLookupExtractor}. + */ +public abstract class MapBasedLookupExtractorTest +{ + protected final Map simpleLookupMap = + ImmutableMap.of( + "foo", "bar", + "null", "", + "empty String", "", + "", "empty_string" + ); + + @BeforeClass + public static void setUpClass() + { + NullHandling.initializeForTests(); + } + + /** + * Subclasses implement this method to test the proper {@link LookupExtractor} implementation. + */ + protected abstract LookupExtractor makeLookupExtractor(Map map); + + @Test + public void test_unapplyAll_simple() + { + final LookupExtractor lookup = makeLookupExtractor(simpleLookupMap); + Assert.assertEquals(Collections.singletonList("foo"), unapply(lookup, "bar")); + if (NullHandling.sqlCompatible()) { + Assert.assertEquals(Collections.emptySet(), Sets.newHashSet(unapply(lookup, null))); + Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(unapply(lookup, ""))); + } else { + // Don't test unapply(lookup, "") under replace-with-default mode, because it isn't allowed in that mode, and + // implementation behavior is undefined. unapply is specced such that it requires its inputs to go + // through nullToEmptyIfNeeded. + Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(unapply(lookup, null))); + } + Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(unapply(lookup, "empty_string"))); + Assert.assertEquals("not existing value returns empty list", Collections.emptyList(), unapply(lookup, "not There")); + } + + @Test + public void test_asMap_simple() + { + final LookupExtractor lookup = makeLookupExtractor(simpleLookupMap); + Assert.assertTrue(lookup.supportsAsMap()); + Assert.assertEquals(simpleLookupMap, lookup.asMap()); + } + + @Test + public void test_apply_simple() + { + final LookupExtractor lookup = makeLookupExtractor(simpleLookupMap); + Assert.assertEquals("bar", lookup.apply("foo")); + Assert.assertEquals(NullHandling.sqlCompatible() ? "" : null, lookup.apply("null")); + Assert.assertEquals(NullHandling.sqlCompatible() ? "" : null, lookup.apply("empty String")); + Assert.assertEquals("empty_string", lookup.apply("")); + Assert.assertEquals(NullHandling.sqlCompatible() ? null : "empty_string", lookup.apply(null)); + } + + @Test + public void test_apply_nullKey() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put(null, "nv"); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertNull(lookup.apply("missing")); + Assert.assertNull(lookup.apply("")); + Assert.assertNull(lookup.apply(null)); + } + + @Test + public void test_unapply_nullKey() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put(null, "nv"); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertEquals( + NullHandling.sqlCompatible() ? Collections.emptyList() : Collections.singletonList(null), + unapply(lookup, "nv") + ); + + Assert.assertEquals( + Collections.emptyList(), + unapply(lookup, null) + ); + } + + @Test + public void test_apply_nullValue() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put("nk", null); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertNull(lookup.apply("nk")); + } + + @Test + public void test_unapply_nullValue() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put("nk", null); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertEquals( + Collections.singletonList("nk"), + unapply(lookup, null) + ); + } + + @Test + public void test_apply_emptyStringValue() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put("nk", ""); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertEquals( + NullHandling.sqlCompatible() ? "" : null, + lookup.apply("nk") + ); + } + + @Test + public void test_unapply_emptyStringValue() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put("nk", ""); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertEquals( + NullHandling.sqlCompatible() ? Collections.emptyList() : Collections.singletonList("nk"), + unapply(lookup, null) + ); + + if (NullHandling.sqlCompatible()) { + // Don't test unapply(lookup, "") under replace-with-default mode, because it isn't allowed in that mode, and + // implementation behavior is undefined. unapply is specced such that it requires its inputs to go + // through nullToEmptyIfNeeded. + Assert.assertEquals( + Collections.singletonList("nk"), + unapply(lookup, "") + ); + } + } + + @Test + public void test_apply_nullAndEmptyStringKey() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put(null, "nv"); + mapWithNullKey.put("", "empty"); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertNull(lookup.apply("missing")); + Assert.assertEquals("empty", lookup.apply("")); + Assert.assertEquals( + NullHandling.sqlCompatible() ? null : "empty", + lookup.apply(null) + ); + } + + @Test + public void test_unapply_nullAndEmptyStringKey() + { + final Map mapWithNullKey = new HashMap<>(); + mapWithNullKey.put(null, "nv"); + mapWithNullKey.put("", "empty"); + final LookupExtractor lookup = makeLookupExtractor(mapWithNullKey); + + Assert.assertEquals( + Collections.singletonList(""), + unapply(lookup, "empty") + ); + + Assert.assertEquals( + NullHandling.sqlCompatible() ? Collections.emptyList() : Collections.singletonList(null), + unapply(lookup, "nv") + ); + } + + @Test + public void test_estimateHeapFootprint() + { + Assert.assertEquals(0L, makeLookupExtractor(Collections.emptyMap()).estimateHeapFootprint()); + Assert.assertEquals(388L, makeLookupExtractor(simpleLookupMap).estimateHeapFootprint()); + } + + protected List unapply(final LookupExtractor lookup, @Nullable final String s) + { + return Lists.newArrayList(lookup.unapplyAll(Collections.singleton(s))); + } +} diff --git a/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractorTest.java b/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractorTest.java index f05a93c3e756..5f97df4b977c 100644 --- a/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractorTest.java +++ b/processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractorTest.java @@ -19,71 +19,30 @@ package org.apache.druid.query.extraction; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Sets; -import org.apache.commons.compress.utils.Lists; -import org.apache.druid.common.config.NullHandling; +import nl.jqno.equalsverifier.EqualsVerifier; +import org.apache.druid.query.lookup.LookupExtractor; import org.junit.Assert; -import org.junit.BeforeClass; import org.junit.Test; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; -public class MapLookupExtractorTest +public class MapLookupExtractorTest extends MapBasedLookupExtractorTest { - private final Map lookupMap = - ImmutableMap.of( - "foo", "bar", - "null", "", - "empty String", "", - "", "empty_string" - ); - private final MapLookupExtractor fn = new MapLookupExtractor(lookupMap, false); - - @BeforeClass - public static void setUpClass() - { - NullHandling.initializeForTests(); - } - - @Test - public void testUnApply() - { - Assert.assertEquals(Collections.singletonList("foo"), unapply("bar")); - if (NullHandling.sqlCompatible()) { - Assert.assertEquals(Collections.emptySet(), Sets.newHashSet(unapply(null))); - Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(unapply(""))); - } else { - // Don't test unapply("") under replace-with-default mode, because it isn't allowed in that mode, and - // implementation behavior is undefined. unapply is specced such that it requires its inputs to go - // through nullToEmptyIfNeeded. - Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(unapply(null))); - } - Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(unapply("empty_string"))); - Assert.assertEquals("not existing value returns empty list", Collections.emptyList(), unapply("not There")); - } - - @Test - public void testGetMap() + @Override + protected LookupExtractor makeLookupExtractor(Map map) { - Assert.assertEquals(lookupMap, fn.getMap()); + return new MapLookupExtractor(map, false); } @Test - public void testApply() + public void test_getCacheKey() { - Assert.assertEquals("bar", fn.apply("foo")); - } - - @Test - public void testGetCacheKey() - { - final MapLookupExtractor fn2 = new MapLookupExtractor(ImmutableMap.copyOf(lookupMap), false); + final LookupExtractor fn = makeLookupExtractor(simpleLookupMap); + final MapLookupExtractor fn2 = new MapLookupExtractor(ImmutableMap.copyOf(simpleLookupMap), false); Assert.assertArrayEquals(fn.getCacheKey(), fn2.getCacheKey()); final MapLookupExtractor fn3 = new MapLookupExtractor(ImmutableMap.of("foo2", "bar"), false); Assert.assertFalse(Arrays.equals(fn.getCacheKey(), fn3.getCacheKey())); @@ -92,77 +51,36 @@ public void testGetCacheKey() } @Test - public void testCanIterate() - { - Assert.assertTrue(fn.canIterate()); - } - - @Test - public void testIterable() - { - Assert.assertEquals( - ImmutableList.copyOf(lookupMap.entrySet()), - ImmutableList.copyOf(fn.iterable()) - ); - } - - @Test - public void testEstimateHeapFootprint() + public void test_estimateHeapFootprint_static() { - Assert.assertEquals(0L, new MapLookupExtractor(Collections.emptyMap(), false).estimateHeapFootprint()); - Assert.assertEquals(388L, new MapLookupExtractor(ImmutableMap.copyOf(lookupMap), false).estimateHeapFootprint()); + Assert.assertEquals(0L, MapLookupExtractor.estimateHeapFootprint(Collections.emptyMap().entrySet())); + Assert.assertEquals(388L, MapLookupExtractor.estimateHeapFootprint(ImmutableMap.copyOf(simpleLookupMap).entrySet())); } @Test - public void testEstimateHeapFootprintStatic() - { - Assert.assertEquals(0L, MapLookupExtractor.estimateHeapFootprint(null)); - Assert.assertEquals(0L, MapLookupExtractor.estimateHeapFootprint(Collections.emptyMap())); - Assert.assertEquals(388L, MapLookupExtractor.estimateHeapFootprint(ImmutableMap.copyOf(lookupMap))); - } - - @Test - public void testEstimateHeapFootprintStaticNullKeysAndValues() + public void test_estimateHeapFootprint_staticNullKeysAndValues() { final Map mapWithNullKeysAndNullValues = new HashMap<>(); mapWithNullKeysAndNullValues.put("foo", "bar"); mapWithNullKeysAndNullValues.put("foo2", null); - Assert.assertEquals(180L, MapLookupExtractor.estimateHeapFootprint(mapWithNullKeysAndNullValues)); + Assert.assertEquals(180L, MapLookupExtractor.estimateHeapFootprint(mapWithNullKeysAndNullValues.entrySet())); } @Test - public void testEstimateHeapFootprintStaticNonStringKeysAndValues() + public void test_estimateHeapFootprint_staticNonStringKeysAndValues() { final Map mapWithNonStringKeysAndValues = new HashMap<>(); mapWithNonStringKeysAndValues.put(3L, 1); mapWithNonStringKeysAndValues.put(4L, 3.2); - Assert.assertEquals(160L, MapLookupExtractor.estimateHeapFootprint(mapWithNonStringKeysAndValues)); - } - - @Test - public void testEquals() - { - final MapLookupExtractor fn2 = new MapLookupExtractor(ImmutableMap.copyOf(lookupMap), false); - Assert.assertEquals(fn, fn2); - final MapLookupExtractor fn3 = new MapLookupExtractor(ImmutableMap.of("foo2", "bar"), false); - Assert.assertNotEquals(fn, fn3); - final MapLookupExtractor fn4 = new MapLookupExtractor(ImmutableMap.of("foo", "bar2"), false); - Assert.assertNotEquals(fn, fn4); + Assert.assertEquals(160L, MapLookupExtractor.estimateHeapFootprint(mapWithNonStringKeysAndValues.entrySet())); } @Test - public void testHashCode() - { - final MapLookupExtractor fn2 = new MapLookupExtractor(ImmutableMap.copyOf(lookupMap), false); - Assert.assertEquals(fn.hashCode(), fn2.hashCode()); - final MapLookupExtractor fn3 = new MapLookupExtractor(ImmutableMap.of("foo2", "bar"), false); - Assert.assertNotEquals(fn.hashCode(), fn3.hashCode()); - final MapLookupExtractor fn4 = new MapLookupExtractor(ImmutableMap.of("foo", "bar2"), false); - Assert.assertNotEquals(fn.hashCode(), fn4.hashCode()); - } - - private List unapply(final String s) + public void test_equalsAndHashCode() { - return Lists.newArrayList(fn.unapplyAll(Collections.singleton(s))); + EqualsVerifier.forClass(MapLookupExtractor.class) + .usingGetClass() + .withNonnullFields("map") + .verify(); } } diff --git a/processing/src/test/java/org/apache/druid/query/filter/InDimFilterTest.java b/processing/src/test/java/org/apache/druid/query/filter/InDimFilterTest.java index 0e1ebc818773..2154b79d10e8 100644 --- a/processing/src/test/java/org/apache/druid/query/filter/InDimFilterTest.java +++ b/processing/src/test/java/org/apache/druid/query/filter/InDimFilterTest.java @@ -28,9 +28,10 @@ import org.apache.druid.common.config.NullHandling; import org.apache.druid.data.input.MapBasedRow; import org.apache.druid.jackson.DefaultObjectMapper; -import org.apache.druid.query.extraction.MapLookupExtractor; import org.apache.druid.query.extraction.RegexDimExtractionFn; +import org.apache.druid.query.lookup.ImmutableLookupMap; import org.apache.druid.query.lookup.LookupExtractionFn; +import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.segment.RowAdapters; import org.apache.druid.segment.RowBasedColumnSelectorFactory; import org.apache.druid.segment.column.ColumnIndexSupplier; @@ -221,7 +222,7 @@ public void testOptimizeLookup_simple() final Map lookupMap = new HashMap<>(); lookupMap.put("abc", "def"); lookupMap.put("foo", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, null, null, true); Assert.assertEquals( @@ -285,7 +286,7 @@ public void testOptimizeLookup_replaceMissingValueWith() final Map lookupMap = new HashMap<>(); lookupMap.put("abc", "def"); lookupMap.put("foo", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, "baz", null, true); Assert.assertEquals( @@ -352,7 +353,7 @@ public void testOptimizeLookup_replaceMissingValue_containingNull() lookupMap.put("nv", null); lookupMap.put("abc", "def"); lookupMap.put("foo", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, "bar", null, true); Assert.assertNull( @@ -418,7 +419,7 @@ public void testOptimizeLookup_replaceMissingValue_containingEmptyString() lookupMap.put("emptystring", ""); lookupMap.put("abc", "def"); lookupMap.put("foo", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, "bar", null, true); Assert.assertNull( @@ -483,7 +484,7 @@ public void testOptimizeLookup_containingEmptyString() { final Map lookupMap = new HashMap<>(); lookupMap.put("emptystring", ""); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, null, null, true); Assert.assertEquals( @@ -513,7 +514,7 @@ public void testOptimizeLookup_emptyStringKey() { final Map lookupMap = new HashMap<>(); lookupMap.put("", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, null, null, true); Assert.assertEquals( @@ -544,7 +545,7 @@ public void testOptimizeLookup_retainMissingValue() final Map lookupMap = new HashMap<>(); lookupMap.put("abc", "def"); lookupMap.put("foo", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, true, null, null, true); Assert.assertEquals( @@ -611,7 +612,7 @@ public void testOptimizeLookup_injective() final Map lookupMap = new HashMap<>(); lookupMap.put("abc", "def"); lookupMap.put("foo", "bar"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, true); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(true, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, null, null, true); Assert.assertEquals( @@ -680,7 +681,7 @@ public void testOptimizeLookup_nullKey() { final Map lookupMap = new HashMap<>(); lookupMap.put(null, "nv"); - final MapLookupExtractor lookup = new MapLookupExtractor(lookupMap, false); + final LookupExtractor lookup = ImmutableLookupMap.fromMap(lookupMap).asLookupExtractor(false, () -> new byte[0]); final LookupExtractionFn extractionFn = new LookupExtractionFn(lookup, false, null, null, true); Assert.assertEquals( diff --git a/processing/src/test/java/org/apache/druid/query/lookup/ImmutableLookupMapTest.java b/processing/src/test/java/org/apache/druid/query/lookup/ImmutableLookupMapTest.java new file mode 100644 index 000000000000..6ae3872b26a0 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/lookup/ImmutableLookupMapTest.java @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.query.lookup; + +import nl.jqno.equalsverifier.EqualsVerifier; +import org.apache.druid.query.extraction.MapBasedLookupExtractorTest; +import org.junit.Test; + +import java.util.Map; + +public class ImmutableLookupMapTest extends MapBasedLookupExtractorTest +{ + @Override + protected LookupExtractor makeLookupExtractor(Map map) + { + return ImmutableLookupMap.fromMap(map).asLookupExtractor(false, () -> new byte[0]); + } + + @Test + public void test_equalsAndHashCode() + { + EqualsVerifier.forClass(ImmutableLookupMap.class) + .withNonnullFields("asMap") + .withIgnoredFields("keyToEntry", "keys", "values") + .verify(); + } +} diff --git a/processing/src/test/java/org/apache/druid/segment/join/lookup/LookupJoinableTest.java b/processing/src/test/java/org/apache/druid/segment/join/lookup/LookupJoinableTest.java index 0582feaa033a..ca993fe1ae02 100644 --- a/processing/src/test/java/org/apache/druid/segment/join/lookup/LookupJoinableTest.java +++ b/processing/src/test/java/org/apache/druid/segment/join/lookup/LookupJoinableTest.java @@ -21,10 +21,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterators; import com.google.common.collect.Sets; import org.apache.druid.common.config.NullHandling; import org.apache.druid.query.filter.InDimFilter; +import org.apache.druid.query.lookup.ImmutableLookupMap; import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.segment.column.ColumnCapabilities; import org.apache.druid.segment.column.ValueType; @@ -34,18 +34,13 @@ import org.junit.Before; import org.junit.Ignore; import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.junit.MockitoJUnitRunner; import java.util.Collections; -import java.util.HashSet; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; -import java.util.Set; -@RunWith(MockitoJUnitRunner.class) public class LookupJoinableTest extends InitializedNullHandlingTest { private static final String UNKNOWN_COLUMN = "UNKNOWN_COLUMN"; @@ -54,27 +49,20 @@ public class LookupJoinableTest extends InitializedNullHandlingTest private static final String SEARCH_VALUE_VALUE = "SEARCH_VALUE_VALUE"; private static final String SEARCH_VALUE_UNKNOWN = "SEARCH_VALUE_UNKNOWN"; - @Mock - private LookupExtractor extractor; - private LookupJoinable target; @Before public void setUp() { - final Set keyValues = new HashSet<>(); - keyValues.add("foo"); - keyValues.add("bar"); - keyValues.add(""); - keyValues.add(null); - - Mockito.doReturn(SEARCH_VALUE_VALUE).when(extractor).apply(SEARCH_KEY_VALUE); - Mockito.when(extractor.unapplyAll(Collections.singleton(SEARCH_VALUE_VALUE))) - .thenAnswer(invocation -> Iterators.singletonIterator(SEARCH_KEY_VALUE)); - Mockito.when(extractor.unapplyAll(Collections.singleton(SEARCH_VALUE_UNKNOWN))) - .thenAnswer(invocation -> Collections.emptyIterator()); - Mockito.doReturn(true).when(extractor).canGetKeySet(); - Mockito.doReturn(keyValues).when(extractor).keySet(); + final Map lookupMap = new HashMap<>(); + lookupMap.put("foo", "xyzzy"); + lookupMap.put("bar", "xyzzy"); + lookupMap.put("", "xyzzy"); + lookupMap.put(null, "xyzzy"); + lookupMap.put(SEARCH_KEY_VALUE, SEARCH_VALUE_VALUE); + + final LookupExtractor extractor = ImmutableLookupMap.fromMap(lookupMap) + .asLookupExtractor(false, () -> new byte[0]); target = LookupJoinable.wrap(extractor); } @@ -300,7 +288,9 @@ public void getMatchableColumnValuesIfAllUniqueForKeyColumnShouldReturnValues() ); Assert.assertEquals( - NullHandling.sqlCompatible() ? ImmutableSet.of("foo", "bar", "") : ImmutableSet.of("foo", "bar"), + NullHandling.sqlCompatible() + ? ImmutableSet.of(SEARCH_KEY_VALUE, "foo", "bar", "") + : ImmutableSet.of(SEARCH_KEY_VALUE, "foo", "bar"), values.getColumnValues() ); } @@ -315,7 +305,7 @@ public void getMatchableColumnValuesWithIncludeNullIfAllUniqueForKeyColumnShould ); Assert.assertEquals( - Sets.newHashSet("foo", "bar", "", null), + Sets.newHashSet(SEARCH_KEY_VALUE, "foo", "bar", "", null), values.getColumnValues() ); } diff --git a/processing/src/test/java/org/apache/druid/segment/join/table/LookupJoinMatcherTest.java b/processing/src/test/java/org/apache/druid/segment/join/table/LookupJoinMatcherTest.java index 08a1a452af9c..6b43b2bc009c 100644 --- a/processing/src/test/java/org/apache/druid/segment/join/table/LookupJoinMatcherTest.java +++ b/processing/src/test/java/org/apache/druid/segment/join/table/LookupJoinMatcherTest.java @@ -64,8 +64,8 @@ public class LookupJoinMatcherTest @Before public void setUp() { - Mockito.doReturn(true).when(extractor).canIterate(); - Mockito.doReturn(lookupMap.entrySet()).when(extractor).iterable(); + Mockito.doReturn(true).when(extractor).supportsAsMap(); + Mockito.doReturn(lookupMap).when(extractor).asMap(); } @Test @@ -82,7 +82,7 @@ public void testCreateConditionAlwaysFalseShouldReturnSuccessfullyAndNotThrowExc public void testCreateConditionAlwaysTrueShouldReturnSuccessfullyAndNotThrowException() { JoinConditionAnalysis condition = JoinConditionAnalysis.forExpression("1", PREFIX, ExprMacroTable.nil()); - Mockito.doReturn(true).when(extractor).canIterate(); + Mockito.doReturn(true).when(extractor).supportsAsMap(); target = LookupJoinMatcher.create(extractor, leftSelectorFactory, condition, false); Assert.assertNotNull(target); target = LookupJoinMatcher.create(extractor, leftSelectorFactory, condition, true); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java index c43e298d847f..5420be74e7f2 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java @@ -1562,8 +1562,8 @@ public void testInnerJoinQueryOfLookup(Map queryContext) .build() ), ImmutableList.of( - new Object[]{"", "a", "xa", "xa"}, - new Object[]{"1", "a", "xa", "xa"} + new Object[]{"", "a", "xabc", "xabc"}, + new Object[]{"1", "a", "xabc", "xabc"} ) ); } @@ -2517,9 +2517,9 @@ public void testSelectOnLookupUsingRightJoinOperator(Map queryCo ), ImmutableList.of( new Object[]{"abc", "abc", "xabc"}, + new Object[]{NULL_STRING, "6", "x6"}, new Object[]{NULL_STRING, "a", "xa"}, - new Object[]{NULL_STRING, "nosuchkey", "mysteryvalue"}, - new Object[]{NULL_STRING, "6", "x6"} + new Object[]{NULL_STRING, "nosuchkey", "mysteryvalue"} ) ); } @@ -2565,9 +2565,9 @@ public void testSelectOnLookupUsingFullJoinOperator(Map queryCon new Object[]{"1", 4f, 1L, NULL_STRING, NULL_STRING}, new Object[]{"def", 5f, 1L, NULL_STRING, NULL_STRING}, new Object[]{"abc", 6f, 1L, "abc", "xabc"}, + new Object[]{NULL_STRING, NULL_FLOAT, NULL_LONG, "6", "x6"}, new Object[]{NULL_STRING, NULL_FLOAT, NULL_LONG, "a", "xa"}, - new Object[]{NULL_STRING, NULL_FLOAT, NULL_LONG, "nosuchkey", "mysteryvalue"}, - new Object[]{NULL_STRING, NULL_FLOAT, NULL_LONG, "6", "x6"} + new Object[]{NULL_STRING, NULL_FLOAT, NULL_LONG, "nosuchkey", "mysteryvalue"} ) ); } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java index 460c0dd76384..2cf8296dfc07 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java @@ -1098,10 +1098,10 @@ public void testSelectStarFromLookup() .build() ), ImmutableList.of( - new Object[]{"a", "xa"}, new Object[]{"abc", "xabc"}, - new Object[]{"nosuchkey", "mysteryvalue"}, - new Object[]{"6", "x6"} + new Object[]{"6", "x6"}, + new Object[]{"a", "xa"}, + new Object[]{"nosuchkey", "mysteryvalue"} ) ); } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/LookylooModule.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/LookylooModule.java index 7dec7babb9b4..4fab6a4a2322 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/LookylooModule.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/LookylooModule.java @@ -27,7 +27,7 @@ import org.apache.druid.initialization.DruidModule; import org.apache.druid.query.expression.LookupEnabledTestExprMacroTable; import org.apache.druid.query.expression.LookupExprMacro; -import org.apache.druid.query.extraction.MapLookupExtractor; +import org.apache.druid.query.lookup.ImmutableLookupMap; import org.apache.druid.query.lookup.LookupExtractor; import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; import org.apache.druid.query.lookup.LookupSerdeModule; @@ -56,30 +56,28 @@ public void configure(Binder binder) MapBinder.newMapBinder(binder, String.class, LookupExtractor.class); lookupBinder.addBinding(LookupEnabledTestExprMacroTable.LOOKYLOO).toInstance( - new MapLookupExtractor( + ImmutableLookupMap.fromMap( ImmutableMap.builder() .put("a", "xa") .put("abc", "xabc") .put("nosuchkey", "mysteryvalue") .put("6", "x6") - .build(), - false - ) + .build() + ).asLookupExtractor(false, () -> new byte[0]) ); lookupBinder.addBinding(LOOKYLOO_CHAINED).toInstance( - new MapLookupExtractor( + ImmutableLookupMap.fromMap( ImmutableMap.builder() .put("xa", "za") .put("xabc", "zabc") .put("x6", "z6") - .build(), - false - ) + .build() + ).asLookupExtractor(false, () -> new byte[0]) ); lookupBinder.addBinding(LOOKYLOO_INJECTIVE).toInstance( - new MapLookupExtractor( + ImmutableLookupMap.fromMap( ImmutableMap.builder() .put("", "x") .put("10.1", "x10.1") @@ -87,9 +85,8 @@ public void configure(Binder binder) .put("1", "x1") .put("def", "xdef") .put("abc", "xabc") - .build(), - true - ) + .build() + ).asLookupExtractor(true, () -> new byte[0]) ); binder.bind(DataSegment.PruneSpecsHolder.class).toInstance(DataSegment.PruneSpecsHolder.DEFAULT);