From 3a92dabdc2a1c5da393124223d80e67ff2417844 Mon Sep 17 00:00:00 2001 From: dwylie Date: Mon, 23 Apr 2018 22:24:18 +0100 Subject: [PATCH 1/3] Use a bimap for reverse lookups on injective maps - A BiMap provides constant-time lookups for mapping values to keys --- .../query/extraction/MapLookupExtractor.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java b/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java index b818bd8cd50c..9b39b715fda8 100644 --- a/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java +++ b/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java @@ -26,6 +26,7 @@ import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.base.Throwables; +import com.google.common.collect.HashBiMap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -52,8 +53,8 @@ public MapLookupExtractor( @JsonProperty("isOneToOne") boolean isOneToOne ) { - this.map = Preconditions.checkNotNull(map, "map"); this.isOneToOne = isOneToOne; + this.map = Preconditions.checkNotNull(this.isOneToOne ? HashBiMap.create(map) : map, "map"); } @JsonProperty @@ -72,13 +73,19 @@ public String apply(@NotNull String val) @Override public List unapply(final String value) { - return Lists.newArrayList(Maps.filterKeys(map, new Predicate() - { - @Override public boolean apply(@Nullable String key) + if (this.map instanceof HashBiMap) { + return Lists.newArrayList(((HashBiMap) this.map).inverse().get(value)); + } else { + return Lists.newArrayList(Maps.filterKeys(map, new Predicate() { - return map.get(key).equals(Strings.nullToEmpty(value)); - } - }).keySet()); + @Override + public boolean apply(@Nullable String key) + { + return map.get(key).equals(Strings.nullToEmpty(value)); + } + }).keySet()); + } + } From 842af5e91491cd8a08e96c70a7e373aa2a1ba14d Mon Sep 17 00:00:00 2001 From: dwylie Date: Sun, 29 Apr 2018 21:18:42 +0100 Subject: [PATCH 2/3] Address comments --- .../query/extraction/MapLookupExtractor.java | 33 +++++++++++-------- .../druid/query/lookup/LookupExtractor.java | 2 +- .../extraction/MapLookupExtractorTest.java | 20 +++++++---- 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java b/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java index 9b39b715fda8..aed6beea3c09 100644 --- a/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java +++ b/processing/src/main/java/io/druid/query/extraction/MapLookupExtractor.java @@ -23,7 +23,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonTypeName; import com.google.common.base.Preconditions; -import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.base.Throwables; import com.google.common.collect.HashBiMap; @@ -37,6 +36,7 @@ import javax.validation.constraints.NotNull; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -44,6 +44,7 @@ public class MapLookupExtractor extends LookupExtractor { private final Map map; + private final Map reverseMap; private final boolean isOneToOne; @@ -53,8 +54,18 @@ public MapLookupExtractor( @JsonProperty("isOneToOne") boolean isOneToOne ) { + + Preconditions.checkNotNull(map, "map"); + this.isOneToOne = isOneToOne; - this.map = Preconditions.checkNotNull(this.isOneToOne ? HashBiMap.create(map) : map, "map"); + + if (this.isOneToOne) { + this.map = HashBiMap.create(map); + this.reverseMap = ((HashBiMap) this.map).inverse(); + } else { + this.map = map; + this.reverseMap = null; + } } @JsonProperty @@ -73,20 +84,14 @@ public String apply(@NotNull String val) @Override public List unapply(final String value) { - if (this.map instanceof HashBiMap) { - return Lists.newArrayList(((HashBiMap) this.map).inverse().get(value)); + String valueToLookup = Strings.nullToEmpty(value); + + if (this.reverseMap != null) { + String val = this.reverseMap.get(valueToLookup); + return (val != null) ? Collections.singletonList(val) : Collections.emptyList(); } else { - return Lists.newArrayList(Maps.filterKeys(map, new Predicate() - { - @Override - public boolean apply(@Nullable String key) - { - return map.get(key).equals(Strings.nullToEmpty(value)); - } - }).keySet()); + return Lists.newArrayList(Maps.filterKeys(map, key -> map.get(key).equals(valueToLookup)).keySet()); } - - } @Override diff --git a/processing/src/main/java/io/druid/query/lookup/LookupExtractor.java b/processing/src/main/java/io/druid/query/lookup/LookupExtractor.java index 0400c21ae1e6..b77290ae0cab 100644 --- a/processing/src/main/java/io/druid/query/lookup/LookupExtractor.java +++ b/processing/src/main/java/io/druid/query/lookup/LookupExtractor.java @@ -74,7 +74,7 @@ public Map applyAll(Iterable keys) * Null and empty are considered to be the same value = nullToEmpty(value) * * @return the list of keys that maps to value or empty list. - * Note that for the case of a none existing value in the lookup we have to cases either return an empty list OR list with null element. + * Note that for the case of a none existing value in the lookup we have two cases either return an empty list OR list with null element. * returning an empty list implies that user want to ignore such a lookup value. * In the other hand returning a list with the null element implies user want to map the none existing value to the key null. */ diff --git a/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java b/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java index 870812dcac4b..3b62a9005cfb 100644 --- a/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java +++ b/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java @@ -20,6 +20,7 @@ package io.druid.query.extraction; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; import com.google.common.collect.Sets; import org.junit.Assert; import org.junit.Test; @@ -33,17 +34,22 @@ public class MapLookupExtractorTest { private final Map lookupMap = ImmutableMap.of("foo", "bar", "null", "", "empty String", "", "", "empty_string"); private final MapLookupExtractor fn = new MapLookupExtractor(lookupMap, false); + private final MapLookupExtractor fnOneToOne = new MapLookupExtractor(lookupMap, true); @Test public void testUnApply() { - Assert.assertEquals(Arrays.asList("foo"), fn.unapply("bar")); - Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(fn.unapply(""))); - Assert.assertEquals("Null value should be equal to empty string", - Sets.newHashSet("null", "empty String"), - Sets.newHashSet(fn.unapply((String) null))); - Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(fn.unapply("empty_string"))); - Assert.assertEquals("not existing value returns empty list", Collections.EMPTY_LIST, fn.unapply("not There")); + for (MapLookupExtractor fn : Lists.newArrayList(fn, fnOneToOne)) { + Assert.assertEquals(Arrays.asList("foo"), fn.unapply("bar")); + Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(fn.unapply(""))); + Assert.assertEquals( + "Null value should be equal to empty string", + Sets.newHashSet("null", "empty String"), + Sets.newHashSet(fn.unapply((String) null)) + ); + Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(fn.unapply("empty_string"))); + Assert.assertEquals("not existing value returns empty list", Collections.EMPTY_LIST, fn.unapply("not There")); + } } @Test From b87b1ddb32230a6ff57973eada40b9e1412a3276 Mon Sep 17 00:00:00 2001 From: dwylie Date: Sun, 29 Apr 2018 21:43:02 +0100 Subject: [PATCH 3/3] Fix Tests --- .../extraction/MapLookupExtractorTest.java | 39 ++++++++++++------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java b/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java index 3b62a9005cfb..03d93765bc4f 100644 --- a/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java +++ b/processing/src/test/java/io/druid/query/extraction/MapLookupExtractorTest.java @@ -20,7 +20,6 @@ package io.druid.query.extraction; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Lists; import com.google.common.collect.Sets; import org.junit.Assert; import org.junit.Test; @@ -34,22 +33,34 @@ public class MapLookupExtractorTest { private final Map lookupMap = ImmutableMap.of("foo", "bar", "null", "", "empty String", "", "", "empty_string"); private final MapLookupExtractor fn = new MapLookupExtractor(lookupMap, false); - private final MapLookupExtractor fnOneToOne = new MapLookupExtractor(lookupMap, true); @Test - public void testUnApply() + public void testNonInjectiveUnApply() { - for (MapLookupExtractor fn : Lists.newArrayList(fn, fnOneToOne)) { - Assert.assertEquals(Arrays.asList("foo"), fn.unapply("bar")); - Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(fn.unapply(""))); - Assert.assertEquals( - "Null value should be equal to empty string", - Sets.newHashSet("null", "empty String"), - Sets.newHashSet(fn.unapply((String) null)) - ); - Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(fn.unapply("empty_string"))); - Assert.assertEquals("not existing value returns empty list", Collections.EMPTY_LIST, fn.unapply("not There")); - } + Assert.assertEquals(Arrays.asList("foo"), fn.unapply("bar")); + Assert.assertEquals(Sets.newHashSet("null", "empty String"), Sets.newHashSet(fn.unapply(""))); + Assert.assertEquals("Null value should be equal to empty string", + Sets.newHashSet("null", "empty String"), + Sets.newHashSet(fn.unapply((String) null))); + Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(fn.unapply("empty_string"))); + Assert.assertEquals("not existing value returns empty list", Collections.EMPTY_LIST, fn.unapply("not There")); + } + + @Test + public void testInjectiveUnApply() + { + MapLookupExtractor injectiveFn = new MapLookupExtractor( + ImmutableMap.of("foo", "bar", "null", "", "", "empty_string"), true + ); + + Assert.assertEquals(Arrays.asList("foo"), injectiveFn.unapply("bar")); + Assert.assertEquals( + "Null value should be equal to empty string", + Sets.newHashSet("null"), + Sets.newHashSet(injectiveFn.unapply((String) null)) + ); + Assert.assertEquals(Sets.newHashSet(""), Sets.newHashSet(injectiveFn.unapply("empty_string"))); + Assert.assertEquals("not existing value returns empty list", Collections.EMPTY_LIST, injectiveFn.unapply("not There")); } @Test