You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/03 06:59:32 UTC
[GitHub] [druid] jon-wei opened a new pull request #9978: Fix join filter rewrites with nested queries
jon-wei opened a new pull request #9978:
URL: https://github.com/apache/druid/pull/9978
Fixes https://github.com/apache/druid/issues/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](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (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](https://github.com/apache/druid/blob/master/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #9978:
URL: https://github.com/apache/druid/pull/9978#issuecomment-637998965
Marking WIP, need to adjust javadocs, will also see test coverage results
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436031493
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ null,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ } else {
+ jfpa = joinFilterPreAnalysisGroup.getAnalyses().computeIfAbsent(
filter,
- joinFilterPreAnalysis.getOriginalFilter()
+ (theFilter) -> {
+ return JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ theFilter,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ }
Review comment:
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.
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
Review comment:
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.
##########
File path: processing/src/main/java/org/apache/druid/query/Query.java
##########
@@ -170,9 +169,4 @@ default String getSqlQueryId()
{
return withOverriddenContext(ImmutableMap.of(QueryContexts.LANE_KEY, lane));
}
-
- default VirtualColumns getVirtualColumns()
Review comment:
I'm assuming this change has nothing to do with the bug fix correct?
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/JoinFilterPreAnalysis.java
##########
@@ -51,8 +52,7 @@
private final List<Filter> normalizedBaseTableClauses;
private final List<Filter> normalizedJoinTableClauses;
private final JoinFilterCorrelations correlations;
- private final boolean enableFilterPushDown;
- private final boolean enableFilterRewrite;
+ private final JoinFilterPreAnalysisGroup myGroup;
Review comment:
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`
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ null,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ } else {
+ jfpa = joinFilterPreAnalysisGroup.getAnalyses().computeIfAbsent(
filter,
- joinFilterPreAnalysis.getOriginalFilter()
+ (theFilter) -> {
+ return JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ theFilter,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ }
Review comment:
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.
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/rewrite/JoinFilterPreAnalysisGroup.java
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.join.filter.rewrite;
+
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.segment.join.filter.JoinFilterPreAnalysis;
+
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * A JoinFilterPreAnalysisGroup holds all of the JoinFilterPreAnalysis objects for a given query and
+ * also stores the per-query parameters that control the filter rewrite operations (from the query context).
+ *
+ * The analyses map is keyed by Filter: each Filter in the map belongs to separate level of query
+ * (e.g. outer query, subquery level 1, etc.)
+ *
+ * A concurrent hash map is used since the per-level Filter processing occurs across multiple threads.
+ */
+public class JoinFilterPreAnalysisGroup
+{
+ private final boolean enableFilterPushDown;
+ private final boolean enableFilterRewrite;
+ private final boolean enableRewriteValueColumnFilters;
+ private final long filterRewriteMaxSize;
+ private final ConcurrentHashMap<Filter, JoinFilterPreAnalysis> analyses;
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #9978:
URL: https://github.com/apache/druid/pull/9978#issuecomment-641846833
I'm going to close this one and open a new PR with a different approach, this one has conflicts as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r437070035
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ null,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ } else {
+ jfpa = joinFilterPreAnalysisGroup.getAnalyses().computeIfAbsent(
filter,
- joinFilterPreAnalysis.getOriginalFilter()
+ (theFilter) -> {
+ return JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ theFilter,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ }
Review comment:
I've added a `computeJoinFilterPreAnalysisIfAbsent` and `getAnalysis` method to `JoinFilterPreAnalysisGroup`
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -11486,79 +11485,75 @@ public void testTimeExtractWithTooFewArguments() throws Exception
@Parameters(source = QueryContextForJoinProvider.class)
public void testNestedGroupByOnInlineDataSourceWithFilterIsNotSupported(Map<String, Object> queryContext) throws Exception
Review comment:
Ah, fixed test name
##########
File path: processing/src/main/java/org/apache/druid/query/filter/AndDimFilter.java
##########
@@ -142,7 +147,10 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return fields != null ? fields.hashCode() : 0;
+ if (fieldsHashCode == null) {
Review comment:
I changed this and elsewhere to use Supplier.memoize
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java
##########
@@ -249,7 +249,6 @@ private GroupByQuery(
return subtotalsSpec;
}
- @Override
Review comment:
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
##########
File path: processing/src/main/java/org/apache/druid/query/QueryContexts.java
##########
@@ -264,6 +266,26 @@ public String toString()
return parseBoolean(query, JOIN_FILTER_REWRITE_ENABLE_KEY, DEFAULT_ENABLE_JOIN_FILTER_REWRITE);
}
+ /**
+ * This is an undocumented option provided as a transition tool:
+ *
+ * The join filter rewrites originally performed the pre-analysis phase prior to any per-segment processing,
+ * analyzing only the filter in the top-level of the query.
+ *
+ * This did not work for nested queries (see https://github.com/apache/druid/pull/9978), so the rewrite pre-analysis
+ * was moved into the cursor creation of the {@link org.apache.druid.segment.join.HashJoinSegmentStorageAdapter}.
+ * This design requires synchronization across multiple segment processing threads; the old rewrite mode
+ * 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)
Review comment:
Hmm, I think that could be worth looking into later on
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/rewrite/JoinFilterPreAnalysisGroup.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.join.filter.rewrite;
+
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.segment.VirtualColumns;
+import org.apache.druid.segment.join.JoinableClause;
+import org.apache.druid.segment.join.filter.JoinFilterAnalyzer;
+import org.apache.druid.segment.join.filter.JoinFilterPreAnalysis;
+import org.apache.druid.segment.join.filter.JoinableClauses;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * A JoinFilterPreAnalysisGroup holds all of the JoinFilterPreAnalysis objects for a given query and
+ * also stores the per-query parameters that control the filter rewrite operations (from the query context).
+ *
+ * The analyses map is keyed by (Filter, JoinableClause list, VirtualColumns): each Filter in the map belongs to a
+ * separate level of query (e.g. outer query, subquery level 1, etc.)
+ *
+ * A concurrent hash map is used since the per-level Filter processing occurs across multiple threads.
+ */
+public class JoinFilterPreAnalysisGroup
+{
+ private final JoinFilterRewriteConfig joinFilterRewriteConfig;
+ private final ConcurrentHashMap<JoinFilterPreAnalysisGroupKey, JoinFilterPreAnalysis> analyses;
+
+ /**
+ * This is an undocumented option provided as a transition tool:
+ *
+ * The join filter rewrites originally performed the pre-analysis phase prior to any per-segment processing,
+ * analyzing only the filter in the top-level of the query.
+ *
+ * This did not work for nested queries (see https://github.com/apache/druid/pull/9978), so the rewrite pre-analysis
+ * was moved into the cursor creation of the {@link org.apache.druid.segment.join.HashJoinSegmentStorageAdapter}.
+ * This design requires synchronization across multiple segment processing threads; the old rewrite mode
+ * is kept temporarily available in case issues arise with the new mode, and the user does not run queries with the
+ * affected nested shape.
+ */
+ private JoinFilterPreAnalysis preAnalysisForOldRewriteMode;
+
+ public JoinFilterPreAnalysisGroup(
+ JoinFilterRewriteConfig joinFilterRewriteConfig
+ )
+ {
+ this.joinFilterRewriteConfig = joinFilterRewriteConfig;
+ this.analyses = new ConcurrentHashMap<>();
+ }
+
+ public JoinFilterRewriteConfig getJoinFilterRewriteConfig()
+ {
+ return joinFilterRewriteConfig;
+ }
+
+ public JoinFilterPreAnalysis computeJoinFilterPreAnalysisIfAbsent(
+ Filter filter,
+ List<JoinableClause> clauses,
+ VirtualColumns virtualColumns
+ )
+ {
+ // Some filters have potentially expensive hash codes that are lazily computed and cached.
+ // We call hashCode() here in a synchronized block before we attempt to use the Filter in the analyses map,
+ // 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();
Review comment:
Removed this after changing to supplier.memoize
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #9978:
URL: https://github.com/apache/druid/pull/9978#issuecomment-641849875
Opened a new issue: https://github.com/apache/druid/pull/10015
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436237625
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
Review comment:
I adjusted the analysis group so the key is now (Filter, JoinableClauses, VirtualColumns) instead of filter, I think that's a more correct key, and the null filters are cached now.
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436241367
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -11486,79 +11485,75 @@ public void testTimeExtractWithTooFewArguments() throws Exception
@Parameters(source = QueryContextForJoinProvider.class)
public void testNestedGroupByOnInlineDataSourceWithFilterIsNotSupported(Map<String, Object> queryContext) throws Exception
Review comment:
heh, this method should probably renamed to `...IsSupported` now i guess?
##########
File path: processing/src/main/java/org/apache/druid/query/QueryContexts.java
##########
@@ -264,6 +266,26 @@ public String toString()
return parseBoolean(query, JOIN_FILTER_REWRITE_ENABLE_KEY, DEFAULT_ENABLE_JOIN_FILTER_REWRITE);
}
+ /**
+ * This is an undocumented option provided as a transition tool:
+ *
+ * The join filter rewrites originally performed the pre-analysis phase prior to any per-segment processing,
+ * analyzing only the filter in the top-level of the query.
+ *
+ * This did not work for nested queries (see https://github.com/apache/druid/pull/9978), so the rewrite pre-analysis
+ * was moved into the cursor creation of the {@link org.apache.druid.segment.join.HashJoinSegmentStorageAdapter}.
+ * This design requires synchronization across multiple segment processing threads; the old rewrite mode
+ * 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)
Review comment:
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?
##########
File path: processing/src/test/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapterTest.java
##########
@@ -302,21 +298,13 @@ public void test_makeCursors_factToCountryLeft()
{
List<JoinableClause> joinableClauses = ImmutableList.of(factToCountryOnIsoCode(JoinType.LEFT));
- JoinFilterPreAnalysis preAnalysis = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
- JoinableClauses.fromList(joinableClauses),
- VirtualColumns.EMPTY,
- null,
- true,
- true,
- true,
- QueryContexts.DEFAULT_ENABLE_JOIN_FILTER_REWRITE_MAX_SIZE
- );
+ JoinFilterPreAnalysisGroup joinFilterPreAnalysisGroup = makeDefaultConfigPreAnalysisGroup();
Review comment:
nice
##########
File path: processing/src/main/java/org/apache/druid/query/filter/AndDimFilter.java
##########
@@ -142,7 +147,10 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return fields != null ? fields.hashCode() : 0;
+ if (fieldsHashCode == null) {
Review comment:
does this need to be threadsafe? Might want to put behind a `Supplier.memoize` instead?
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java
##########
@@ -249,7 +249,6 @@ private GroupByQuery(
return subtotalsSpec;
}
- @Override
Review comment:
Why this change, `Query.java` provides a default implementation, so doesn't it `@Override`?
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/rewrite/JoinFilterPreAnalysisGroup.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.join.filter.rewrite;
+
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.segment.VirtualColumns;
+import org.apache.druid.segment.join.JoinableClause;
+import org.apache.druid.segment.join.filter.JoinFilterAnalyzer;
+import org.apache.druid.segment.join.filter.JoinFilterPreAnalysis;
+import org.apache.druid.segment.join.filter.JoinableClauses;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * A JoinFilterPreAnalysisGroup holds all of the JoinFilterPreAnalysis objects for a given query and
+ * also stores the per-query parameters that control the filter rewrite operations (from the query context).
+ *
+ * The analyses map is keyed by (Filter, JoinableClause list, VirtualColumns): each Filter in the map belongs to a
+ * separate level of query (e.g. outer query, subquery level 1, etc.)
+ *
+ * A concurrent hash map is used since the per-level Filter processing occurs across multiple threads.
+ */
+public class JoinFilterPreAnalysisGroup
+{
+ private final JoinFilterRewriteConfig joinFilterRewriteConfig;
+ private final ConcurrentHashMap<JoinFilterPreAnalysisGroupKey, JoinFilterPreAnalysis> analyses;
+
+ /**
+ * This is an undocumented option provided as a transition tool:
+ *
+ * The join filter rewrites originally performed the pre-analysis phase prior to any per-segment processing,
+ * analyzing only the filter in the top-level of the query.
+ *
+ * This did not work for nested queries (see https://github.com/apache/druid/pull/9978), so the rewrite pre-analysis
+ * was moved into the cursor creation of the {@link org.apache.druid.segment.join.HashJoinSegmentStorageAdapter}.
+ * This design requires synchronization across multiple segment processing threads; the old rewrite mode
+ * is kept temporarily available in case issues arise with the new mode, and the user does not run queries with the
+ * affected nested shape.
+ */
+ private JoinFilterPreAnalysis preAnalysisForOldRewriteMode;
+
+ public JoinFilterPreAnalysisGroup(
+ JoinFilterRewriteConfig joinFilterRewriteConfig
+ )
+ {
+ this.joinFilterRewriteConfig = joinFilterRewriteConfig;
+ this.analyses = new ConcurrentHashMap<>();
+ }
+
+ public JoinFilterRewriteConfig getJoinFilterRewriteConfig()
+ {
+ return joinFilterRewriteConfig;
+ }
+
+ public JoinFilterPreAnalysis computeJoinFilterPreAnalysisIfAbsent(
+ Filter filter,
+ List<JoinableClause> clauses,
+ VirtualColumns virtualColumns
+ )
+ {
+ // Some filters have potentially expensive hash codes that are lazily computed and cached.
+ // We call hashCode() here in a synchronized block before we attempt to use the Filter in the analyses map,
+ // 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();
Review comment:
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?
##########
File path: processing/src/main/java/org/apache/druid/query/filter/InDimFilter.java
##########
@@ -149,28 +153,36 @@ public FilterTuning getFilterTuning()
@Override
public byte[] getCacheKey()
{
+ if (valuesHashCode == null) {
+ valuesHashCode = computeValuesHashCode(values);
+ }
if (cacheKey == null) {
- final List<String> sortedValues = new ArrayList<>(values);
- sortedValues.sort(Comparator.nullsFirst(Ordering.natural()));
- final Hasher hasher = Hashing.sha256().newHasher();
- for (String v : sortedValues) {
- if (v == null) {
- hasher.putInt(0);
- } else {
- hasher.putString(v, StandardCharsets.UTF_8);
- }
- }
cacheKey = new CacheKeyBuilder(DimFilterUtils.IN_CACHE_ID)
.appendString(dimension)
.appendByte(DimFilterUtils.STRING_SEPARATOR)
.appendByteArray(extractionFn == null ? new byte[0] : extractionFn.getCacheKey())
.appendByte(DimFilterUtils.STRING_SEPARATOR)
- .appendByteArray(hasher.hash().asBytes())
+ .appendByteArray(valuesHashCode.asBytes())
.build();
}
return cacheKey;
}
+ private static HashCode computeValuesHashCode(Set<String> values)
+ {
+ final List<String> sortedValues = new ArrayList<>(values);
+ sortedValues.sort(Comparator.nullsFirst(Ordering.natural()));
+ final Hasher hasher = Hashing.sha256().newHasher();
Review comment:
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436237683
##########
File path: processing/src/main/java/org/apache/druid/query/Query.java
##########
@@ -170,9 +169,4 @@ default String getSqlQueryId()
{
return withOverriddenContext(ImmutableMap.of(QueryContexts.LANE_KEY, lane));
}
-
- default VirtualColumns getVirtualColumns()
Review comment:
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r437534343
##########
File path: processing/src/main/java/org/apache/druid/segment/filter/AndFilter.java
##########
@@ -257,12 +264,12 @@ public boolean equals(Object o)
return false;
}
AndFilter andFilter = (AndFilter) o;
- return Objects.equals(getFilters(), andFilter.getFilters());
+ return Objects.equals(hashCode(), andFilter.hashCode());
Review comment:
equals should not compare hashCodes. It's possible that different and filters hash to the same hashCode.
Similar comments in the other filters
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #9978:
URL: https://github.com/apache/druid/pull/9978#issuecomment-638405946
Added javadocs and fixed some inspections, removing WIP
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436051470
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/JoinFilterPreAnalysis.java
##########
@@ -51,8 +52,7 @@
private final List<Filter> normalizedBaseTableClauses;
private final List<Filter> normalizedJoinTableClauses;
private final JoinFilterCorrelations correlations;
- private final boolean enableFilterPushDown;
- private final boolean enableFilterRewrite;
+ private final JoinFilterPreAnalysisGroup myGroup;
Review comment:
Did some reading... Java's GC is smart enough to handle circular dependencies, so I _think_ this should be ok.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436040946
##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentStorageAdapter.java
##########
@@ -209,13 +209,36 @@ public Metadata getMetadata()
@Nullable final QueryMetrics<?> queryMetrics
)
{
- if (!Objects.equals(joinFilterPreAnalysis.getOriginalFilter(), filter)) {
- throw new ISE(
- "Filter provided to cursor [%s] does not match join pre-analysis filter [%s]",
+ JoinFilterPreAnalysis jfpa;
+ if (filter == null) {
+ jfpa = JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ null,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ } else {
+ jfpa = joinFilterPreAnalysisGroup.getAnalyses().computeIfAbsent(
filter,
- joinFilterPreAnalysis.getOriginalFilter()
+ (theFilter) -> {
+ return JoinFilterAnalyzer.computeJoinFilterPreAnalysis(
+ joinFilterPreAnalysisGroup,
+ JoinableClauses.fromList(clauses),
+ virtualColumns,
+ theFilter,
+ joinFilterPreAnalysisGroup.isEnableFilterPushDown(),
+ joinFilterPreAnalysisGroup.isEnableFilterRewrite(),
+ joinFilterPreAnalysisGroup.isEnableRewriteValueColumnFilters(),
+ joinFilterPreAnalysisGroup.getFilterRewriteMaxSize()
+ );
+ }
Review comment:
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436237539
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/JoinFilterPreAnalysis.java
##########
@@ -51,8 +52,7 @@
private final List<Filter> normalizedBaseTableClauses;
private final List<Filter> normalizedJoinTableClauses;
private final JoinFilterCorrelations correlations;
- private final boolean enableFilterPushDown;
- private final boolean enableFilterRewrite;
+ private final JoinFilterPreAnalysisGroup myGroup;
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #9978:
URL: https://github.com/apache/druid/pull/9978#discussion_r436237490
##########
File path: processing/src/main/java/org/apache/druid/segment/join/filter/rewrite/JoinFilterPreAnalysisGroup.java
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.join.filter.rewrite;
+
+import org.apache.druid.query.filter.Filter;
+import org.apache.druid.segment.join.filter.JoinFilterPreAnalysis;
+
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * A JoinFilterPreAnalysisGroup holds all of the JoinFilterPreAnalysis objects for a given query and
+ * also stores the per-query parameters that control the filter rewrite operations (from the query context).
+ *
+ * The analyses map is keyed by Filter: each Filter in the map belongs to separate level of query
+ * (e.g. outer query, subquery level 1, etc.)
+ *
+ * A concurrent hash map is used since the per-level Filter processing occurs across multiple threads.
+ */
+public class JoinFilterPreAnalysisGroup
+{
+ private final boolean enableFilterPushDown;
+ private final boolean enableFilterRewrite;
+ private final boolean enableRewriteValueColumnFilters;
+ private final long filterRewriteMaxSize;
+ private final ConcurrentHashMap<Filter, JoinFilterPreAnalysis> analyses;
Review comment:
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei edited a comment on pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei edited a comment on pull request #9978:
URL: https://github.com/apache/druid/pull/9978#issuecomment-641849875
Opened a new PR: https://github.com/apache/druid/pull/10015
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #9978:
URL: https://github.com/apache/druid/pull/9978#issuecomment-639982397
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei closed pull request #9978: Fix join filter rewrites with nested queries
Posted by GitBox <gi...@apache.org>.
jon-wei closed pull request #9978:
URL: https://github.com/apache/druid/pull/9978
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org