code review

processing/src/main/java/org/apache/druid/query/filter/InDimFilter.java

@@ -142,12 +142,12 @@ public class InDimFilter extends AbstractOptimizableDimFilter implements Filter
 
     // The values set can be huge. Try to avoid copying the set if possible.
     // Note that we may still need to copy values to a list for caching. See getCacheKey().
-    if (NullHandling.sqlCompatible() || !values.remove("")) {
+    if (!NullHandling.sqlCompatible() && values.remove("")) {
-      this.values = values;
+      // In Non sql compatible mode, empty strings should be converted to nulls for the filter.
-    } else {
+      // In sql compatible mode, empty strings and nulls should be treated differently
       values.add(null);
-      this.values = values;
     }
+    this.values = values;
 
     this.dimension = Preconditions.checkNotNull(dimension, "dimension cannot be null");
     this.extractionFn = extractionFn;

processing/src/main/java/org/apache/druid/segment/join/lookup/LookupJoinable.java

@@ -20,7 +20,7 @@
 package org.apache.druid.segment.join.lookup;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import org.apache.druid.java.util.common.io.Closer;
 import org.apache.druid.query.lookup.LookupExtractor;
 import org.apache.druid.segment.ColumnSelectorFactory;
@@ -33,7 +33,6 @@ import org.apache.druid.segment.join.Joinable;
 
 import javax.annotation.Nullable;
 import java.io.Closeable;
-import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
@@ -107,10 +106,10 @@ public class LookupJoinable implements Joinable
     Set<String> correlatedValues;
     if (LookupColumnSelectorFactory.KEY_COLUMN.equals(searchColumnName)) {
       if (LookupColumnSelectorFactory.KEY_COLUMN.equals(retrievalColumnName)) {
-        correlatedValues = ImmutableSet.of(searchColumnValue);
+        correlatedValues = Sets.newHashSet(searchColumnValue);
       } else {
         // This should not happen in practice because the column to be joined on must be a key.
-        correlatedValues = Collections.singleton(extractor.apply(searchColumnValue));
+        correlatedValues = Sets.newHashSet(extractor.apply(searchColumnValue));
       }
     } else {
       if (!allowNonKeyColumnSearch) {
@@ -118,11 +117,11 @@ public class LookupJoinable implements Joinable
       }
       if (LookupColumnSelectorFactory.VALUE_COLUMN.equals(retrievalColumnName)) {
         // This should not happen in practice because the column to be joined on must be a key.
-        correlatedValues = ImmutableSet.of(searchColumnValue);
+        correlatedValues = Sets.newHashSet(searchColumnValue);
       } else {
         // Lookup extractor unapply only provides a list of strings, so we can't respect
         // maxCorrelationSetSize easily. This should be handled eventually.
-        correlatedValues = ImmutableSet.copyOf(extractor.unapply(searchColumnValue));
+        correlatedValues = Sets.newHashSet(extractor.unapply(searchColumnValue));
       }
     }
     return Optional.of(correlatedValues);

processing/src/test/java/org/apache/druid/query/filter/InDimFilterTest.java

@@ -21,7 +21,6 @@ package org.apache.druid.query.filter;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import org.apache.druid.common.config.NullHandling;
 import org.apache.druid.jackson.DefaultObjectMapper;
@@ -59,26 +58,37 @@ public class InDimFilterTest extends InitializedNullHandlingTest
   }
 
   @Test
-  public void testGetValuesWithValuesSetOfNonEmptyStringsUseTheGivenSet()
+  public void testInitWithoutEmptyStringsShouldUseValuesAsIs()
   {
-    final Set<String> values = ImmutableSet.of("v1", "v2", "v3");
+    Set<String> values = Sets.newHashSet("good", "bad", null);
-    final InDimFilter filter = new InDimFilter("dim", values, null, null);
+    final InDimFilter inDimFilter = new InDimFilter("dimTest", values, null);
-    Assert.assertSame(values, filter.getValues());
+    Assert.assertSame(values, inDimFilter.getValues());
+    Assert.assertEquals(3, values.size());
+    Assert.assertEquals(Sets.newHashSet("good", "bad", null), inDimFilter.getValues());
   }
 
   @Test
-  public void testGetValuesWithValuesSetIncludingEmptyString()
+  public void testInitWithEmptyStringsShouldUseValuesAndReplaceWithNullInNonSqlCompatibleMode()
   {
-    final Set<String> values = Sets.newHashSet("v1", "", "v3");
+    Set<String> values = Sets.newHashSet("good", "bad", "");
-    final InDimFilter filter = new InDimFilter("dim", values, null, null);
+    final InDimFilter inDimFilter = new InDimFilter("dimTest", values, null);
-    if (NullHandling.replaceWithDefault()) {
+    Assert.assertSame(values, inDimFilter.getValues());
-      Assert.assertNotSame(values, filter.getValues());
+    Assert.assertEquals(3, values.size());
-      Assert.assertEquals(Sets.newHashSet("v1", null, "v3"), filter.getValues());
+    if (NullHandling.sqlCompatible()) {
+      Assert.assertEquals(Sets.newHashSet("good", "bad", ""), inDimFilter.getValues());
     } else {
-      Assert.assertSame(values, filter.getValues());
+      Assert.assertEquals(Sets.newHashSet("good", "bad", null), inDimFilter.getValues());
     }
   }
 
+  @Test
+  public void testGetValuesWithValuesSetOfNonEmptyStringsUseTheGivenSet()
+  {
+    final Set<String> values = Sets.newHashSet("v1", "v2", "v3");
+    final InDimFilter filter = new InDimFilter("dim", values, null, null);
+    Assert.assertSame(values, filter.getValues());
+  }
+
   @Test
   public void testGetCacheKeyReturningSameKeyForValuesOfDifferentOrders()
   {