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
swimfish09 · Apr 1, 2016 · b6e4d8b · b6e4d8b
1 parent 4eb5a2c
commit b6e4d8b
Show file tree

Hide file tree

Showing 11 changed files with 66 additions and 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
@@ -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.
    * <p>
    * 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.
+   * <p>
+   * 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.
+   * <p>
+   * 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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