Fix join filter rewrites with nested queries by jon-wei · Pull Request #9978 · apache/druid

jon-wei · 2020-06-03T06:59:18Z

Fixes #9792 by moving the join filter pre-analysis into the makeCursors method of HashJoinSegmentStorageAdapter. This is done by introducing a new JoinFilterPreAnalysisGroup class, which holds a concurrent hash map of Filter -> JoinFilterPreAnalysis, used to avoid redundant computation of the JoinFilterPreAnalysis

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

jon-wei · 2020-06-03T06:59:39Z

Marking WIP, need to adjust javadocs, will also see test coverage results

jon-wei · 2020-06-03T19:14:59Z

Added javadocs and fixed some inspections, removing WIP

suneet-s

I haven't looked through any of the tests yet. Since there's a lot of tradeoffs to be made, I think we should have a short-circuit in the JoinFilterPreAnalysisGroup that falls back to the old behavior which is disabled by default.

This gives Druid operators a way to go back to the ~ 0.18.1 behavior in case there are any un-foreseen issues with the pre analysis filter caching and they are confident their queries will not run into the issue described in #9792

suneet-s · 2020-06-05T16:20:39Z

    return withOverriddenContext(ImmutableMap.of(QueryContexts.LANE_KEY, lane));
  }
-
-  default VirtualColumns getVirtualColumns()


I'm assuming this change has nothing to do with the bug fix correct?

It was something failing inspections since all the implementations of Query implement that method, it's only needed for the old rewrite mode (so I added this back in)

suneet-s · 2020-06-05T16:25:43Z

-          "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+    JoinFilterPreAnalysis jfpa;
+    if (filter == null) {
+      jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(


Does this mean we'll re-compute the pre-analysis every time if the filter is null?

I think this code path is short-circuited right now, but it could change in the future and it would be hard to remember that the computation is not cached.

I adjusted the analysis group so the key is now (Filter, JoinableClauses, VirtualColumns) instead of just Filter, I think that's a more correct key, and the null filters are cached now.

suneet-s · 2020-06-05T16:28:27Z

+                joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+                joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+            );
+          }


I think a better abstraction is to hide all of this logic in the JoinFilterPreAnalysisGroup. We shouldn't expose getAnalyses() to the other classes.

Instead, consider exposing a function like getPreAnalysisForFilter(Filter f) - that provides the jfpa Users of the API don't need to worry about how it works as long as we guarantee that this function is thread safe.

Also, since the pre-analysis computation is expensive, the way this is written, I think it's still possible for all the threads on the historical to attempt to compute the preAnalysis, so we could see a spike in CPU usage.

Also, since the pre-analysis computation is expensive, the way this is written, I think it's still possible for all the threads on the historical to attempt to compute the preAnalysis, so we could see a spike in CPU usage.

Sorry I misread the javadocs. ConcurrentHashMap blocks on an operation on the same key when a computation is on-going, so this shouldn't be an issue.

From the javadocs

The entire method invocation is performed atomically, so the function is
applied at most once per key. Some attempted update operations
on this map by other threads may be blocked while computation
is in progress, so the computation should be short and simple,
and must not attempt to update any other mappings of this map.

I've added a computeJoinFilterPreAnalysisIfAbsent and getAnalysis method to JoinFilterPreAnalysisGroup

suneet-s · 2020-06-05T16:32:05Z

  private final JoinFilterCorrelations correlations;
-  private final boolean enableFilterPushDown;
-  private final boolean enableFilterRewrite;
+  private final JoinFilterPreAnalysisGroup myGroup;


Does this introduce a memory leak / a circular dependency of some kind?

The group holds a reference to the pre-analysis object via the analyses concurrent map and the pre-analyses object holds a reference back to the group object via myGroup

Did some reading... Java's GC is smart enough to handle circular dependencies, so I think this should be ok.

I think it's fine, but I restructured this to store all the config parameters in a new JoinFilterRewriteConfig class, so there's no more reference to the group in the pre-analysis objects.

suneet-s · 2020-06-05T16:37:54Z

+  private final boolean enableFilterRewrite;
+  private final boolean enableRewriteValueColumnFilters;
+  private final long filterRewriteMaxSize;
+  private final ConcurrentHashMap<Filter, JoinFilterPreAnalysis> analyses;


Have you looked at the implementations of hashCode and equals for all filters? Are they efficient? What happens if the filter is something like an IN filter with a very large list of values? The hashCode check could be slower than deciding that the filter can not be pushed down.

The main one I would be concerned with is InFilter, and to a lesser extent AND/OR.

I added lazy computation/caching for InFilter/AndFilter/OrFilter hashcodes, as long as the hashing is done once per query, and not once per segment, I wouldn't expect hashing overhead to outweigh the benefits of the rewrite (esp since such large filters would be expensive to apply per-row on the RHS)

jon-wei · 2020-06-06T04:49:03Z

I haven't looked through any of the tests yet. Since there's a lot of tradeoffs to be made, I think we should have a short-circuit in the JoinFilterPreAnalysisGroup that falls back to the old behavior which is disabled by default.

I added a query context option and a separate set of methods for the old rewrite mode to JoinFilterPreAnalysisGroup, described as something available temporarily until the new mode is more battle-tested.

