fix filtering bug in filtering unnest cols and dim cols: Received a non-applicable rewrite by pranavbhole · Pull Request #14587 · apache/druid

pranavbhole · 2023-07-14T19:45:05Z

Fixes #XXXX.

Description

This PR fixes the bug in the following query, currently it throws the exception org.apache.druid.query.QueryException: Received a non-applicable rewrite: {j0.unnest=c2}, filter's dimension: c1
Whenever we have And / OR filters on unnest columns, we rewrite the filters on unnest cols.
We were covering OR filters and disqualifying the rewrite for non unnest cols and we have to to do same thing with AND filters as well. This PR checks for Boolean filter and disqualify the rewrite for non unnest cols.

Adding the 3 tests that covers the given scenario.

select * from ( 
          select * from (
                         select * from mytest, unnest(mv_to_array(c2)) as u(n1)
                     ), unnest(mv_to_array(c2)) as u(n2)
               ), unnest(mv_to_array(c2)) as u(n3)
where n1='A' and c1>=1

3rd test case cover the nested filer scenario for unnest filters, This PR address the unpack the nested OR/AND filters and attempt to rewrite only for qualified filters.

SELECT dimZipf, dim3_unnest1, dim3_unnest2, dim3_unnest3 FROM 
      ( SELECT * FROM 
           ( SELECT * FROM lotsocolumns, UNNEST(MV_TO_ARRAY(dimMultivalEnumerated)) as ut(dim3_unnest1) )           ,UNNEST(MV_TO_ARRAY(dimMultivalEnumerated)) as ut(dim3_unnest2) 
      ), UNNEST(MV_TO_ARRAY(dimMultivalEnumerated)) as ut(dim3_unnest3)  WHERE dimZipf=27 AND (dim3_unnest1='Baz' OR dim3_unnest2='Hello') AND dim3_unnest3='World'

Thank you

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

MyFoo
OurBar
TheirBaz

This PR has:

somu-imply

Looks good. Thanks for the PR. The recursive one is a nice touch to handle the complex filters

clintropolis · 2023-07-14T23:21:28Z

            final OrFilter preOrFilter = new OrFilter(preFilterList);
            filterSplitter.addPreFilter(preOrFilter);


why do we always make an OrFilter if we are only checking for BooleanFilter to break down? Isn't this going to turn AndFilter into OrFilter? Am I missing something?

you are previously this code was dealign with only OR filters, we need to change the final filter based on type. I Need refactor the code to create new PreFilterList.

Oh shoot, I missed that. Good catch. We are extracting the filters and adding each to the pre-filter list. Maybe at each level of recursion, we need to recreate the And/Or filter and add it back to the pre-filter list. In such a case the number of filters in pre-filter list will be equal to the original filter size.

@somu-imply @clintropolis Pushed a change to address the comment. Can you please review?

There seems to be an issue with the test test_pushdown_or_filters_unnested_and_original_dimension_with_unnest_adapters as the expected results have changed. I'm going through the code today

somu-imply · 2023-07-17T22:48:41Z

