Retain order of AND, OR filter children. by gianm · Pull Request #10758 · apache/druid

gianm · 2021-01-14T01:33:01Z

If we retain the order, it enables short-circuiting. People can put a
more selective filter earlier in the list and lower the chance that
later filters will need to be evaluated.

Ordering was working before #9608, which switched to unordered
sets to solve a different problem. This patch tries to solve that
problem a different way.

This patch moves filter simplification logic from "optimize" to
"toFilter", because that allows the code to be shared with Filters.and
and Filters.or. The simplification has become more complicated and so
it's useful to share it.

This patch also removes code from CalciteCnfHelper that is no longer
necessary because Filters.and and Filters.or are now doing the work.

If we retain the order, it enables short-circuiting. People can put a more selective filter earlier in the list and lower the chance that later filters will need to be evaluated. Short-circuiting was working before apache#9608, which switched to unordered sets to solve a different problem. This patch tries to solve that problem a different way. This patch moves filter simplification logic from "optimize" to "toFilter", because that allows the code to be shared with Filters.and and Filters.or. The simplification has become more complicated and so it's useful to share it. This patch also removes code from CalciteCnfHelper that is no longer necessary because Filters.and and Filters.or are now doing the work.

gianm · 2021-01-14T01:38:30Z

This patch moves filter simplification logic from "optimize" to
"toFilter", because that allows the code to be shared with Filters.and
and Filters.or. The simplification has become more complicated and so
it's useful to share it.

Note on this part: in theory, there is a performance risk here, since if toFilter() is called more often than optimize(), it means we'd do the simplification work more often. As far as I can tell, this isn't a big risk, since calls to toFilter() are generally either going through AbstractOptimizableDimFilter.toOptimizedFilter (which are cached) or are happening in places that are once per query, not once per segment (like FilteredAggregatorFactory's constructor).

Doing the above analysis made me feel like collapsing DimFilter and Filter would be a good idea.

gianm · 2021-01-15T09:05:37Z

processing/src/test/java/org/apache/druid/segment/filter/FilterCnfConversionTest.java

        )
    );
    final Filter expected = FilterTestUtils.and(
+        // The below OR filter could be eliminated because this filter also has


Note: these result changes were just re-orderings.

gianm · 2021-01-15T09:05:57Z

processing/src/test/java/org/apache/druid/segment/filter/FilterCnfConversionTest.java

        )
    );
-    final Set<Filter> expected = ImmutableSet.of(
+    final List<Filter> expected = ImmutableList.of(


Another note: these were also just reorderings.

gianm · 2021-01-15T09:06:46Z

processing/src/test/java/org/apache/druid/segment/join/JoinFilterAnalyzerTest.java

-                        new InDimFilter("regionIsoCode", ImmutableSet.of("CA"), null, null).toFilter()
-                    )
-                )
+                new InDimFilter("countryIsoCode", ImmutableSet.of("US"), null, null).toFilter(),


This rewrite got better!

suneet-s · 2021-01-16T04:24:06Z

processing/src/main/java/org/apache/druid/segment/filter/Filters.java

