From a15f8b1ab19d0900aec0b2288e5e755db51c3d8b Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 28 Jan 2020 17:07:38 -0800 Subject: [PATCH 1/6] Add LookupJoinableFactory. Enables joins where the right-hand side is a lookup. Includes an integration test. Also, includes changes to LookupExtractorFactoryContainerProvider: 1) Add "getAllLookupNames", which will be needed to eventually connect lookups to Druid's SQL catalog. 2) Convert "get" from nullable to Optional return. 3) Swap out most usages of LookupReferencesManager in favor of the simpler LookupExtractorFactoryContainerProvider interface. --- .../sql/BloomFilterSqlAggregatorTest.java | 2 +- .../filter/sql/BloomDimFilterSqlTest.java | 2 +- .../queries/wikipedia_editstream_queries.json | 60 +++++++++ .../query/dimension/LookupDimensionSpec.java | 19 ++- ...okupExtractorFactoryContainerProvider.java | 19 ++- .../lookup/RegisteredLookupExtractionFn.java | 11 +- .../druid/segment/join/JoinableFactory.java | 2 + .../druid/guice/JoinableFactoryModule.java | 7 +- .../lookup/LookupIntrospectionResource.java | 8 +- .../query/lookup/LookupReferencesManager.java | 12 +- .../druid/query/lookup/LookupSerdeModule.java | 15 ++- .../segment/join/InlineJoinableFactory.java | 7 +- .../segment/join/LookupJoinableFactory.java | 58 ++++++++ .../dimension/LookupDimensionSpecTest.java | 21 ++- .../LookupEnabledTestExprMacroTable.java | 103 ++++++++------ .../LookupIntrospectionResourceTest.java | 24 ++-- .../lookup/LookupReferencesManagerTest.java | 70 ++++++++-- .../RegisteredLookupExtractionFnTest.java | 72 +++++----- .../join/LookupJoinableFactoryTest.java | 127 ++++++++++++++++++ .../druid/sql/calcite/util/CalciteTests.java | 12 +- 20 files changed, 515 insertions(+), 136 deletions(-) create mode 100644 server/src/main/java/org/apache/druid/segment/join/LookupJoinableFactory.java create mode 100644 server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java diff --git a/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/aggregation/bloom/sql/BloomFilterSqlAggregatorTest.java b/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/aggregation/bloom/sql/BloomFilterSqlAggregatorTest.java index 6c63e1943bd3..9ca58856fa29 100644 --- a/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/aggregation/bloom/sql/BloomFilterSqlAggregatorTest.java +++ b/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/aggregation/bloom/sql/BloomFilterSqlAggregatorTest.java @@ -101,7 +101,7 @@ public class BloomFilterSqlAggregatorTest extends InitializedNullHandlingTest binder -> { binder.bind(Key.get(ObjectMapper.class, Json.class)).toInstance(TestHelper.makeJsonMapper()); binder.bind(LookupExtractorFactoryContainerProvider.class).toInstance( - LookupEnabledTestExprMacroTable.createTestLookupReferencesManager( + LookupEnabledTestExprMacroTable.createTestLookupProvider( ImmutableMap.of( "a", "xa", "abc", "xabc" diff --git a/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/filter/sql/BloomDimFilterSqlTest.java b/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/filter/sql/BloomDimFilterSqlTest.java index 6e5265a75366..188dfdf148b3 100644 --- a/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/filter/sql/BloomDimFilterSqlTest.java +++ b/extensions-core/druid-bloom-filter/src/test/java/org/apache/druid/query/filter/sql/BloomDimFilterSqlTest.java @@ -68,7 +68,7 @@ public class BloomDimFilterSqlTest extends BaseCalciteQueryTest binder -> { binder.bind(Key.get(ObjectMapper.class, Json.class)).toInstance(TestHelper.makeJsonMapper()); binder.bind(LookupExtractorFactoryContainerProvider.class).toInstance( - LookupEnabledTestExprMacroTable.createTestLookupReferencesManager( + LookupEnabledTestExprMacroTable.createTestLookupProvider( ImmutableMap.of( "a", "xa", "abc", "xabc" diff --git a/integration-tests/src/test/resources/queries/wikipedia_editstream_queries.json b/integration-tests/src/test/resources/queries/wikipedia_editstream_queries.json index 9c6560da1242..16b99ba03f18 100644 --- a/integration-tests/src/test/resources/queries/wikipedia_editstream_queries.json +++ b/integration-tests/src/test/resources/queries/wikipedia_editstream_queries.json @@ -1300,6 +1300,66 @@ } ] }, + { + "description": "topN, 1 agg, join to lookup", + "query": { + "queryType": "topN", + "dataSource": { + "type": "join", + "left": "wikipedia_editstream", + "right": { + "type": "lookup", + "lookup": "wiki-simple" + }, + "rightPrefix": "j.", + "condition": "page == \"j.k\"", + "joinType": "LEFT" + }, + "intervals": ["2013-01-01T00:00:00.000/2013-01-08T00:00:00.000"], + "granularity": "all", + "virtualColumns": [ + { + "type": "expression", + "name": "lookupPage", + "expression": "nvl(\"j.v\", \"page\")", + "outputType": "string" + } + ], + "aggregations": [ + { + "type": "count", + "name": "rows" + } + ], + "dimension": "lookupPage", + "metric": "rows", + "threshold": 3, + "context": { + "useCache": "true", + "populateCache": "true", + "timeout": 360000 + } + }, + "expectedResults": [ + { + "timestamp": "2013-01-01T00:00:00.000Z", + "result": [ + { + "lookupPage": "lookup!", + "rows": 991 + }, + { + "lookupPage": "Wikipedia:Administrators'_noticeboard/Incidents", + "rows": 990 + }, + { + "lookupPage": "Wikipedia:Administrator_intervention_against_vandalism", + "rows": 800 + } + ] + } + ] + }, { "description": "topN, 1 agg, join to inline", "query": { diff --git a/processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java b/processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java index 7e8923653d63..69c3b91621dc 100644 --- a/processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java +++ b/processing/src/main/java/org/apache/druid/query/dimension/LookupDimensionSpec.java @@ -25,6 +25,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Strings; 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.extraction.ExtractionFn; import org.apache.druid.query.filter.DimFilterUtils; @@ -136,13 +137,17 @@ public String getName() @Override public ExtractionFn getExtractionFn() { - final LookupExtractor lookupExtractor = Strings.isNullOrEmpty(name) - ? this.lookup - : Preconditions.checkNotNull( - lookupExtractorFactoryContainerProvider.get(name), - "Lookup [%s] not found", - name - ).getLookupExtractorFactory().get(); + final LookupExtractor lookupExtractor; + + if (Strings.isNullOrEmpty(name)) { + lookupExtractor = this.lookup; + } else { + lookupExtractor = lookupExtractorFactoryContainerProvider + .get(name) + .orElseThrow(() -> new ISE("Lookup [%s] not found", name)) + .getLookupExtractorFactory() + .get(); + } return new LookupExtractionFn( lookupExtractor, diff --git a/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractorFactoryContainerProvider.java b/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractorFactoryContainerProvider.java index a9fada052c50..1a61520903b2 100644 --- a/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractorFactoryContainerProvider.java +++ b/processing/src/main/java/org/apache/druid/query/lookup/LookupExtractorFactoryContainerProvider.java @@ -19,14 +19,25 @@ package org.apache.druid.query.lookup; -import javax.annotation.Nullable; +import java.util.Optional; +import java.util.Set; /** * Provides {@link LookupExtractorFactoryContainer} to query and indexing time dimension transformations. + * + * The most important production implementation is LookupReferencesManager. */ -@FunctionalInterface public interface LookupExtractorFactoryContainerProvider { - @Nullable - LookupExtractorFactoryContainer get(String lookupName); + /** + * Returns the set of all lookup names that {@link #get} can return containers for. Note that because the underlying + * set of valid lookups might change over time, it is not guaranteed that calling {@link #get} on the results will + * actually yield a container (it might have been removed). + */ + Set getAllLookupNames(); + + /** + * Returns a lookup container for the provided lookupName, if it exists. + */ + Optional get(String lookupName); } diff --git a/processing/src/main/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFn.java b/processing/src/main/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFn.java index d9b0b676e79b..f7f5b7b00970 100644 --- a/processing/src/main/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFn.java +++ b/processing/src/main/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFn.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; +import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.query.extraction.ExtractionFn; @@ -145,11 +146,11 @@ private LookupExtractionFn ensureDelegate() // http://www.javamex.com/tutorials/double_checked_locking.shtml synchronized (delegateLock) { if (null == delegate) { - final LookupExtractor factory = Preconditions.checkNotNull( - manager.get(getLookup()), - "Lookup [%s] not found", - getLookup() - ).getLookupExtractorFactory().get(); + final LookupExtractor factory = + manager.get(getLookup()) + .orElseThrow(() -> new ISE("Lookup [%s] not found", getLookup())) + .getLookupExtractorFactory() + .get(); delegate = new LookupExtractionFn( factory, diff --git a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java index 5016d75a79c5..ae56bc289aa5 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java +++ b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java @@ -25,6 +25,8 @@ /** * Utility for creating {@link Joinable} objects. + * + * @see org.apache.druid.guice.DruidBinders#joinableFactoryBinder to register factories */ public interface JoinableFactory { diff --git a/server/src/main/java/org/apache/druid/guice/JoinableFactoryModule.java b/server/src/main/java/org/apache/druid/guice/JoinableFactoryModule.java index a5d406af37c3..cf2b05d3dd2e 100644 --- a/server/src/main/java/org/apache/druid/guice/JoinableFactoryModule.java +++ b/server/src/main/java/org/apache/druid/guice/JoinableFactoryModule.java @@ -26,8 +26,10 @@ import com.google.inject.multibindings.MapBinder; import org.apache.druid.query.DataSource; import org.apache.druid.query.InlineDataSource; +import org.apache.druid.query.LookupDataSource; import org.apache.druid.segment.join.InlineJoinableFactory; import org.apache.druid.segment.join.JoinableFactory; +import org.apache.druid.segment.join.LookupJoinableFactory; import org.apache.druid.segment.join.MapDataSourceJoinableFactoryWarehouse; import java.util.Map; @@ -41,7 +43,10 @@ public class JoinableFactoryModule implements Module * Default mappings of datasources to factories. */ private static final Map, Class> FACTORY_MAPPINGS = - ImmutableMap.of(InlineDataSource.class, InlineJoinableFactory.class); + ImmutableMap.of( + InlineDataSource.class, InlineJoinableFactory.class, + LookupDataSource.class, LookupJoinableFactory.class + ); @Override public void configure(Binder binder) diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupIntrospectionResource.java b/server/src/main/java/org/apache/druid/query/lookup/LookupIntrospectionResource.java index 092ab6cf9033..8ccce127f0c0 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupIntrospectionResource.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupIntrospectionResource.java @@ -28,6 +28,7 @@ import javax.ws.rs.PathParam; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; +import java.util.Optional; @Path("/druid/v1/lookups/introspect") @ResourceFilters(ConfigResourceFilter.class) @@ -48,12 +49,15 @@ public LookupIntrospectionResource( @Path("/{lookupId}") public Object introspectLookup(@PathParam("lookupId") final String lookupId) { - final LookupExtractorFactoryContainer container = lookupExtractorFactoryContainerProvider.get(lookupId); + final Optional maybeContainer = + lookupExtractorFactoryContainerProvider.get(lookupId); - if (container == null) { + if (!maybeContainer.isPresent()) { LOGGER.error("trying to introspect non existing lookup [%s]", lookupId); return Response.status(Response.Status.NOT_FOUND).build(); } + + final LookupExtractorFactoryContainer container = maybeContainer.get(); LookupIntrospectHandler introspectHandler = container.getLookupExtractorFactory().getIntrospectHandler(); if (introspectHandler != null) { return introspectHandler; diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java index 39b3613bc9b6..1c395221ec46 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java @@ -55,6 +55,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletionService; import java.util.concurrent.ExecutionException; @@ -295,11 +296,16 @@ private void addNotice(Notice notice) } @Override - @Nullable - public LookupExtractorFactoryContainer get(String lookupName) + public Optional get(String lookupName) { Preconditions.checkState(lifecycleLock.awaitStarted(1, TimeUnit.MILLISECONDS)); - return stateRef.get().lookupMap.get(lookupName); + return Optional.ofNullable(stateRef.get().lookupMap.get(lookupName)); + } + + @Override + public Set getAllLookupNames() + { + return stateRef.get().lookupMap.keySet(); } // Note that this should ensure that "toLoad" and "toDrop" are disjoint. diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java b/server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java index d08205cd4128..d8a981b8db46 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java @@ -30,8 +30,10 @@ import org.apache.druid.query.dimension.LookupDimensionSpec; import org.apache.druid.query.expression.LookupExprMacro; -import javax.annotation.Nullable; +import java.util.Collections; import java.util.List; +import java.util.Optional; +import java.util.Set; /** * Variant of {@link LookupModule} that only supports serde of {@link org.apache.druid.query.Query} objects, to allow @@ -66,11 +68,16 @@ public void configure(Binder binder) */ private static class NoopLookupExtractorFactoryContainerProvider implements LookupExtractorFactoryContainerProvider { - @Nullable @Override - public LookupExtractorFactoryContainer get(String lookupName) + public Set getAllLookupNames() { - return null; + return Collections.emptySet(); + } + + @Override + public Optional get(String lookupName) + { + return Optional.empty(); } } } diff --git a/server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java b/server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java index d4b9937165bb..5ebd038b74d2 100644 --- a/server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java +++ b/server/src/main/java/org/apache/druid/segment/join/InlineJoinableFactory.java @@ -31,14 +31,17 @@ /** * A {@link JoinableFactory} for {@link InlineDataSource}. It works by building an {@link IndexedTable}. + * + * It is not valid to pass any other DataSource type to the "build" method. */ public class InlineJoinableFactory implements JoinableFactory { @Override public Optional build(final DataSource dataSource, final JoinConditionAnalysis condition) { - if (condition.canHashJoin() && dataSource instanceof InlineDataSource) { - final InlineDataSource inlineDataSource = (InlineDataSource) dataSource; + final InlineDataSource inlineDataSource = (InlineDataSource) dataSource; + + if (condition.canHashJoin()) { final List rightKeyColumns = condition.getEquiConditions().stream().map(Equality::getRightColumn).distinct().collect(Collectors.toList()); diff --git a/server/src/main/java/org/apache/druid/segment/join/LookupJoinableFactory.java b/server/src/main/java/org/apache/druid/segment/join/LookupJoinableFactory.java new file mode 100644 index 000000000000..a6fd209b1d12 --- /dev/null +++ b/server/src/main/java/org/apache/druid/segment/join/LookupJoinableFactory.java @@ -0,0 +1,58 @@ +/* + * 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.segment.join; + +import com.google.inject.Inject; +import org.apache.druid.query.DataSource; +import org.apache.druid.query.LookupDataSource; +import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; +import org.apache.druid.segment.join.lookup.LookupJoinable; + +import java.util.Optional; + +/** + * A {@link JoinableFactory} for {@link LookupDataSource}. + * + * It is not valid to pass any other DataSource type to the "build" method. + */ +public class LookupJoinableFactory implements JoinableFactory +{ + private final LookupExtractorFactoryContainerProvider lookupProvider; + + @Inject + public LookupJoinableFactory(LookupExtractorFactoryContainerProvider lookupProvider) + { + this.lookupProvider = lookupProvider; + } + + @Override + public Optional build(final DataSource dataSource, final JoinConditionAnalysis condition) + { + final LookupDataSource lookupDataSource = (LookupDataSource) dataSource; + + if (condition.canHashJoin()) { + final String lookupName = lookupDataSource.getLookupName(); + return lookupProvider.get(lookupName) + .map(c -> LookupJoinable.wrap(c.getLookupExtractorFactory().get())); + } else { + return Optional.empty(); + } + } +} diff --git a/server/src/test/java/org/apache/druid/query/dimension/LookupDimensionSpecTest.java b/server/src/test/java/org/apache/druid/query/dimension/LookupDimensionSpecTest.java index fb5d1b458574..59b4ae974d91 100644 --- a/server/src/test/java/org/apache/druid/query/dimension/LookupDimensionSpecTest.java +++ b/server/src/test/java/org/apache/druid/query/dimension/LookupDimensionSpecTest.java @@ -42,6 +42,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.Map; +import java.util.Optional; @RunWith(JUnitParamsRunner.class) public class LookupDimensionSpecTest @@ -55,7 +56,9 @@ public class LookupDimensionSpecTest static { EasyMock .expect(LOOKUP_REF_MANAGER.get(EasyMock.eq("lookupName"))) - .andReturn(new LookupExtractorFactoryContainer("v0", new MapLookupExtractorFactory(STRING_MAP, false))) + .andReturn( + Optional.of(new LookupExtractorFactoryContainer("v0", new MapLookupExtractorFactory(STRING_MAP, false))) + ) .anyTimes(); EasyMock.replay(LOOKUP_REF_MANAGER); } @@ -75,14 +78,26 @@ public void testSerDesr(DimensionSpec lookupDimSpec) throws IOException LOOKUP_REF_MANAGER ); String serLookup = mapper.writeValueAsString(lookupDimSpec); - Assert.assertEquals(lookupDimSpec, mapper.readerFor(DimensionSpec.class).with(injectableValues).readValue(serLookup)); + Assert.assertEquals( + lookupDimSpec, + mapper.readerFor(DimensionSpec.class).with(injectableValues).readValue(serLookup) + ); } private Object[] parametersForTestSerDesr() { return new Object[]{ new LookupDimensionSpec("dimName", "outputName", MAP_LOOKUP_EXTRACTOR, true, null, null, true, null), - new LookupDimensionSpec("dimName", "outputName", MAP_LOOKUP_EXTRACTOR, false, "Missing_value", null, true, null), + new LookupDimensionSpec( + "dimName", + "outputName", + MAP_LOOKUP_EXTRACTOR, + false, + "Missing_value", + null, + true, + null + ), new LookupDimensionSpec("dimName", "outputName", MAP_LOOKUP_EXTRACTOR, false, null, null, true, null), new LookupDimensionSpec("dimName", "outputName", null, false, null, "name", true, LOOKUP_REF_MANAGER) }; diff --git a/server/src/test/java/org/apache/druid/query/expression/LookupEnabledTestExprMacroTable.java b/server/src/test/java/org/apache/druid/query/expression/LookupEnabledTestExprMacroTable.java index 6c36b82ea217..503fa1cd8243 100644 --- a/server/src/test/java/org/apache/druid/query/expression/LookupEnabledTestExprMacroTable.java +++ b/server/src/test/java/org/apache/druid/query/expression/LookupEnabledTestExprMacroTable.java @@ -20,6 +20,7 @@ package org.apache.druid.query.expression; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import org.apache.druid.math.expr.ExprMacroTable; @@ -29,10 +30,12 @@ import org.apache.druid.query.lookup.LookupExtractorFactoryContainer; import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; import org.apache.druid.query.lookup.LookupIntrospectHandler; -import org.easymock.EasyMock; import javax.annotation.Nullable; import java.util.Collections; +import java.util.Map; +import java.util.Optional; +import java.util.Set; /** * Separate from {@link TestExprMacroTable} since that one is in druid-processing, which doesn't have @@ -41,6 +44,7 @@ public class LookupEnabledTestExprMacroTable extends ExprMacroTable { public static final ExprMacroTable INSTANCE = new LookupEnabledTestExprMacroTable(); + private static final String LOOKYLOO = "lookyloo"; private LookupEnabledTestExprMacroTable() { @@ -49,7 +53,7 @@ private LookupEnabledTestExprMacroTable() Iterables.concat( TestExprMacroTable.INSTANCE.getMacros(), Collections.singletonList( - new LookupExprMacro(createTestLookupReferencesManager(ImmutableMap.of("foo", "xfoo"))) + new LookupExprMacro(createTestLookupProvider(ImmutableMap.of("foo", "xfoo"))) ) ) ) @@ -57,53 +61,64 @@ private LookupEnabledTestExprMacroTable() } /** - * Returns a mock {@link LookupExtractorFactoryContainerProvider} that has one lookup, "lookyloo". + * Returns a {@link LookupExtractorFactoryContainerProvider} that has one lookup, "lookyloo". Public so other tests + * can use this helper method directly. */ - public static LookupExtractorFactoryContainerProvider createTestLookupReferencesManager( - final ImmutableMap theLookup - ) + public static LookupExtractorFactoryContainerProvider createTestLookupProvider(final Map theLookup) { - final LookupExtractorFactoryContainerProvider lookupExtractorFactoryContainerProvider = - EasyMock.createMock(LookupExtractorFactoryContainerProvider.class); - EasyMock.expect(lookupExtractorFactoryContainerProvider.get(EasyMock.eq("lookyloo"))).andReturn( - new LookupExtractorFactoryContainer( - "v0", - new LookupExtractorFactory() - { - @Override - public boolean start() - { - throw new UnsupportedOperationException(); - } + final LookupExtractorFactoryContainer container = new LookupExtractorFactoryContainer( + "v0", + new LookupExtractorFactory() + { + @Override + public boolean start() + { + throw new UnsupportedOperationException(); + } - @Override - public boolean close() - { - throw new UnsupportedOperationException(); - } + @Override + public boolean close() + { + throw new UnsupportedOperationException(); + } - @Override - public boolean replaces(@Nullable final LookupExtractorFactory other) - { - throw new UnsupportedOperationException(); - } + @Override + public boolean replaces(@Nullable final LookupExtractorFactory other) + { + throw new UnsupportedOperationException(); + } - @Override - public LookupIntrospectHandler getIntrospectHandler() - { - throw new UnsupportedOperationException(); - } + @Override + public LookupIntrospectHandler getIntrospectHandler() + { + throw new UnsupportedOperationException(); + } - @Override - public LookupExtractor get() - { - return new MapLookupExtractor(theLookup, false); - } - } - ) - ).anyTimes(); - EasyMock.expect(lookupExtractorFactoryContainerProvider.get(EasyMock.not(EasyMock.eq("lookyloo")))).andReturn(null).anyTimes(); - EasyMock.replay(lookupExtractorFactoryContainerProvider); - return lookupExtractorFactoryContainerProvider; + @Override + public LookupExtractor get() + { + return new MapLookupExtractor(theLookup, false); + } + } + ); + + return new LookupExtractorFactoryContainerProvider() + { + @Override + public Set getAllLookupNames() + { + return ImmutableSet.of(LOOKYLOO); + } + + @Override + public Optional get(String lookupName) + { + if (LOOKYLOO.equals(lookupName)) { + return Optional.of(container); + } else { + return Optional.empty(); + } + } + }; } } diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupIntrospectionResourceTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupIntrospectionResourceTest.java index 37cab5168826..dd8c84a96dcd 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupIntrospectionResourceTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupIntrospectionResourceTest.java @@ -36,6 +36,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import java.net.URI; +import java.util.Optional; public class LookupIntrospectionResourceTest { @@ -65,19 +66,25 @@ public void setup() throws Exception EasyMock.reset(mockLookupExtractorFactory); EasyMock.reset(mockLookupIntrospectHandler); EasyMock.expect(mockLookupExtractorFactoryContainerProvider.get("lookupId")).andReturn( - new LookupExtractorFactoryContainer( - "v0", - mockLookupExtractorFactory + Optional.of( + new LookupExtractorFactoryContainer( + "v0", + mockLookupExtractorFactory + ) ) ).anyTimes(); EasyMock.expect(mockLookupExtractorFactoryContainerProvider.get("lookupId1")).andReturn( - new LookupExtractorFactoryContainer( - "v0", - actualLookupExtractorFactory + Optional.of( + new LookupExtractorFactoryContainer( + "v0", + actualLookupExtractorFactory + ) ) ).anyTimes(); - EasyMock.expect(mockLookupExtractorFactoryContainerProvider.get(EasyMock.anyString())).andReturn(null).anyTimes(); + EasyMock.expect(mockLookupExtractorFactoryContainerProvider.get(EasyMock.anyString())) + .andReturn(Optional.empty()) + .anyTimes(); EasyMock.replay(mockLookupExtractorFactoryContainerProvider); baseUri = WebserverTestUtils.createBaseUri(); @@ -124,7 +131,8 @@ public void testNotExistingLookup() ); } - @Test public void testExistingLookup() + @Test + public void testExistingLookup() { EasyMock.expect(mockLookupExtractorFactory.getIntrospectHandler()).andReturn(mockLookupIntrospectHandler); EasyMock.expect(mockLookupExtractorFactory.get()) diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index af301055874b..9827197e5854 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import org.apache.druid.discovery.DruidLeaderClient; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.emitter.EmittingLogger; @@ -43,6 +44,7 @@ import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import java.util.concurrent.TimeUnit; public class LookupReferencesManagerTest @@ -183,19 +185,19 @@ public void testAddGetRemove() throws Exception EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); - Assert.assertNull(lookupReferencesManager.get("test")); + Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("test")); LookupExtractorFactoryContainer testContainer = new LookupExtractorFactoryContainer("0", lookupExtractorFactory); lookupReferencesManager.add("test", testContainer); lookupReferencesManager.handlePendingNotices(); - Assert.assertEquals(testContainer, lookupReferencesManager.get("test")); + Assert.assertEquals(Optional.of(testContainer), lookupReferencesManager.get("test")); lookupReferencesManager.remove("test"); lookupReferencesManager.handlePendingNotices(); - Assert.assertNull(lookupReferencesManager.get("test")); + Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("test")); } @Test @@ -289,7 +291,7 @@ public void testGetNotThere() throws Exception EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); - Assert.assertNull(lookupReferencesManager.get("notThere")); + Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("notThere")); } @Test @@ -394,6 +396,43 @@ public void testRemoveNonExisting() throws Exception lookupReferencesManager.handlePendingNotices(); } + @Test + public void testGetAllLookupNames() throws Exception + { + LookupExtractorFactoryContainer container1 = new LookupExtractorFactoryContainer( + "0", + new MapLookupExtractorFactory(ImmutableMap.of("key1", "value1"), true) + ); + + LookupExtractorFactoryContainer container2 = new LookupExtractorFactoryContainer( + "0", + new MapLookupExtractorFactory(ImmutableMap.of("key2", "value2"), true) + ); + Map lookupMap = new HashMap<>(); + String strResult = mapper.writeValueAsString(lookupMap); + Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx")); + EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); + EasyMock.replay(config); + EasyMock.expect( + druidLeaderClient.makeRequest( + HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + ) + ).andReturn(request); + StringFullResponseHolder responseHolder = new StringFullResponseHolder( + HttpResponseStatus.OK, + newEmptyResponse(), + StandardCharsets.UTF_8 + ).addChunk(strResult); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.replay(druidLeaderClient); + lookupReferencesManager.start(); + lookupReferencesManager.add("one", container1); + lookupReferencesManager.add("two", container2); + lookupReferencesManager.handlePendingNotices(); + + Assert.assertEquals(ImmutableSet.of("one", "two"), lookupReferencesManager.getAllLookupNames()); + } + @Test public void testGetAllLookupsState() throws Exception { @@ -495,21 +534,25 @@ public void testRealModeWithMainThread() throws Exception EasyMock.expect(lookupExtractorFactory.start()).andReturn(true).once(); EasyMock.expect(lookupExtractorFactory.destroy()).andReturn(true).once(); EasyMock.replay(lookupExtractorFactory); - Assert.assertNull(lookupReferencesManager.get("test")); + Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("test")); LookupExtractorFactoryContainer testContainer = new LookupExtractorFactoryContainer("0", lookupExtractorFactory); lookupReferencesManager.add("test", testContainer); - while (!testContainer.equals(lookupReferencesManager.get("test"))) { + while (!Optional.of(testContainer).equals(lookupReferencesManager.get("test"))) { Thread.sleep(100); } + Assert.assertEquals(ImmutableSet.of("test"), lookupReferencesManager.getAllLookupNames()); + lookupReferencesManager.remove("test"); - while (lookupReferencesManager.get("test") != null) { + while (lookupReferencesManager.get("test").isPresent()) { Thread.sleep(100); } + Assert.assertEquals(ImmutableSet.of(), lookupReferencesManager.getAllLookupNames()); + lookupReferencesManager.stop(); Assert.assertFalse(lookupReferencesManager.mainThread.isAlive()); @@ -569,9 +612,9 @@ public void testCoordinatorLookupSync() throws Exception EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); - Assert.assertEquals(container1, lookupReferencesManager.get("testLookup1")); - Assert.assertEquals(container2, lookupReferencesManager.get("testLookup2")); - Assert.assertEquals(container3, lookupReferencesManager.get("testLookup3")); + Assert.assertEquals(Optional.of(container1), lookupReferencesManager.get("testLookup1")); + Assert.assertEquals(Optional.of(container2), lookupReferencesManager.get("testLookup2")); + Assert.assertEquals(Optional.of(container3), lookupReferencesManager.get("testLookup3")); } @@ -638,7 +681,10 @@ public int getCoordinatorRetryDelay() EasyMock.expect(druidLeaderClient.go(request)).andThrow(new IllegalStateException()).anyTimes(); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); - Assert.assertEquals(container, lookupReferencesManager.get("testMockForLoadLookupOnCoordinatorFailure")); + Assert.assertEquals( + Optional.of(container), + lookupReferencesManager.get("testMockForLoadLookupOnCoordinatorFailure") + ); } @Test @@ -677,6 +723,6 @@ public boolean getEnableLookupSyncOnStartup() EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); lookupReferencesManager.start(); - Assert.assertNull(lookupReferencesManager.get("testMockForDisableLookupSync")); + Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("testMockForDisableLookupSync")); } } diff --git a/server/src/test/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFnTest.java b/server/src/test/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFnTest.java index 147f79cc5c74..5e877e8288cd 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFnTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/RegisteredLookupExtractionFnTest.java @@ -34,6 +34,7 @@ import javax.annotation.Nullable; import java.util.Arrays; import java.util.Map; +import java.util.Optional; public class RegisteredLookupExtractionFnTest { @@ -96,7 +97,7 @@ public void testInheritInjective() public void testMissingDelegation() { final LookupExtractorFactoryContainerProvider manager = EasyMock.createStrictMock(LookupReferencesManager.class); - EasyMock.expect(manager.get(EasyMock.eq(LOOKUP_NAME))).andReturn(null).once(); + EasyMock.expect(manager.get(EasyMock.eq(LOOKUP_NAME))).andReturn(Optional.empty()).once(); EasyMock.replay(manager); expectedException.expectMessage("Lookup [some lookup] not found"); @@ -147,7 +148,10 @@ public void testSerDe() throws Exception ); EasyMock.verify(manager); - final Map result = mapper.readValue(mapper.writeValueAsString(fn), JacksonUtils.TYPE_REFERENCE_MAP_STRING_OBJECT); + final Map result = mapper.readValue( + mapper.writeValueAsString(fn), + JacksonUtils.TYPE_REFERENCE_MAP_STRING_OBJECT + ); Assert.assertEquals(mapper.convertValue(fn, JacksonUtils.TYPE_REFERENCE_MAP_STRING_OBJECT), result); Assert.assertEquals(LOOKUP_NAME, result.get("lookup")); Assert.assertEquals(true, result.get("retainMissingValue")); @@ -248,41 +252,43 @@ public void testEquals() private void managerReturnsMap(LookupExtractorFactoryContainerProvider manager) { EasyMock.expect(manager.get(EasyMock.eq(LOOKUP_NAME))).andReturn( - new LookupExtractorFactoryContainer( - "v0", - new LookupExtractorFactory() - { - @Override - public boolean start() - { - return false; - } + Optional.of( + new LookupExtractorFactoryContainer( + "v0", + new LookupExtractorFactory() + { + @Override + public boolean start() + { + return false; + } - @Override - public boolean replaces(@Nullable LookupExtractorFactory other) - { - return false; - } + @Override + public boolean replaces(@Nullable LookupExtractorFactory other) + { + return false; + } - @Override - public boolean close() - { - return false; - } + @Override + public boolean close() + { + return false; + } - @Nullable - @Override - public LookupIntrospectHandler getIntrospectHandler() - { - return null; - } + @Nullable + @Override + public LookupIntrospectHandler getIntrospectHandler() + { + return null; + } - @Override - public LookupExtractor get() - { - return LOOKUP_EXTRACTOR; - } - } + @Override + public LookupExtractor get() + { + return LOOKUP_EXTRACTOR; + } + } + ) ) ).anyTimes(); } diff --git a/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java b/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java new file mode 100644 index 000000000000..95409ee383b8 --- /dev/null +++ b/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java @@ -0,0 +1,127 @@ +/* + * 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.segment.join; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.query.LookupDataSource; +import org.apache.druid.query.TableDataSource; +import org.apache.druid.query.extraction.MapLookupExtractor; +import org.apache.druid.query.lookup.LookupExtractorFactoryContainer; +import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; +import org.apache.druid.query.lookup.MapLookupExtractorFactory; +import org.apache.druid.segment.join.lookup.LookupJoinable; +import org.hamcrest.CoreMatchers; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; +import java.util.Optional; +import java.util.Set; + +public class LookupJoinableFactoryTest +{ + private static final String PREFIX = "j."; + + private final LookupJoinableFactory factory; + private final LookupDataSource lookupDataSource = new LookupDataSource("country_code_to_name"); + + public LookupJoinableFactoryTest() + { + try { + final MapLookupExtractor countryIsoCodeToNameLookup = JoinTestHelper.createCountryIsoCodeToNameLookup(); + this.factory = new LookupJoinableFactory( + new LookupExtractorFactoryContainerProvider() + { + @Override + public Set getAllLookupNames() + { + return ImmutableSet.of(lookupDataSource.getLookupName()); + } + + @Override + public Optional get(String lookupName) + { + if (lookupDataSource.getLookupName().equals(lookupName)) { + return Optional.of( + new LookupExtractorFactoryContainer( + "v0", + new MapLookupExtractorFactory( + countryIsoCodeToNameLookup.getMap(), + false + ) + ) + ); + } else { + return Optional.empty(); + } + } + } + ); + } + catch (IOException e) { + throw new RuntimeException(e); + } + } + + @Test + public void testBuildNonLookup() + { + Assert.assertEquals( + Optional.empty(), + factory.build(new TableDataSource("foo"), makeCondition("x == \"j.k\"")) + ); + } + + @Test + public void testBuildNonHashJoin() + { + Assert.assertEquals( + Optional.empty(), + factory.build(lookupDataSource, makeCondition("x > \"j.k\"")) + ); + } + + @Test + public void testBuildDifferentLookup() + { + Assert.assertEquals( + Optional.empty(), + factory.build(new LookupDataSource("beep"), makeCondition("x == \"j.k\"")) + ); + } + + @Test + public void testBuild() + { + final Joinable joinable = factory.build(lookupDataSource, makeCondition("x == \"j.k\"")).get(); + + Assert.assertThat(joinable, CoreMatchers.instanceOf(LookupJoinable.class)); + Assert.assertEquals(ImmutableList.of("k", "v"), joinable.getAvailableColumns()); + Assert.assertEquals(Joinable.CARDINALITY_UNKNOWN, joinable.getCardinality("k")); + Assert.assertEquals(Joinable.CARDINALITY_UNKNOWN, joinable.getCardinality("v")); + } + + private static JoinConditionAnalysis makeCondition(final String condition) + { + return JoinConditionAnalysis.forExpression(condition, PREFIX, ExprMacroTable.nil()); + } +} diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java index 2f1626d2da58..5c225b4666b5 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java @@ -222,15 +222,15 @@ public AuthenticationResult createEscalatedAuthenticationResult() // This Module is just to get a LookupExtractorFactoryContainerProvider with a usable "lookyloo" lookup. - binder.bind(LookupExtractorFactoryContainerProvider.class).toInstance( - LookupEnabledTestExprMacroTable.createTestLookupReferencesManager( + final LookupExtractorFactoryContainerProvider lookupProvider = + LookupEnabledTestExprMacroTable.createTestLookupProvider( ImmutableMap.of( "a", "xa", - "abc", "xabc" + "abc", "xabc", + "nosuchkey", "mysteryvalue" ) - ) - ); - + ); + binder.bind(LookupExtractorFactoryContainerProvider.class).toInstance(lookupProvider); } ); From be6feecedad317e8087ddaccb96fb3ff916a4502 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 28 Jan 2020 19:10:34 -0800 Subject: [PATCH 2/6] Fixes for tests. --- .../guice/JoinableFactoryModuleTest.java | 27 +++++++++++-------- .../druid/query/expression/ExprMacroTest.java | 9 ++++++- .../lookup/LookupReferencesManagerTest.java | 14 ++++++---- .../join/InlineJoinableFactoryTest.java | 13 ++++++--- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java b/server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java index 70fd39214f90..47c34fd9cef2 100644 --- a/server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java +++ b/server/src/test/java/org/apache/druid/guice/JoinableFactoryModuleTest.java @@ -28,6 +28,8 @@ import com.google.inject.TypeLiteral; import org.apache.druid.query.DataSource; import org.apache.druid.query.InlineDataSource; +import org.apache.druid.query.expression.LookupEnabledTestExprMacroTable; +import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; import org.apache.druid.segment.join.InlineJoinableFactory; import org.apache.druid.segment.join.JoinableFactory; import org.apache.druid.segment.join.MapJoinableFactory; @@ -37,6 +39,7 @@ import org.junit.Before; import org.junit.Test; +import java.util.Collections; import java.util.Map; public class JoinableFactoryModuleTest @@ -63,7 +66,7 @@ public void testInjectDefaultBindingsShouldBeInjected() { Map, JoinableFactory> joinableFactories = injector.getInstance(Key.get(new TypeLiteral, JoinableFactory>>() {})); - Assert.assertEquals(1, joinableFactories.size()); + Assert.assertEquals(2, joinableFactories.size()); Assert.assertEquals(InlineJoinableFactory.class, joinableFactories.get(InlineDataSource.class).getClass()); } @@ -75,23 +78,25 @@ public void testJoinableFactoryCanBind() .joinableFactoryBinder(binder).addBinding(NoopDataSource.class).toInstance(NoopJoinableFactory.INSTANCE)); Map, JoinableFactory> joinableFactories = injector.getInstance(Key.get(new TypeLiteral, JoinableFactory>>() {})); - Assert.assertEquals(2, joinableFactories.size()); + Assert.assertEquals(3, joinableFactories.size()); Assert.assertEquals(NoopJoinableFactory.INSTANCE, joinableFactories.get(NoopDataSource.class)); } private Injector makeInjectorWithProperties(Module... otherModules) { - ImmutableList.Builder modulesBuilder = + final LookupExtractorFactoryContainerProvider lookupProvider = + LookupEnabledTestExprMacroTable.createTestLookupProvider(Collections.emptyMap()); + + final ImmutableList.Builder modulesBuilder = ImmutableList.builder() - .add(new JoinableFactoryModule()) - .add(binder -> { - binder.bindScope(LazySingleton.class, Scopes.SINGLETON); - }); - for (Module otherModule : otherModules) { + .add(new JoinableFactoryModule()) + .add(binder -> binder.bind(LookupExtractorFactoryContainerProvider.class).toInstance(lookupProvider)) + .add(binder -> binder.bindScope(LazySingleton.class, Scopes.SINGLETON)); + + for (final Module otherModule : otherModules) { modulesBuilder.add(otherModule); } - return Guice.createInjector( - modulesBuilder.build() - ); + + return Guice.createInjector(modulesBuilder.build()); } } diff --git a/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java b/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java index 74e74394570a..abe34d0a2eb9 100644 --- a/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java +++ b/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java @@ -25,6 +25,7 @@ import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.Parser; import org.junit.Assert; +import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -51,6 +52,12 @@ public class ExprMacroTest .build() ); + @BeforeClass + public static void setUpClass() + { + NullHandling.initializeForTests(); + } + @Rule public ExpectedException expectedException = ExpectedException.none(); @@ -75,7 +82,7 @@ public void testLookup() @Test public void testLookupNotFound() { - expectedException.expect(NullPointerException.class); + expectedException.expect(IllegalStateException.class); expectedException.expectMessage("Lookup [lookylook] not found"); assertExpr("lookup(x, 'lookylook')", null); } diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index 9827197e5854..0c9b0a66c558 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -414,9 +414,7 @@ public void testGetAllLookupNames() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect( - druidLeaderClient.makeRequest( - HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - ) + druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true") ).andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( HttpResponseStatus.OK, @@ -543,7 +541,10 @@ public void testRealModeWithMainThread() throws Exception Thread.sleep(100); } - Assert.assertEquals(ImmutableSet.of("test"), lookupReferencesManager.getAllLookupNames()); + Assert.assertEquals( + ImmutableSet.of("test", "testMockForRealModeWithMainThread"), + lookupReferencesManager.getAllLookupNames() + ); lookupReferencesManager.remove("test"); @@ -551,7 +552,10 @@ public void testRealModeWithMainThread() throws Exception Thread.sleep(100); } - Assert.assertEquals(ImmutableSet.of(), lookupReferencesManager.getAllLookupNames()); + Assert.assertEquals( + ImmutableSet.of("testMockForRealModeWithMainThread"), + lookupReferencesManager.getAllLookupNames() + ); lookupReferencesManager.stop(); diff --git a/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java b/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java index 827f54701710..5e39d51dcb97 100644 --- a/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java +++ b/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java @@ -27,7 +27,9 @@ import org.apache.druid.segment.join.table.IndexedTableJoinable; import org.hamcrest.CoreMatchers; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.util.Optional; @@ -35,6 +37,9 @@ public class InlineJoinableFactoryTest { private static final String PREFIX = "j."; + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private final InlineJoinableFactory factory = new InlineJoinableFactory(); private final InlineDataSource inlineDataSource = InlineDataSource.fromIterable( @@ -49,10 +54,12 @@ public class InlineJoinableFactoryTest @Test public void testBuildNonInline() { - Assert.assertEquals( - Optional.empty(), - factory.build(new TableDataSource("foo"), makeCondition("x == \"j.y\"")) + expectedException.expect(ClassCastException.class); + expectedException.expectMessage( + "org.apache.druid.query.TableDataSource cannot be cast to org.apache.druid.query.InlineDataSource" ); + + final Optional ignored = factory.build(new TableDataSource("foo"), makeCondition("x == \"j.y\"")); } @Test From 4a11b6df5794db7a986111680103745120b71bd4 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 28 Jan 2020 20:06:07 -0800 Subject: [PATCH 3/6] Fix another test. --- .../segment/join/LookupJoinableFactoryTest.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java b/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java index 95409ee383b8..0e7c25f4d34a 100644 --- a/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java +++ b/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java @@ -31,7 +31,9 @@ import org.apache.druid.segment.join.lookup.LookupJoinable; import org.hamcrest.CoreMatchers; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.io.IOException; import java.util.Optional; @@ -41,6 +43,9 @@ public class LookupJoinableFactoryTest { private static final String PREFIX = "j."; + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private final LookupJoinableFactory factory; private final LookupDataSource lookupDataSource = new LookupDataSource("country_code_to_name"); @@ -85,10 +90,12 @@ public Optional get(String lookupName) @Test public void testBuildNonLookup() { - Assert.assertEquals( - Optional.empty(), - factory.build(new TableDataSource("foo"), makeCondition("x == \"j.k\"")) + expectedException.expect(ClassCastException.class); + expectedException.expectMessage( + "org.apache.druid.query.TableDataSource cannot be cast to org.apache.druid.query.LookupDataSource" ); + + final Optional ignored = factory.build(new TableDataSource("foo"), makeCondition("x == \"j.k\"")); } @Test From 05e76b884ce170871ec8ef78d1d91740f008fb51 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 28 Jan 2020 22:10:02 -0800 Subject: [PATCH 4/6] Java 11 message fix. --- .../apache/druid/segment/join/InlineJoinableFactoryTest.java | 4 +--- .../apache/druid/segment/join/LookupJoinableFactoryTest.java | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java b/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java index 5e39d51dcb97..50a4a93a8e3a 100644 --- a/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java +++ b/server/src/test/java/org/apache/druid/segment/join/InlineJoinableFactoryTest.java @@ -55,9 +55,7 @@ public class InlineJoinableFactoryTest public void testBuildNonInline() { expectedException.expect(ClassCastException.class); - expectedException.expectMessage( - "org.apache.druid.query.TableDataSource cannot be cast to org.apache.druid.query.InlineDataSource" - ); + expectedException.expectMessage("TableDataSource cannot be cast"); final Optional ignored = factory.build(new TableDataSource("foo"), makeCondition("x == \"j.y\"")); } diff --git a/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java b/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java index 0e7c25f4d34a..44ed4b2810af 100644 --- a/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java +++ b/server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java @@ -91,9 +91,7 @@ public Optional get(String lookupName) public void testBuildNonLookup() { expectedException.expect(ClassCastException.class); - expectedException.expectMessage( - "org.apache.druid.query.TableDataSource cannot be cast to org.apache.druid.query.LookupDataSource" - ); + expectedException.expectMessage("TableDataSource cannot be cast"); final Optional ignored = factory.build(new TableDataSource("foo"), makeCondition("x == \"j.k\"")); } From 06122ea779f3e0df89ae0881b3e4b89c70295d21 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 29 Jan 2020 07:10:19 -0800 Subject: [PATCH 5/6] Fixups. --- .../java/org/apache/druid/segment/join/JoinableFactory.java | 2 -- .../druid/query/lookup/LookupReferencesManagerTest.java | 5 +++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java index ae56bc289aa5..5016d75a79c5 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java +++ b/processing/src/main/java/org/apache/druid/segment/join/JoinableFactory.java @@ -25,8 +25,6 @@ /** * Utility for creating {@link Joinable} objects. - * - * @see org.apache.druid.guice.DruidBinders#joinableFactoryBinder to register factories */ public interface JoinableFactory { diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index 0c9b0a66c558..3b953da096d8 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -429,6 +429,11 @@ public void testGetAllLookupNames() throws Exception lookupReferencesManager.handlePendingNotices(); Assert.assertEquals(ImmutableSet.of("one", "two"), lookupReferencesManager.getAllLookupNames()); + + Assert.assertEquals( + ImmutableSet.of("one", "two"), + ((LookupExtractorFactoryContainerProvider) lookupReferencesManager).getAllLookupNames() + ); } @Test From 33d7908c0bd956129530839cd18b80321c05d768 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Thu, 30 Jan 2020 09:05:50 -0800 Subject: [PATCH 6/6] Fixup benchmark class. --- .../benchmark/JoinAndLookupBenchmark.java | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/benchmarks/src/main/java/org/apache/druid/benchmark/JoinAndLookupBenchmark.java b/benchmarks/src/main/java/org/apache/druid/benchmark/JoinAndLookupBenchmark.java index c57459ce1e20..ca6852fec2e0 100644 --- a/benchmarks/src/main/java/org/apache/druid/benchmark/JoinAndLookupBenchmark.java +++ b/benchmarks/src/main/java/org/apache/druid/benchmark/JoinAndLookupBenchmark.java @@ -20,6 +20,7 @@ package org.apache.druid.benchmark; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.FileUtils; import org.apache.druid.java.util.common.Intervals; @@ -31,6 +32,7 @@ import org.apache.druid.query.expression.LookupExprMacro; import org.apache.druid.query.filter.SelectorDimFilter; import org.apache.druid.query.lookup.LookupExtractorFactoryContainer; +import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; import org.apache.druid.query.lookup.MapLookupExtractorFactory; import org.apache.druid.segment.Cursor; import org.apache.druid.segment.DimensionSelector; @@ -67,6 +69,8 @@ import java.io.File; import java.io.IOException; import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.concurrent.TimeUnit; @State(Scope.Benchmark) @@ -190,19 +194,34 @@ public void setup() throws IOException final ExprMacroTable exprMacroTable = new ExprMacroTable( ImmutableList.of( new LookupExprMacro( - lookupName -> { - if (LOOKUP_COUNTRY_CODE_TO_NAME.equals(lookupName)) { - return new LookupExtractorFactoryContainer( - "0", - new MapLookupExtractorFactory(countryCodeToNameMap, false) - ); - } else if (LOOKUP_COUNTRY_NUMBER_TO_NAME.equals(lookupName)) { - return new LookupExtractorFactoryContainer( - "0", - new MapLookupExtractorFactory(countryNumberToNameMap, false) - ); - } else { - return null; + new LookupExtractorFactoryContainerProvider() + { + @Override + public Set getAllLookupNames() + { + return ImmutableSet.of(LOOKUP_COUNTRY_CODE_TO_NAME, LOOKUP_COUNTRY_NUMBER_TO_NAME); + } + + @Override + public Optional get(String lookupName) + { + if (LOOKUP_COUNTRY_CODE_TO_NAME.equals(lookupName)) { + return Optional.of( + new LookupExtractorFactoryContainer( + "0", + new MapLookupExtractorFactory(countryCodeToNameMap, false) + ) + ); + } else if (LOOKUP_COUNTRY_NUMBER_TO_NAME.equals(lookupName)) { + return Optional.of( + new LookupExtractorFactoryContainer( + "0", + new MapLookupExtractorFactory(countryNumberToNameMap, false) + ) + ); + } else { + return Optional.empty(); + } } } )