+        ))
+    ));
+
+    final OrFilter expectedPushDownFilter = new OrFilter(ImmutableList.of(


Can we add a similar test case on AND of OR filters as well?

adding more test case

somu-imply · 2023-07-17T23:34:40Z

Almost there. Suggesting 1 more change:

There are 2 cases:

Consider the query

select * from foo, UNNEST(mv_to_array(dim3)) as u(d3) where d3 IN (a,b) or m1 < 10

Here since d3 can be mapped directly to an input column dim3, the filter can be reconstructed as a pre-filter with dim3 IN (a,b) or m1<10 and pushed to the base cursor.

Consider the query

select * from foo, UNNEST(ARRAY[dim1,dim2]) as u(d12) where d12 IN (a,b) or m1 < 10

since the filter outside contains an OR filter and d12 has no direct match to an input column there should be no pre-filters and the entire filter should be just used as a post-filter. In the current code, this needs to be accounted for and a test case needs to be added for the same. Currently, for the failing query in CI, this does not seem to be the case and returns incorrect results

filterSplitter.filtersForPostUnnestCursor = {ArrayList@10853}  size = 1
0 = {OrFilter@8617} "(m1 as numeric < 2 || j0.unnest IN (a, aa))"
filterSplitter.filtersPushedDownToBaseCursor = {ArrayList@10854}  size = 1
0 = {OrFilter@10852} "(m1 as numeric < 2)"

clintropolis · 2023-07-18T19:21:09Z

+                      bound("dimZipf", "27", "27", false, false, null, StringComparators.NUMERIC),
+                      new SelectorDimFilter("j0.unnest", "Baz", null)


since #14542 has been merged, this should use range and equality instead of bound and making a SelectorDimFilter directly. same for other tests added in this PR

somu-imply

Thanks for updating the code and adding those tests. I have some comments so far, mostly cosmetic.

somu-imply · 2023-08-01T00:06:38Z

+    return new SelectorDimFilter(dimension, value, extractionFn).toFilter();
+  }
+
+  public static DimFilter sdfd(String dimension, String value, ExtractionFn extractionFn)


nit. maybe some better names...but am good with these too

somu-imply · 2023-08-01T00:16:11Z

+   * 3. Filters on unnest column which is derived from Array or any other Expression can not be pushe down to base.
+   * for example: a=1 and vc=3 , lets say vc is ExpressionVirtualColumn, and vc=3 can not be push down to base even if top level is AND filter.
+   * 4.
+   *


Thanks for the comments, for future developers can we make it a bit clear with why in case 1, the entire filter (b=2 OR c=2) cannot be pushed down to the preFilter. Probably we can add an example saying that either b2 or c2 references a column that is not a part of the input for the case of unnest.

Something like, in unnest we come across 2 cases:

unnesting a single dimension e.g. select * from foo, UNNEST(MV_TO_ARRAY(dim3)) as u(d3)

Unnesting an expression from multiple columns e.g. select * from foo, UNNEST(ARRAY[dim1,dim2]) as u(d12)

In case 1, d3 is a direct reference to dim3 so any filter using d3 can be rewritten using dim3 and added to pre filter while in case2, due to presence of the expression virtual column expressionVirtualColumn("j0.unnest", "array(\"dim1\",\"dim2\")", ColumnType.STRING_ARRAY) the filters on d12 cannot be pushed to the pre filters

updated the comments

clintropolis

would be nice to add some tests with deeper nesting to UnnestStorageAdapterTest just to flex the recursive stuff a bit more

clintropolis · 2023-08-01T18:44:57Z

+                      NullHandling.sqlCompatible()
+                      ? equality("dimZipf", "27", ColumnType.LONG)
+                      : bound("dimZipf", "27", "27", false, false, null, StringComparators.NUMERIC),


you can use numericEquality instead of this

clintropolis · 2023-08-01T18:49:08Z

    return new SelectorFilter(fieldName, value, null);
  }
+
+  public static Filter sdf(String dimension, String value)


why does this need to exist instead of just using selector? I don't see any callers using the version that takes extractionFn, and only one caller using sdfd which seems overkill..

removed static functions that I added

clintropolis · 2023-08-01T19:01:38Z

