From b6e4d8b2c1413293f28d340f904f1460535b01b7 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 1 Apr 2016 14:19:54 -0700 Subject: [PATCH] Various ExtractionFn null handling fixes. - JavaScriptExtractionFn shouldn't pass empty strings to its JS functions - Upper/LowerExtractionFn properly handles null Objects (DimExtractionFn's implementation works here) - MatchingDimExtractionFn properly returns nulls rather than empties - RegexDimExtractionFn properly attempts matching on nulls and empties - SearchQuerySpecDimExtractionFn properly returns nulls when passed empties --- .../druid/query/extraction/ExtractionFn.java | 22 ++++++++++++++++++- .../extraction/JavaScriptExtractionFn.java | 2 +- .../query/extraction/LowerExtractionFn.java | 14 +----------- .../extraction/MatchingDimExtractionFn.java | 6 ++--- .../extraction/RegexDimExtractionFn.java | 7 ++---- .../SearchQuerySpecDimExtractionFn.java | 3 ++- .../query/extraction/UpperExtractionFn.java | 14 +----------- .../extraction/LowerExtractionFnTest.java | 2 ++ .../MatchingDimExtractionFnTest.java | 11 ++++++++++ .../extraction/RegexDimExtractionFnTest.java | 21 +++++++++++++++++- .../extraction/UpperExtractionFnTest.java | 2 ++ 11 files changed, 66 insertions(+), 38 deletions(-) diff --git a/processing/src/main/java/io/druid/query/extraction/ExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/ExtractionFn.java index fd1de09fd44c..d59581ab9f69 100644 --- a/processing/src/main/java/io/druid/query/extraction/ExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/ExtractionFn.java @@ -59,7 +59,7 @@ public interface ExtractionFn public byte[] getCacheKey(); /** - * The "extraction" function. This should map a value into some other String value. + * The "extraction" function. This should map an Object into some String value. *

* In order to maintain the "null and empty string are equivalent" semantics that Druid provides, the * empty string is considered invalid output for this method and should instead return null. This is @@ -72,8 +72,28 @@ public interface ExtractionFn */ public String apply(Object value); + /** + * The "extraction" function. This should map a String value into some other String value. + *

+ * Like {@link #apply(Object)}, the empty string is considered invalid output for this method and it should + * instead return null. + * + * @param value the original value of the dimension + * + * @return a value that should be used instead of the original + */ public String apply(String value); + /** + * The "extraction" function. This should map a long value into some String value. + *

+ * Like {@link #apply(Object)}, the empty string is considered invalid output for this method and it should + * instead return null. + * + * @param value the original value of the dimension + * + * @return a value that should be used instead of the original + */ public String apply(long value); /** diff --git a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java index 0263ddc10aa9..a6b3ac08110d 100644 --- a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java @@ -109,7 +109,7 @@ public String apply(Object value) @Override public String apply(String value) { - return this.apply((Object) value); + return this.apply((Object) Strings.emptyToNull(value)); } @Override diff --git a/processing/src/main/java/io/druid/query/extraction/LowerExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/LowerExtractionFn.java index 4f3b87b8dbf7..64ebfe0f9dbb 100644 --- a/processing/src/main/java/io/druid/query/extraction/LowerExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/LowerExtractionFn.java @@ -31,7 +31,7 @@ import java.util.Locale; @JsonTypeName("lower") -public class LowerExtractionFn implements ExtractionFn +public class LowerExtractionFn extends DimExtractionFn { private final Locale locale; @@ -60,12 +60,6 @@ public String apply(String key) return key.toLowerCase(locale); } - @Override - public String apply(long value) - { - return apply(String.valueOf(value)); - } - @Override public boolean preservesOrdering() { @@ -88,10 +82,4 @@ public byte[] getCacheKey() .put(localeBytes) .array(); } - - @Override - public String apply(Object value) - { - return apply(String.valueOf(value)); - } } diff --git a/processing/src/main/java/io/druid/query/extraction/MatchingDimExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/MatchingDimExtractionFn.java index c69edd58c638..ae2b6ddb9e32 100644 --- a/processing/src/main/java/io/druid/query/extraction/MatchingDimExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/MatchingDimExtractionFn.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; +import com.google.common.base.Strings; import com.metamx.common.StringUtils; import java.nio.ByteBuffer; @@ -59,9 +60,8 @@ public byte[] getCacheKey() @Override public String apply(String dimValue) { - dimValue = (dimValue == null) ? "" : dimValue; - Matcher matcher = pattern.matcher(dimValue); - return matcher.find() ? dimValue : null; + Matcher matcher = pattern.matcher(Strings.nullToEmpty(dimValue)); + return matcher.find() ? Strings.emptyToNull(dimValue) : null; } @JsonProperty("expr") diff --git a/processing/src/main/java/io/druid/query/extraction/RegexDimExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/RegexDimExtractionFn.java index 800f734f9458..be2600ec6487 100644 --- a/processing/src/main/java/io/druid/query/extraction/RegexDimExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/RegexDimExtractionFn.java @@ -86,11 +86,8 @@ public byte[] getCacheKey() @Override public String apply(String dimValue) { - if (dimValue == null) { - return null; - } - String retVal; - Matcher matcher = pattern.matcher(dimValue); + final String retVal; + final Matcher matcher = pattern.matcher(Strings.nullToEmpty(dimValue)); if (matcher.find()) { retVal = matcher.group(1); } else { diff --git a/processing/src/main/java/io/druid/query/extraction/SearchQuerySpecDimExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/SearchQuerySpecDimExtractionFn.java index 31f0ccabb628..77ba930680ea 100644 --- a/processing/src/main/java/io/druid/query/extraction/SearchQuerySpecDimExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/SearchQuerySpecDimExtractionFn.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; +import com.google.common.base.Strings; import io.druid.query.search.search.SearchQuerySpec; import java.nio.ByteBuffer; @@ -61,7 +62,7 @@ public byte[] getCacheKey() @Override public String apply(String dimValue) { - return searchQuerySpec.accept(dimValue) ? dimValue : null; + return searchQuerySpec.accept(dimValue) ? Strings.emptyToNull(dimValue) : null; } @Override diff --git a/processing/src/main/java/io/druid/query/extraction/UpperExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/UpperExtractionFn.java index 7d6ded1e8a4a..c43891fcb7d2 100644 --- a/processing/src/main/java/io/druid/query/extraction/UpperExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/UpperExtractionFn.java @@ -31,7 +31,7 @@ import java.util.Locale; @JsonTypeName("upper") -public class UpperExtractionFn implements ExtractionFn +public class UpperExtractionFn extends DimExtractionFn { private final Locale locale; @@ -59,12 +59,6 @@ public String apply(String key) return key.toUpperCase(locale); } - @Override - public String apply(long value) - { - return apply(String.valueOf(value)); - } - @Override public boolean preservesOrdering() { @@ -87,10 +81,4 @@ public byte[] getCacheKey() .put(localeBytes) .array(); } - - @Override - public String apply(Object value) - { - return apply(String.valueOf(value)); - } } diff --git a/processing/src/test/java/io/druid/query/extraction/LowerExtractionFnTest.java b/processing/src/test/java/io/druid/query/extraction/LowerExtractionFnTest.java index a239c4347cbf..8ffbfd1a8ae7 100644 --- a/processing/src/test/java/io/druid/query/extraction/LowerExtractionFnTest.java +++ b/processing/src/test/java/io/druid/query/extraction/LowerExtractionFnTest.java @@ -35,6 +35,8 @@ public void testApply() Assert.assertEquals("lower 1 string", extractionFn.apply("lOwER 1 String")); Assert.assertEquals(null, extractionFn.apply("")); Assert.assertEquals(null, extractionFn.apply(null)); + Assert.assertEquals(null, extractionFn.apply((Object)null)); + Assert.assertEquals("1", extractionFn.apply(1)); } @Test diff --git a/processing/src/test/java/io/druid/query/extraction/MatchingDimExtractionFnTest.java b/processing/src/test/java/io/druid/query/extraction/MatchingDimExtractionFnTest.java index 5af52c4a967e..4415056f542a 100644 --- a/processing/src/test/java/io/druid/query/extraction/MatchingDimExtractionFnTest.java +++ b/processing/src/test/java/io/druid/query/extraction/MatchingDimExtractionFnTest.java @@ -67,6 +67,17 @@ public void testExtraction() } } + @Test + public void testNullExtraction() + { + String regex = "^$"; + ExtractionFn extractionFn = new MatchingDimExtractionFn(regex); + + Assert.assertNull(extractionFn.apply((Object) null)); + Assert.assertNull(extractionFn.apply((String) null)); + Assert.assertNull(extractionFn.apply((String) "")); + } + @Test public void testSerde() throws Exception { diff --git a/processing/src/test/java/io/druid/query/extraction/RegexDimExtractionFnTest.java b/processing/src/test/java/io/druid/query/extraction/RegexDimExtractionFnTest.java index 6b745345e170..080501ea061b 100644 --- a/processing/src/test/java/io/druid/query/extraction/RegexDimExtractionFnTest.java +++ b/processing/src/test/java/io/druid/query/extraction/RegexDimExtractionFnTest.java @@ -102,7 +102,6 @@ public void testStringExtraction() Assert.assertEquals(expected, extracted); } - @Test public void testNullAndEmpty() { @@ -116,6 +115,26 @@ public void testNullAndEmpty() Assert.assertEquals(null, extractionFn.apply("/a/b")); } + @Test + public void testMissingValueReplacementFromNullAndEmpty() + { + String regex = "(bob)"; + ExtractionFn extractionFn = new RegexDimExtractionFn(regex, true, "NO MATCH"); + Assert.assertEquals("NO MATCH", extractionFn.apply("")); + Assert.assertEquals("NO MATCH", extractionFn.apply(null)); + } + + @Test + public void testMissingValueReplacementToEmpty() + { + String regex = "(bob)"; + ExtractionFn extractionFn = new RegexDimExtractionFn(regex, true, ""); + Assert.assertEquals(null, extractionFn.apply(null)); + Assert.assertEquals(null, extractionFn.apply("")); + Assert.assertEquals(null, extractionFn.apply("abc")); + Assert.assertEquals(null, extractionFn.apply("123")); + } + @Test public void testMissingValueReplacement() { diff --git a/processing/src/test/java/io/druid/query/extraction/UpperExtractionFnTest.java b/processing/src/test/java/io/druid/query/extraction/UpperExtractionFnTest.java index 55b82c346b7d..4b2a8e841147 100644 --- a/processing/src/test/java/io/druid/query/extraction/UpperExtractionFnTest.java +++ b/processing/src/test/java/io/druid/query/extraction/UpperExtractionFnTest.java @@ -35,6 +35,8 @@ public void testApply() Assert.assertEquals("UPPER", extractionFn.apply("uPpeR")); Assert.assertEquals(null, extractionFn.apply("")); Assert.assertEquals(null, extractionFn.apply(null)); + Assert.assertEquals(null, extractionFn.apply((Object)null)); + Assert.assertEquals("1", extractionFn.apply(1)); } @Test