clintropolis commented on code in PR #15420: URL: https://github.com/apache/druid/pull/15420#discussion_r1448191941
########## processing/src/main/java/org/apache/druid/query/groupby/GroupByUtils.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.query.groupby; + +import com.fasterxml.jackson.core.type.TypeReference; +import org.apache.druid.collections.ResourceHolder; +import org.apache.druid.error.DruidException; +import org.apache.druid.query.context.ResponseContext; + +import java.nio.ByteBuffer; + +public class GroupByUtils +{ + /** + * + */ + public static final String CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION = "mergeRunnersUsingChainedExecution"; + + /** + * + */ + public static final String CTX_KEY_RUNNER_MERGES_USING_GROUP_BY_MERGING_QUERY_RUNNER_V2 = "runnerMergesUsingGroupByMergingQueryRunnerV2"; Review Comment: there are a bunch of query context group by keys in `GroupByQuery`, these should probably live there too instead of adding a new class to hold them. Alternatively, I guess they could have just stayed in GroupByMergingQueryRunnerV2 since they seem kind of specific to its function. Also please drop the "V2" from this flag name since there is only v2 now ########## processing/src/main/java/org/apache/druid/query/groupby/GroupByUtils.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.query.groupby; + +import com.fasterxml.jackson.core.type.TypeReference; +import org.apache.druid.collections.ResourceHolder; +import org.apache.druid.error.DruidException; +import org.apache.druid.query.context.ResponseContext; + +import java.nio.ByteBuffer; + +public class GroupByUtils +{ + /** + * + */ + public static final String CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION = "mergeRunnersUsingChainedExecution"; + + /** + * + */ + public static final String CTX_KEY_RUNNER_MERGES_USING_GROUP_BY_MERGING_QUERY_RUNNER_V2 = "runnerMergesUsingGroupByMergingQueryRunnerV2"; + + /** + * Reason for using this is to ensure that we donot set the merge buffers multiple times on the same response context + */ + public static final ResponseContext.Key RESPONSE_KEY_GROUP_BY_MERGING_QUERY_RUNNER_BUFFERS = Review Comment: this could just live in `GroupingEngine` or somewhere else if we move the query context parameters to `GroupByQuery` (or I suppose could also live with the query context parameters in `GroupByQuery`) ########## server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java: ########## @@ -237,13 +239,16 @@ public <T> QueryRunner<T> getQueryRunnerForIntervals(Query<T> query, Iterable<In newQuery ); } else if (canRunQueryUsingClusterWalker(newQuery)) { + Query<T> queryToRun = newQuery.withOverriddenContext( + ImmutableMap.of(GroupByUtils.CTX_KEY_RUNNER_MERGES_USING_GROUP_BY_MERGING_QUERY_RUNNER_V2, false) + ); Review Comment: why is this set to false here, and then inside of the cluster walker (which is CachingClusteredClient) it is immediately set to true? Also, shouldn't there be something for the local walker? The local walker is the only thing on the broker that calls 'mergeRunners' to use the group by merging query runner... otherwise the broker does not use it for any other query shape afaict. ########## server/src/main/java/org/apache/druid/client/CachingClusteredClient.java: ########## @@ -201,9 +202,14 @@ private <T> Sequence<T> run( final boolean specificSegments ) { - final ClusterQueryResult<T> result = new SpecificQueryRunnable<>(queryPlus, responseContext) + QueryPlus<T> queryPlus1 = queryPlus.withQuery( + queryPlus.getQuery().withOverriddenContext( + ImmutableMap.of(GroupByUtils.CTX_KEY_RUNNER_MERGES_USING_GROUP_BY_MERGING_QUERY_RUNNER_V2, true) + ) + ); + final ClusterQueryResult<T> result = new SpecificQueryRunnable<>(queryPlus1, responseContext) Review Comment: this seems off to apply it to all query types, when it only affects group by queries. Also, it doesn't really seem quite correct either, since if `CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION` is set, or if 'by segment' is set, mergeRunners does not take a merge buffer. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