clintropolis · 2020-06-06T05:48:49Z

@@ -11486,79 +11485,75 @@ public void testTimeExtractWithTooFewArguments() throws Exception
  @Parameters(source = QueryContextForJoinProvider.class)
  public void testNestedGroupByOnInlineDataSourceWithFilterIsNotSupported(Map<String, Object> queryContext) throws Exception


heh, this method should probably renamed to ...IsSupported now i guess?

Ah, fixed test name

clintropolis · 2020-06-06T05:49:47Z

  public int hashCode()
  {
-    return fields != null ? fields.hashCode() : 0;
+    if (fieldsHashCode == null) {


does this need to be threadsafe? Might want to put behind a Supplier.memoize instead?

I changed this and elsewhere to use Supplier.memoize

clintropolis · 2020-06-06T06:19:35Z

    return subtotalsSpec;
  }

-  @Override


Why this change, Query.java provides a default implementation, so doesn't it @Override?

This was a change from removing the "old rewrite mode" initially, but now that it's added back in, so I put the overrides back

clintropolis · 2020-06-06T06:26:08Z

+   * is kept temporarily available in case issues arise with the new mode, and the user does not run queries with the
+   * affected nested shape.
+   */
+  public static <T> boolean getUseJoinFilterRewriteOldRewriteMode(Query<T> query)


is it possible to detect this and use the different modes automatically since it sounds like the old mode is perhaps better if there are no subqueries involved?

Hmm, I think that could be worth looking into later on

clintropolis · 2020-06-06T06:35:10Z

+  {
+    final List<String> sortedValues = new ArrayList<>(values);
+    sortedValues.sort(Comparator.nullsFirst(Ordering.natural()));
+    final Hasher hasher = Hashing.sha256().newHasher();


I know this isn't new, but might be worth investigating if there are faster hashes that are unique enough for this (not necessary in this PR, just thinking out loud)

clintropolis · 2020-06-06T06:39:41Z

+    // to ensure that the hashCode is only computed once per Filter since the Filter interface is not thread-safe.
+    synchronized (analyses) {
+      if (filter != null) {
+        filter.hashCode();


Oh, is this why we don't need thread-safety on filter hashcode methods i guess? This seems kind of a funny way to prime them with the cached values, I think maybe the supplier.memoize pattern would be a little cleaner and make this not necessary?

Removed this after changing to supplier.memoize

clintropolis · 2020-06-06T06:50:30Z

-        true,
-        QueryContexts.DEFAULT_ENABLE_JOIN_FILTER_REWRITE_MAX_SIZE
-    );
+    JoinFilterPreAnalysisGroup joinFilterPreAnalysisGroup = makeDefaultConfigPreAnalysisGroup();


clintropolis

lgtm thanks 👍

suneet-s · 2020-06-09T15:46:19Z

    }
    AndFilter andFilter = (AndFilter) o;
-    return Objects.equals(getFilters(), andFilter.getFilters());
+    return Objects.equals(hashCode(), andFilter.hashCode());


equals should not compare hashCodes. It's possible that different and filters hash to the same hashCode.

Similar comments in the other filters

jon-wei · 2020-06-10T08:41:55Z

I'm going to close this one and open a new PR with a different approach, this one has conflicts as well.

jon-wei · 2020-06-10T08:46:58Z

Opened a new PR: #10015

Fix join filter rewrites with nested queries

3c84e62

jon-wei added Bug Area - Querying WIP labels Jun 3, 2020

Add javadocs, fix inspections

d3b3055

jon-wei removed the WIP label Jun 3, 2020

jon-wei added 2 commits June 3, 2020 13:43

Remove unused imports and method

da4af6b

Fix overrides

f96752f

suneet-s reviewed Jun 5, 2020

View reviewed changes

Address PR comments

1726531

Fixes

d74ff79

clintropolis reviewed Jun 6, 2020

View reviewed changes

jon-wei added 3 commits June 5, 2020 23:58

Restore @OverRide

8024443

Fix inspections and tests

d79b301

Memoize, change test name

b6bd57d

clintropolis approved these changes Jun 9, 2020

View reviewed changes

suneet-s reviewed Jun 9, 2020

View reviewed changes

jon-wei closed this Jun 10, 2020

jon-wei mentioned this pull request Jun 10, 2020

Fix join filter rewrites with nested queries #10015

Merged

8 tasks

		@@ -11486,79 +11485,75 @@ public void testTimeExtractWithTooFewArguments() throws Exception
		@Parameters(source = QueryContextForJoinProvider.class)
		public void testNestedGroupByOnInlineDataSourceWithFilterIsNotSupported(Map<String, Object> queryContext) throws Exception

Conversation

jon-wei commented Jun 3, 2020

Uh oh!

jon-wei commented Jun 3, 2020

Uh oh!

jon-wei commented Jun 3, 2020

Uh oh!

suneet-s 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!

jon-wei Jun 6, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Jun 6, 2020

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!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Jun 10, 2020

Uh oh!

jon-wei commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jon-wei Jun 6, 2020 •

edited

Loading

jon-wei commented Jun 10, 2020 •

edited

Loading