+
+    public FilterSplitter(
+        String inputColumn,
+        ColumnCapabilities inputColumnCapabilites, VirtualColumns queryVirtualColumns


nit formatting, VirtualColumns should be on a separate line (the style checker has trouble enforcing this sometimes)

clintropolis · 2023-08-03T20:28:53Z

-              preFilterList.add(filter);
+        // requires check for OR and And filters, disqualify rewrite for non-unnest filters
+        if (queryFilter instanceof BooleanFilter) {
+          int originalFilterCount = Filters.countNumberOfFilters(queryFilter);


it seems like it would be a lot more efficient to do both the before and after count as part of the traversal that the filter splitter does, any reason I'm missing not to do that? Otherwise it seems like we have to basically traverse the filter tree 3 times per segment, before to get a count, then to try to rewrite, then after to get the new count. It seems like this could all just be baked into the FilterSplitter itself. Apologies for not spotting this earlier on in review... but if we do this then i think we no longer need Filters.countNumberOfFilters

clintropolis · 2023-08-03T20:31:30Z

    return retVal;
  }
+
+  public static int countNumberOfFilters(Filter filter)


nit: if this method needs to stay (re if other comment suggesting pushing counting inside of filter splitter does not work out for some reason) then this argument should be marked @Nullable

clintropolis · 2023-08-04T18:44:16Z

+    ));
+    testComputeBaseAndPostUnnestFilters(
+        testQueryFilter,
+        "unnested-multi-string1 = 3",


this doesn't seem right, shouldn't it be pushing down multi-string1 = 3 since unnested-multi-string1 doesn't exist on the base adapter? I think the problem is that testComputeBaseAndPostUnnestFilters is not getting the virtual column capabilities correctly, which makes it use the default capabilities to define it as float typed rather than string typed, so it isn't rewritten correctly in rewriteFilterOnUnnestColumnIfPossible because it is not string typed.

If i change the line to get the capabilities from the unnest storage adapter

vc.capabilities(UNNEST_STORAGE_ADAPTER, inputColumn)

, the test fails because the pushed down filter is (multi-string1 = 3 && multi-string1 = 2) which seems like the correct answer? Also makes most of the other tests fail expectations. I think we should probably add a check to ensure that filters on the unnest column never get pushed down, since I think that would effectively make the results null since the filter would match nothing since the column doesn't exist (unless it was filtering for nulls i guess)

clintropolis · 2023-08-04T18:47:09Z

+        null,
+        VirtualColumns.EMPTY,
+        inputColumn,
+        vc.capabilities(inputColumn)


this should be

Suggested change

vc.capabilities(inputColumn)

vc.capabilities(UNNEST_STORAGE_ADAPTER, inputColumn)

clintropolis · 2023-08-07T23:37:04Z

+    ));
+    testComputeBaseAndPostUnnestFilters(
+        testQueryFilter,
+        "multi-string1 = 3",


why is it that only the filter on the unnest column pushed down instead of the whole and? same question for other test cases involving an AND? I think I would expect that in the case of AND especially, everything which can be pushed down is pushed down. This is what the comments in computeBaseAndPostUnnestFilters indicates should happen..

In the case of an AND filter, I think that everything can be pushed down except for the case where the unnest output column is not a simple direct access to a MVD. Since it is an AND though, I think all of the other clauses could still be pushed down in that case, leaving only the unnest filter which could not be rewritten as a post filter.

But in the case of OR filters, I think its either all or nothing can be pushed down. So if there is a filter on the unnest output column where the virtual column is not a simple direct column access to a multi-value string, then nothing can be pushed down, else if the unnest column filters can be rewritten because they are simple mvd direct access, everything can be pushed down.

in this test, it is case of And filters, and We only push down the filters to base where rewriteFilterOnUnnestColumnIfPossible is successful.

fixed the bug in filter counts and now tests looks good, Can you please verify again?

somu-imply · 2023-08-14T21:23:18Z

+            filterSplitter.addPreFilter(newFilter != null ? newFilter : filter);
+            filterSplitter.addToPreFilterCount(1);
+          }
+          filterSplitter.addToOriginalFilterCount(1);


Isn't the filter getting added to the preFilter list twice or counted twice with addToPreFilterCount ? It already got added to preFilterList once in line 503. This would cause the count to go up and when you make the comparison of count on line 330, it would give the wrong count.

somu-imply

Looks good, two optimizations that need to be done are:

Make sure not filters on unnested column are not pushed to base
Update post filters for and filters to include only the subset not pushed to base

These can be done in a followup PR to not delay this one

clintropolis

👍

per comment (and agreeing with @somu-imply) I think there is still some follow-up work to be done. Also it would be nice to have a few more tests where it can't push anything down, such as when filter is on unnest column that is wrapped in some expression like array_slice which would make the virtual column more than a direct access and so shouldn't be eligible for pushdown.

clintropolis · 2023-08-17T00:53:07Z

+    testComputeBaseAndPostUnnestFilters(
+        testQueryFilter,
+        "(multi-string1 = 3 && multi-string1 = 2)",
+        "(unnested-multi-string1 = 3 && multi-string1 = 2)"
+    );


I think there is still some work to do here, but is ok to do as a follow-up.

Side note, this would be a very strange query since there is like coincidentally a filter on the underlying unnest column that is referring to it directly alongside a filter on the unnested column itself. I think it would make sense to migrate a lot of these tests away from having filters on both the unnest column and its underlying column, since I cant imagine its going to be common in practice.

But thinking about the more general case, like if it was WHERE unnested-multi-string1 = 3 && other-column = 2 i the best rewrite would be

