Add "like" filter. #3642

gianm · 2016-11-01T20:19:12Z

The main purpose of this is a performance boost over the regex filter (which can do similar stuff) for prefix patterns like "foo%" (see benchmarks below). A secondary purpose is making life easier for people that are familiar with SQL LIKE syntax.

Benchmarks:

matchLikeEquals is foo LIKE "1000000"
matchSelectorEquals is foo = "1000000" using the existing selector filter
matchLikePrefix is foo LIKE "50%"
matchRegexPrefix is foo RLIKE "^50.*" using the existing regex filter

Benchmark                                (cardinality)  Mode  Cnt       Score      Error  Units
LikeFilterBenchmark.matchLikeEquals               1000  avgt   10       0.799 ±    0.033  us/op
LikeFilterBenchmark.matchLikeEquals             100000  avgt   10       1.570 ±    0.055  us/op
LikeFilterBenchmark.matchLikeEquals            1000000  avgt   10       1.688 ±    0.046  us/op
LikeFilterBenchmark.matchSelectorEquals           1000  avgt   10       0.838 ±    0.009  us/op
LikeFilterBenchmark.matchSelectorEquals         100000  avgt   10       1.447 ±    0.035  us/op
LikeFilterBenchmark.matchSelectorEquals        1000000  avgt   10       1.825 ±    0.087  us/op
LikeFilterBenchmark.matchLikePrefix               1000  avgt   10       5.214 ±    0.145  us/op
LikeFilterBenchmark.matchLikePrefix             100000  avgt   10     937.082 ±   53.887  us/op
LikeFilterBenchmark.matchLikePrefix            1000000  avgt   10   10594.309 ±  537.874  us/op
LikeFilterBenchmark.matchRegexPrefix              1000  avgt   10     153.203 ±   12.560  us/op
LikeFilterBenchmark.matchRegexPrefix            100000  avgt   10   15425.745 ± 1269.856  us/op
LikeFilterBenchmark.matchRegexPrefix           1000000  avgt   10  150684.500 ± 3378.273  us/op

leventov · 2016-11-01T21:31:43Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+
+  private final String dimension;
+  private final String pattern;
+  private final String escape;


Maybe store escape as Character, since it must be null or 1-symbol string?

Also maybe add @Nullable, to help IDE to highlight non-checking access?

leventov · 2016-11-01T21:32:53Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+    private final SuffixStyle suffixStyle;
+    private final Pattern suffixPattern;
+
+    public LikeMatcher(final String prefix, final SuffixStyle suffixStyle, final Pattern suffixPattern)


Should this constructor be public?

No good reason; changed to private

leventov · 2016-11-01T21:34:12Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+      MATCH_PATTERN
+    }
+
+    private final String prefix;


@Nullable?

leventov · 2016-11-01T21:34:38Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+
+    private final String prefix;
+    private final SuffixStyle suffixStyle;
+    private final Pattern suffixPattern;


@Nullable?