+      assert !filters.isEmpty();
+      // Original "filters" list must have been 100% literally-true filters.
+      return Optional.of(TrueFilter.instance());
+    } else if (filtersToUse.stream().anyMatch(filter -> filter instanceof FalseFilter)) {


nit: You can avoid this anyMatch check by doing the FalseFilter optimization in flattenAndChildren by making it return a single FalseFilter

I'm not sure exactly what you're suggesting, so I left this as-is.

suneet-s · 2021-01-16T04:25:05Z

processing/src/main/java/org/apache/druid/segment/filter/Filters.java

+      assert !filters.isEmpty();
+      // Original "filters" list must have been 100% literally-false filters.
+      return Optional.of(FalseFilter.instance());
+    } else if (filtersToUse.stream().anyMatch(filter -> filter instanceof TrueFilter)) {


nit: similar comment to line 518

suneet-s · 2021-01-16T04:31:59Z

processing/src/main/java/org/apache/druid/segment/filter/Filters.java

+        // Order of children doesn't matter for equivalence.
+        final Set<Filter> ourFilterSet = new HashSet<>(((BooleanFilter) filter).getFilters());
+        final Set<Filter> theirFilterSet = new HashSet<>(((BooleanFilter) that.filter).getFilters());
+        return ourFilterSet.equals(theirFilterSet);


Does this handle nested BooleanFilters correctly?

It handles them correctly in that it's not wrong. But, it isn't necessarily doing the best possible optimization. I edited it to use EquivalenceCheckedFilter recursively and added some more tests (see FiltersTest for the new ones).

Nevermind, I decided to take your advice and switch the List to a LinkedHashSet, which makes this EquivalenceCheckedFilter technique no longer necessary.

suneet-s · 2021-01-16T04:41:57Z

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

  ValueMatcher[] EMPTY_VALUE_MATCHER_ARRAY = new ValueMatcher[0];

-  Set<Filter> getFilters();
+  List<Filter> getFilters();


Could you add a javadoc to this function explaining why we a list is used here so that we can preserve the order of the filters to enable short-circuiting.

Writing out this comment, made me think perhaps we could achieve the same outcome by using a LinkedHashSet instead - that would make it clearer in the rest of the code that there are no duplicates in the filters

I think doing the LinkedHashSet makes sense, so I went with that approach. It simplified the code a bit, and let me delete the EquivalenceCheckedFilter stuff.

abhishekagarwal87 · 2021-01-18T15:16:26Z

processing/src/main/java/org/apache/druid/segment/filter/Filters.java

+
+      if (filter instanceof BooleanFilter && filter.getClass().equals(that.filter.getClass())) {
+        // Order of children doesn't matter for equivalence.
+        final Set<Filter> ourFilterSet = new HashSet<>(((BooleanFilter) filter).getFilters());


will your changes fix #10316? I am guessing, your changes will make the situations describe in that PR less likely as Set is being created only when two AndFilter/OrFilters are compared. could we also compare the size of child filter and return early before creating a possible expensive hashSet?

We're back to a Set due to #10758 (comment), so this won't change much. Probably memoizing the hashCode is still the way to go.

suneet-s

LGTM

* Retain order of AND, OR filter children. If we retain the order, it enables short-circuiting. People can put a more selective filter earlier in the list and lower the chance that later filters will need to be evaluated. Short-circuiting was working before apache#9608, which switched to unordered sets to solve a different problem. This patch tries to solve that problem a different way. This patch moves filter simplification logic from "optimize" to "toFilter", because that allows the code to be shared with Filters.and and Filters.or. The simplification has become more complicated and so it's useful to share it. This patch also removes code from CalciteCnfHelper that is no longer necessary because Filters.and and Filters.or are now doing the work. * Fixes for inspections. * Fix tests. * Back to a Set.

gianm added Area - Querying Performance labels Jan 14, 2021

gianm added 2 commits January 14, 2021 08:21

Fixes for inspections.

933edc1

Fix tests.

076538a

gianm commented Jan 15, 2021

View reviewed changes

gianm requested a review from jihoonson January 15, 2021 10:38

suneet-s reviewed Jan 16, 2021

View reviewed changes

abhishekagarwal87 reviewed Jan 18, 2021

View reviewed changes

gianm added 2 commits January 19, 2021 23:33

Merge branch 'master' into filter-and-or-retain-order

832c66d

Back to a Set.

82482be

suneet-s approved these changes Jan 20, 2021

View reviewed changes

suneet-s merged commit 8b808c4 into apache:master Jan 20, 2021

gianm added this to the 0.21.0 milestone Jan 20, 2021

gianm deleted the filter-and-or-retain-order branch September 23, 2022 19:22

Conversation

gianm commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm commented Jan 14, 2021

Uh oh!

gianm Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gianm commented Jan 14, 2021 •

edited

Loading

gianm Jan 15, 2021 •

edited

Loading

gianm Jan 15, 2021 •

edited

Loading

gianm Jan 20, 2021 •

edited

Loading