testQueryFilter, "(multi-string1 = 3 && other-column= 2)", "unnested-multi-string1 = 3" );``` but the current code leaves the `other-column = 2` in the post-filters, which isn't necessary. It would be a pretty big optimization to leave these off and only have to do post-filters when absolutely necessary. OR filters cannot have the same optimization. But, i think it should be ok to do this in a follow-up PR, since there is some other correctness stuff to be done as well, such as skipping pushdown of NOT filters, which could incorrectly filter out too much before the post-filter has a chance to match stuff.

somu-imply reviewed Jul 14, 2023

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/segment/UnnestStorageAdapter.java

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch from a3dc3ac to 89aa0b5 Compare July 14, 2023 21:52

somu-imply approved these changes Jul 14, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch 2 times, most recently from 209c889 to 93afd47 Compare July 14, 2023 22:35

clintropolis reviewed Jul 14, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch 3 times, most recently from 35bee11 to 42c0a61 Compare July 17, 2023 22:15

somu-imply reviewed Jul 17, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch from 42c0a61 to 7d9518e Compare July 18, 2023 19:15

clintropolis reviewed Jul 18, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch 3 times, most recently from 3b27907 to dd1f92d Compare July 31, 2023 16:48

github-advanced-security AI found potential problems Jul 31, 2023

View reviewed changes

Comment thread processing/src/test/java/org/apache/druid/segment/UnnestStorageAdapterTest.java Fixed

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch 3 times, most recently from 26c4fb1 to 77b62a6 Compare July 31, 2023 23:39

somu-imply reviewed Aug 1, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch from 77b62a6 to ccabe2b Compare August 1, 2023 01:23

abhishekagarwal87 added the Bug label Aug 1, 2023

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch from ccabe2b to 7210ae7 Compare August 1, 2023 18:41

clintropolis reviewed Aug 1, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch 2 times, most recently from c3e0a33 to 0508ec4 Compare August 2, 2023 19:38

clintropolis reviewed Aug 3, 2023

View reviewed changes

clintropolis reviewed Aug 4, 2023

View reviewed changes

clintropolis reviewed Aug 7, 2023

View reviewed changes

somu-imply reviewed Aug 14, 2023

View reviewed changes

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch 2 times, most recently from b4b0ec1 to a0a8324 Compare August 16, 2023 06:02

pranavbhole added 6 commits August 16, 2023 09:31

fix filtering bug in filtering unnest cols and dim cols

8c26d4c

Refactor logic for unnest filters/query filters and add tests

c408620

addressing comments

9a6e911

Removing 2 parse for filter counts and addressing comments

f8f969b

Fixing the bug in the count and fix tests

0a1dece

Fixing sql compat mode tests

96a8208

pranavbhole force-pushed the fixBugInfilterOnUnnestColWithDimFilers branch from a0a8324 to 96a8208 Compare August 16, 2023 16:31

somu-imply approved these changes Aug 17, 2023

View reviewed changes

clintropolis approved these changes Aug 17, 2023

View reviewed changes

clintropolis merged commit 26d82fd into apache:master Aug 17, 2023

pranavbhole deleted the fixBugInfilterOnUnnestColWithDimFilers branch September 6, 2023 16:11

LakshSingla added this to the 28.0 milestone Oct 12, 2023

		final OrFilter preOrFilter = new OrFilter(preFilterList);
		filterSplitter.addPreFilter(preOrFilter);

		bound("dimZipf", "27", "27", false, false, null, StringComparators.NUMERIC),
		new SelectorDimFilter("j0.unnest", "Baz", null)

	vc.capabilities(inputColumn)
	vc.capabilities(UNNEST_STORAGE_ADAPTER, inputColumn)

Conversation

pranavbhole commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

Uh oh!

Uh oh!

somu-imply left a comment

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!

somu-imply Jul 15, 2023 • 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!

somu-imply commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

somu-imply left a comment

Choose a reason for hiding this comment

Uh oh!

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!

clintropolis left a comment

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!

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

pranavbhole commented Jul 14, 2023 •

edited

Loading

somu-imply Jul 15, 2023 •

edited

Loading

somu-imply commented Jul 17, 2023 •

edited

Loading

somu-imply Aug 14, 2023 •

edited

Loading

clintropolis left a comment •

edited

Loading