leventov · 2016-11-01T21:35:02Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+      this.suffixPattern = suffixPattern;
+
+      if (suffixPattern == null && suffixStyle == SuffixStyle.MATCH_PATTERN) {
+        throw new ISE("Need suffixPattern for pattern matching!");


Maybe IAE, since we are in constructor?

Changed to IAE

leventov · 2016-11-01T21:54:59Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+
+    public DruidPredicateFactory predicateFactory(final ExtractionFn extractionFn)
+    {
+      return new DruidPredicateFactory()


Could you please give this class a name? Something like class LikeDruidPredicateFactory {...} return new LikeDruidPredicateFactory()? Just one extra LOC, but more readability in traces and logging. I've started fighting anonymous classes here: https://github.com/metamx/druid/commits/more-topn-metrics, this branch is far from merge, but probably new code could avoid this

IMO the anonymous class is easier to read (much less boilerplate code). My personal feeling is the balance between code readability and logging readability is in favor of anonymous classes for things like this. But I'm open to being convinced otherwise or outvoted.

actually, nevermind, I thought you were asking for a normal inner class. But based on "just one extra LOC" I guess you're asking for a local inner class in that method. Sure that would work, although I guess I think they look kind of weird…

@gianm yes I mean local inner class. The name also serves as additional documentation sometimes, e. g. metamx@225c7e9#diff-8da1c4dd87d76bcce224a6be6bf74ed8R615 (another extra LOC justifying using local inner class probably shouldn't be there if this would be a common practice)

leventov · 2016-11-01T21:56:31Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+              }
+            };
+          } else {
+            return new Predicate<String>()


This instance could be cached, or LikeMatcher could implement Predicate itself and return this here. Less indirection is a good thing for performance.

leventov · 2016-11-01T21:57:03Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+              }
+            };
+          } else {
+            return new DruidLongPredicate()


This instance could be cached, or LikeMatcher could implement DruidLongPredicate itself and return this here

The predicate returned depends on whether there's an extraction fn or not so it can't just be this.

Oh, you mean just for the else…

I think the benefit there relative to code complexity is gonna be marginal.

Ok anyway it should (?) be changed in a number of DimFilters, so when and if I will make such PR this could be discussed again

leventov · 2016-11-01T22:01:52Z

processing/src/main/java/io/druid/segment/filter/LikeFilter.java

+
+public class LikeFilter implements Filter
+{
+  private static final String HIGHEST_CHARACTER = String.valueOf(Character.MAX_VALUE);


Why couldn't this constant be char?

This is just so + works later on

@gianm Hm + works with chars as well isn't it? And after that I think own constant is not needed, can + Character.MAX_VALUE directly.

Ah, yeah, you're right. I changed it.

leventov · 2016-11-01T22:09:41Z

processing/src/main/java/io/druid/segment/filter/LikeFilter.java

+
+      // Union bitmaps for all matching dimension values in range
+      return selector.getBitmapFactory().union(
+          new Iterable<ImmutableBitmap>()


This Iterable chain of nested classes is very noisy. Maybe create a simple List of bitmaps, fill it in a for loop with i between startIndex and endIndex, then return bitmapFactory.union(bitmaps)?

I like the iterator since it can create and union the bitmaps one by one. That means decreased memory use for unioning a large number of bitmaps.

@gianm ok, thanks. Could you please document this in the code? Because it is not obvious.

Or maybe can create a list of integers from beginIndex to endIndex and then transform it using Guava's lazy transformation functions.

I added a comment.

leventov · 2016-11-02T01:15:51Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

      } else {
        // suffixStyle is MATCH_PATTERN
-        return suffixPattern.matcher(suffix).matches();
+        return suffixPattern.matcher(Strings.nullToEmpty(s).substring(prefix.length())).matches();


Maybe can just return matches(s); here? (That is what I meant in my earlier comment). It's not clear what would be faster, anyway it should be comparable. But suffixPattern could then be removed, reducing complexity of LikeMatcher.from() and LikeMatcher is general.

I bet this is faster, since it totally skips even looking at the first prefix.length characters. Calling matches would have to read the first prefix.length characters to verify that they match the prefix.

Hmm, actually I thought substring would return a view but it copies the underlying array. Now I'm not so sure. Okay… I'll get rid of suffixPattern if the benchmarks look equivalent.

@gianm right. My concern is substring(). But seems it could be addressed by Matcher m = suffixPattern.matcher(s); return m.find(prefix.length());

More precisely return m.find(prefix.length()) && m.start() == prefix.length();

@gianm I written this comment: #3642 (comment) before I see your last comment in this mini-thread. So I actually agree matching only suffix should be faster. And I suggested a way to avoid both substring and matching a prefix.

Hmm, m.find(prefix.length()) && m.start() == prefix.length(); could potentially suffer performance-wise due to spending extra effort looking for substring matches after the prefix.length.

At any rate, I tried that, and didn't see a noticeable difference in performance with really any of these approaches, meaning it's tough to verify guesses like the above. So to keep things simple I'm planning to kill suffixPattern and go with your simple suggestion return matches(s);.

The real increase is going to come from extracting prefix and using that for index lookups.

Ok, I pushed a new version with just pattern, no suffixPattern. benchmarks are the same within the margin of error.

leventov · 2016-11-02T01:18:20Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

      if (suffixStyle == SuffixStyle.MATCH_ANY) {
        return true;
      } else if (suffixStyle == SuffixStyle.MATCH_EMPTY) {
-        return suffix.isEmpty();
+        return s == null || s.length() == prefix.length();


Why in this case s == null means a match here? Maybe only if prefix.length() == 0?

The assumption is you should only call this method if you independently verified the first prefix.length characters are equal to prefix. This check as-is will return the right answer if the precondition is adhered to. If not, that's the caller's problem.

@gianm seems like if caller is feeding null here while prefix is not empty/null, it's a mistake in caller code, maybe should better throw something like NPE/IAE, rather than silently return true?

Yeah, the caller code is definitely wrong if this is happening. I just wanted to avoid checking unnecessary things here since this code is going to happen in a tight loop.

I changed it a bit to be:

return (s == null ? 0 : s.length()) == prefix.length();

So now the length check is always done.

yes, this seems to be the right version.

leventov · 2016-11-02T01:21:06Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

@@ -75,94 +79,113 @@ public LikeDimFilter(
      MATCH_PATTERN
    }

+    // Strings match if they start with "prefix" AND:


Maybe these comments could be Javadocs of SuffixStyle enum or constants themselves?

leventov · 2016-11-02T03:12:35Z

processing/src/main/java/io/druid/query/filter/LikeDimFilter.java

+    )
+    {
+      final StringBuilder prefix = new StringBuilder();
+      final StringBuilder suffixRegex = new StringBuilder();


I think now this method could be simplified. Instead of accumulating suffixRegex, it could just assign suffixStyle variable, and make a simple constructor call in the end instead of if-else chain.

Ok, good point. I removed suffixRegex and replaced it with a variable for the suffix matching style. I also removed boolean extractPrefix since it wasn't doing anything really useful anymore.

navis · 2016-11-02T04:15:37Z

hm.. we have 'like' math function code of which is copied from hive. would it be better to use somewhere validated code?

gianm · 2016-11-02T07:30:06Z

@navis I'm not familiar with hive's version to know if it does the stuff this version does (we need to split out any exact prefix for the index lookup optimization, and we want to support custom "escape" chars for clauses such as foo LIKE "bar@%" ESCAPE "@"). If so and if performance is as good or better… sure. Really I didn't look into it since LIKE parsing is relatively simple.

navis · 2016-11-03T02:48:00Z

I got this is specialized version for bitmap index not like what I've commented above. So forget what I've said.

gianm · 2016-11-03T15:16:39Z

@leventov any further comments?

leventov · 2016-11-03T15:31:12Z

@gianm no, LGTM.

nishantmonu51

Looks good to me, some more boundary tests would be nice.

nishantmonu51 · 2016-11-03T20:24:39Z

processing/src/test/java/io/druid/segment/filter/LikeFilterTest.java

+        ImmutableList.of("2", "4")
+    );
+  }
+


minor nit: can we also add boundary tests for matchAll, matchNone, match against string having wildcard only ?

added tests for matching an empty string with and without extractionFns, and matching with just a wildcard.

fjy · 2016-11-03T20:45:35Z

👍

nishantmonu51 · 2016-11-04T17:54:38Z

👍

* Add "like" filter. * Addressed some PR comments. * Slight simplifications to LikeFilter. * Additional simplifications. * Fix comment in LikeFilter. * Clarify comment in LikeFilter. * Simplify LikeMatcher a bit. * No use going through the optimized path if prefix is empty. * Add more tests.

gianm added the Feature label Nov 1, 2016

gianm added this to the 0.9.3 milestone Nov 1, 2016

leventov requested changes Nov 1, 2016

View reviewed changes

leventov reviewed Nov 2, 2016

View reviewed changes

leventov requested changes Nov 2, 2016

View reviewed changes

nishantmonu51 approved these changes Nov 3, 2016

View reviewed changes

gianm added 9 commits November 3, 2016 14:36

Add "like" filter.

1bceb73

Addressed some PR comments.

d05252e

Slight simplifications to LikeFilter.

eafa54f

Additional simplifications.

117edb9

Fix comment in LikeFilter.

b89f56c

Clarify comment in LikeFilter.

f10ca0f

Simplify LikeMatcher a bit.

813e4f5

No use going through the optimized path if prefix is empty.

14cb4f5

Add more tests.

b4a3022

gianm force-pushed the like-filter branch from b7df90f to b4a3022 Compare November 3, 2016 22:09

nishantmonu51 merged commit 2c504b6 into apache:master Nov 4, 2016

leventov mentioned this pull request Jan 24, 2017

Add filter selectivity estimation for auto search strategy #3848

Merged

gianm deleted the like-filter branch February 9, 2017 07:48

gianm added the Release Notes label Feb 23, 2017

gianm mentioned this pull request Feb 28, 2017

Druid 0.10.0 release notes #3944

Closed

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3642 Still query blocks on indexing node

434d405

Add "like" filter. #3642

Add "like" filter. #3642

Conversation

gianm commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navis commented Nov 2, 2016

gianm commented Nov 2, 2016

navis commented Nov 3, 2016

gianm commented Nov 3, 2016

leventov commented Nov 3, 2016

nishantmonu51 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjy commented Nov 3, 2016

nishantmonu51 commented Nov 4, 2016