You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kgyrtkirk (via GitHub)" <gi...@apache.org> on 2025/02/27 15:53:39 UTC
[PR] Introduce ExecutionVertex (druid)
kgyrtkirk opened a new pull request, #17763:
URL: https://github.com/apache/druid/pull/17763
Replaces `DataSourceAnalysis` with `ExecutionVertex`; the most important difference is that the vertex is defined over a `Query`
Some methods were renamed; `ExecutionVertex` should help in working with a `Query+DataSource` batch which is executable in one go.
includes the following refactors:
* removal of `DataSource#getAnalysis`
* removal of `DataSource#withUpdatedDataSource`
* removal of `Query#getDataSourceAnalysis`
* join related parts of `DataSourceAnalysis` live as `JoinDataSourceAnalysis`
* `isConcreate` was renamed to `isProcessable` which
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2022825455
##########
processing/src/main/java/org/apache/druid/query/TableDataSource.java:
##########
@@ -125,13 +119,7 @@ public DataSource withPolicies(Map<String, Optional<Policy>> policyMap)
@Override
public byte[] getCacheKey()
{
- return new byte[0];
- }
-
- @Override
- public DataSourceAnalysis getAnalysis()
- {
- return new DataSourceAnalysis(this, null, null, Collections.emptyList(), null);
+ return getName().getBytes(StandardCharsets.UTF_8);
Review Comment:
I've moved the datasources cachekeys to `DataSource`
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on PR #17763:
URL: https://github.com/apache/druid/pull/17763#issuecomment-2786058548
@gianm could you please take another look?
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2023684984
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnionDataSource;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Represents the native engine's execution vertex.
+ *
+ * Most likely due to evolutional purposes there are some concepts which went
+ * beyond their inital design:
+ *
+ * Multiple queries might be executed in one stage: <br/>
+ * The GroupBy query could be collapsed at exection time).
+ *
+ * Dag of datasources: <br/>
+ * an execution may process an entire dag of datasource in some cases
+ * (joindatasource) ; or collapse some into the execution (filter)
+ */
+public class ExecutionVertex
+{
+ protected final Query<?> topQuery;
+ protected final DataSource baseDataSource;
+ protected final QuerySegmentSpec querySegmentSpec;
+ protected final List<String> joinPrefixes;
Review Comment:
Ok, that sounds great. Thanks for writing those thoughts 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #17763:
URL: https://github.com/apache/druid/pull/17763#issuecomment-2787140415
LGTM, thank you! There was one failing integration test, which might have been a flake. I re-ran it.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2032944957
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -97,9 +98,6 @@ public boolean isCacheable(boolean isBroker)
@Override
public boolean isGlobal()
{
- // FIXME: this was query.getDataSource().isGlobal()
- // but it feeled dodgy to me
- // in general: I think isProcessable should be a precondition to isGblobal
return query.getDataSource().isGlobal();
Review Comment:
actually; with the most recent changes - I think this would not cause any difference anymore ; updated to return `false`
the tests which needed modifications are in [this commit](https://github.com/apache/druid/pull/17763/commits/f28a5aa28b46ea7ca69784d13fe78c609263e7c6)
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2031137249
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -79,29 +86,22 @@ public interface DataSource extends Cacheable
* Decides if this datasource can be accessed globally.
*
* This means that all servers have a full copy of this datasource.
- * True for things like inline, lookup, etc, or for queries of those.
- * <p>
- * Currently this is coupled with joinability - if this returns true then the query engine expects there exists a
- * {@link org.apache.druid.segment.join.JoinableFactory} which might build a
- * {@link org.apache.druid.segment.join.Joinable} for this datasource directly. If a subquery 'inline' join is
- * required to join this datasource on the right hand side, then this value must be false for now.
- * <p>
- * In the future, instead of directly using this method, the query planner and engine should consider
- * {@link org.apache.druid.segment.join.JoinableFactory#isDirectlyJoinable(DataSource)} when determining if the
- * right hand side is directly joinable, which would allow decoupling this property from joins.
+ *
+ * Examples: inline table, lookup.
*/
boolean isGlobal();
/**
* Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Processable examples are: Tables / Inline datasources.
+ * A Processable datasource could pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
+ *
+ * Processable examples are: {@link TableDataSource}, {@link InlineDataSource}, {@link FilteredDataSource}.
* Non-processable ones are those which need further pre-processing before running them.
+ * examples are: {@link QueryDataSource} and join which are not supported directly.
* </p>
- * Processing boundaries are identified {@link ExecutionVertex}.
+ * Processing boundaries could be identified with {@link ExecutionVertex}.
Review Comment:
I've removed this cloudy sentence :)
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2024902815
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnionDataSource;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Represents the native engine's execution vertex.
+ *
+ * Most likely due to evolutional purposes there are some concepts which went
+ * beyond their inital design:
+ *
+ * Multiple queries might be executed in one stage: <br/>
+ * The GroupBy query could be collapsed at exection time).
+ *
+ * Dag of datasources: <br/>
+ * an execution may process an entire dag of datasource in some cases
+ * (joindatasource) ; or collapse some into the execution (filter)
+ */
Review Comment:
updated the apidoc - it got much better
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2023124347
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnionDataSource;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Represents the native engine's execution vertex.
+ *
+ * Most likely due to evolutional purposes there are some concepts which went
+ * beyond their inital design:
+ *
+ * Multiple queries might be executed in one stage: <br/>
+ * The GroupBy query could be collapsed at exection time).
+ *
+ * Dag of datasources: <br/>
+ * an execution may process an entire dag of datasource in some cases
+ * (joindatasource) ; or collapse some into the execution (filter)
+ */
+public class ExecutionVertex
+{
+ protected final Query<?> topQuery;
+ protected final DataSource baseDataSource;
+ protected final QuerySegmentSpec querySegmentSpec;
+ protected final List<String> joinPrefixes;
Review Comment:
I agree that `joinPrefixes` are out of scope - and they are here due to how `DataSourceAnalysis` was used.
I imagine that we will have a `leafs` field about the input `DataSource`-s in this class instead of these at some point.
The `joinPrefixes` / `isBaseColumn` could possibly be removed after when the `segmentMapFunction` is done differently: I think `segmentMapFunction` (beyond the mapper function) should also return filtering details which could be applied to the input segment; that's part of the next step.
I think the `isSegmentMapFunctionExpensive` will also find a more natural place in that world - as around the place where `isJoin` is called the question is already around the segmentMap function.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2031111949
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -79,29 +86,22 @@ public interface DataSource extends Cacheable
* Decides if this datasource can be accessed globally.
*
* This means that all servers have a full copy of this datasource.
- * True for things like inline, lookup, etc, or for queries of those.
- * <p>
- * Currently this is coupled with joinability - if this returns true then the query engine expects there exists a
- * {@link org.apache.druid.segment.join.JoinableFactory} which might build a
- * {@link org.apache.druid.segment.join.Joinable} for this datasource directly. If a subquery 'inline' join is
- * required to join this datasource on the right hand side, then this value must be false for now.
- * <p>
- * In the future, instead of directly using this method, the query planner and engine should consider
- * {@link org.apache.druid.segment.join.JoinableFactory#isDirectlyJoinable(DataSource)} when determining if the
- * right hand side is directly joinable, which would allow decoupling this property from joins.
+ *
+ * Examples: inline table, lookup.
*/
boolean isGlobal();
/**
* Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Processable examples are: Tables / Inline datasources.
+ * A Processable datasource could pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
Review Comment:
changed to:
```
A Processable datasource must be able to pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
```
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2013053737
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessorFactory.java:
##########
@@ -339,7 +340,8 @@ private FrameProcessor<Function<SegmentReference, SegmentReference>> makeSegment
);
if (broadcastInputs.isEmpty()) {
- if (query.getDataSourceAnalysis().isJoin()) {
+ ExecutionVertex ev = ExecutionVertex.of(query);
+ if (ev.isJoin()) {
Review Comment:
would be nice to retain the single-lineliness: `ExecutionVertext.of(query).isJoin()`
##########
processing/src/main/java/org/apache/druid/java/util/common/Cacheable.java:
##########
@@ -24,7 +24,7 @@ public interface Cacheable
/**
* Get a byte array used as a cache key.
*
- * @return a cache key
+ * @return bytes to be used as cache key - or null if this object should not be cached.
*/
byte[] getCacheKey();
Review Comment:
nit- add `@Nullable` since null is a possible return.
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -151,12 +143,6 @@ default DataSource withPolicies(Map<String, Optional<Policy>> policyMap)
*
* @return the cache key to be used as part of query cache key
*/
+ @Override
Review Comment:
We probably need this to be null for non-cacheable datasources, so please add `@Nullable`.
##########
processing/src/main/java/org/apache/druid/query/cache/CacheKeyBuilder.java:
##########
@@ -108,7 +107,6 @@ private static byte[] cacheableToByteArray(@Nullable Cacheable cacheable)
return EMPTY_BYTES;
} else {
final byte[] key = cacheable.getCacheKey();
- Preconditions.checkArgument(!Arrays.equals(key, EMPTY_BYTES), "cache key is equal to the empty key");
Review Comment:
It looks like you already edited `TableDataSource` to use the table name as cache key. Could this check for empty cache keys be restored?
##########
processing/src/main/java/org/apache/druid/query/filter/DimFilterUtils.java:
##########
@@ -155,20 +155,17 @@ public static <T> Set<T> filterShards(
}
/**
- * Returns a copy of "fields" only including base fields from {@link DataSourceAnalysis}.
- *
- * @param fields field list, must be nonnull
- * @param dataSourceAnalysis analyzed datasource
+ * Returns a copy of "fields" filtered by the predicate function.
Review Comment:
please include some description of what `isBaseColumnFn` is used for.
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnionDataSource;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Represents the native engine's execution vertex.
+ *
+ * Most likely due to evolutional purposes there are some concepts which went
+ * beyond their inital design:
+ *
+ * Multiple queries might be executed in one stage: <br/>
+ * The GroupBy query could be collapsed at exection time).
+ *
+ * Dag of datasources: <br/>
+ * an execution may process an entire dag of datasource in some cases
+ * (joindatasource) ; or collapse some into the execution (filter)
+ */
+public class ExecutionVertex
+{
+ protected final Query<?> topQuery;
Review Comment:
each of these fields (or, alternatively, the getter methods) should include javadoc about what they mean, including examples when necessary for clarity.
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -98,13 +97,16 @@ public boolean isCacheable(boolean isBroker)
@Override
public boolean isGlobal()
{
+ // FIXME: this was query.getDataSource().isGlobal()
Review Comment:
Does any test fail if we change this and `isGlobal` to both `return false`? IMO, given how processable and global are defined, it makes more sense to consider `query` as neither processable nor global.
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -548,13 +506,15 @@ private static DataSourceAnalysis constructAnalysis(final JoinDataSource dataSou
@Nullable
private static DimFilter validateLeftFilter(final DataSource leftDataSource, @Nullable final DimFilter leftFilter)
{
+ if (leftFilter == null || TrueDimFilter.instance().equals(leftFilter)) {
+ return null;
+ }
// Currently we only support leftFilter when applied to concrete leaf datasources (ones with no children).
// Note that this mean we don't support unions of table, even though this would be reasonable to add in the future.
Preconditions.checkArgument(
- leftFilter == null || (leftDataSource.isConcrete() && leftDataSource.getChildren().isEmpty()),
+ leftDataSource.isProcessable() && leftDataSource.getChildren().isEmpty(),
Review Comment:
The comments and error mesasge don't seem right here. For example, a FilteredDataSource on top of a TableDataSource would return `true` for `isProcessable`, but that wouldn't be "one with no children" nor would it be "direct table access".
This problem predates the PR, because even in the prior code, `isConcrete` would return `true`.
Is this problem something you can fix in this PR, though, or possibly fix in a follow-up?
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -90,33 +93,22 @@ public interface DataSource
boolean isGlobal();
/**
- * Returns true if this datasource can be the base datasource of query processing.
- * <p>
- * Base datasources drive query processing. If the base datasource is {@link TableDataSource}, for example, queries
- * are processed in parallel on data servers. If the base datasource is {@link InlineDataSource}, queries are
- * processed on the Broker. See {@link DataSourceAnalysis#getBaseDataSource()} for further discussion.
+ * Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Datasources that are *not* concrete must be pre-processed in some way before they can be processed by the main
- * query stack. For example, {@link QueryDataSource} must be executed first and substituted with its results.
- *
- * @see DataSourceAnalysis#isConcreteBased() which uses this
- * @see DataSourceAnalysis#isConcreteAndTableBased() which uses this
+ * Processable examples are: Tables / Inline datasources.
Review Comment:
This description is IMO unclear. What about "wrapper" datasources that can be applied as segment-map functions, such as `unnest` or `join`? Are these "processable"?
Perhaps a good definition would be that "processable" means _either_:
- a physical datasource that corresponds directly to some set of physical units of data (such as `inline` or `table`)
- a datasource that wraps such a physical datasource and where `createSegmentMapFunction` returns some valid mapping function that fully encapsulates the logic.
This definition of "processable" would correspond exactly to datasources that can be used to provide the `Segment` that is passed to `QueryRunnerFactory#createRunner`.
With this definition, it would make sense that for non-processable datasources, `createSegmentMapFunction` should throw an error.
For example:
- `table`, `lookup`, `inline` are processable
- `query` is not processable (and `createSegmentMapFn` throws an error)
- `filtered` -> `table` is processable
- `join` -> [`table`, `inline`] is processable
- `join` -> [`table`, `table`] is not processable unless the second `table` is global (and `createSegmentMapFn` in this case should throw an error)
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -90,33 +93,22 @@ public interface DataSource
boolean isGlobal();
/**
- * Returns true if this datasource can be the base datasource of query processing.
- * <p>
- * Base datasources drive query processing. If the base datasource is {@link TableDataSource}, for example, queries
- * are processed in parallel on data servers. If the base datasource is {@link InlineDataSource}, queries are
- * processed on the Broker. See {@link DataSourceAnalysis#getBaseDataSource()} for further discussion.
+ * Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Datasources that are *not* concrete must be pre-processed in some way before they can be processed by the main
- * query stack. For example, {@link QueryDataSource} must be executed first and substituted with its results.
- *
- * @see DataSourceAnalysis#isConcreteBased() which uses this
- * @see DataSourceAnalysis#isConcreteAndTableBased() which uses this
+ * Processable examples are: Tables / Inline datasources.
+ * Non-processable ones are those which need further pre-processing before running them.
+ * </p>
+ * Processing boundaries are identified {@link ExecutionVertex}.
*/
- boolean isConcrete();
+ //FIXME: maybe this should renamed to "canBeReadDirectly"
+ // the need seems to be to have a flag which communicates that it could be read directly.
+ boolean isProcessable();
/**
* Returns a segment function on to how to segment should be modified.
*/
Function<SegmentReference, SegmentReference> createSegmentMapFunction(Query query);
- /**
- * Returns an updated datasource based on the specified new source.
- *
- * @param newSource the new datasource to be used to update an existing query
- * @return the updated datasource to be used
- */
- DataSource withUpdatedDataSource(DataSource newSource);
Review Comment:
nice to see this method going away.
##########
server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java:
##########
@@ -80,21 +81,28 @@ public LocalQuerySegmentWalker(
@Override
public <T> QueryRunner<T> getQueryRunnerForIntervals(final Query<T> query, final Iterable<Interval> intervals)
{
+ ExecutionVertex ev = ExecutionVertex.of(query);
final DataSource dataSourceFromQuery = query.getDataSource();
- final DataSourceAnalysis analysis = query.getDataSourceAnalysis();
- if (!analysis.isConcreteBased() || !dataSourceFromQuery.isGlobal()) {
+ if (!ev.canRunQueryUsingLocalWalker()) {
throw new IAE("Cannot query dataSource locally: %s", dataSourceFromQuery);
}
// wrap in ReferenceCountingSegment, these aren't currently managed by SegmentManager so reference tracking doesn't
// matter, but at least some or all will be in a future PR
final Iterable<ReferenceCountingSegment> segments =
- FunctionalIterable.create(segmentWrangler.getSegmentsForIntervals(analysis.getBaseDataSource(), intervals))
+ FunctionalIterable.create(segmentWrangler.getSegmentsForIntervals(ev.getBaseDataSource(), intervals))
.transform(ReferenceCountingSegment::wrapRootGenerationSegment);
final AtomicLong cpuAccumulator = new AtomicLong(0L);
+ if (dataSourceFromQuery instanceof QueryDataSource) {
+ // is this a possible bug? dataSourceFromQuery is QueryDataSource ; query is GBY with canPerformSubquery
+ // FIXME: let's not fix this
+ // throw DruidException.defensive("This might not work correctly");
+ }
+
+ // segmentMapFn will ignore all DS maps ass QDS will return identity
Review Comment:
Trying to think through how this should work for queries where the toolchest can perform a subquery. In core this only happens for `groupBy` with a `query` datasource containing another `groupBy`. In this situation the toolchest returns true from `canPerformSubquery`.
IMO, in this case it would make sense to do this:
- QueryDataSource throws an error from `createSegmentMapFunction` (as mentioned in another review comment)
- Prior to calling `createSegmentMapFunction`, this code, in LocalQuerySegmentWalker, strips stacks of `query` datasources that are handleable by the toolchest. The idea is that it needs to get to the processable datasource that is embedded under the stack of toolchest-handleable `query`.
##########
processing/src/main/java/org/apache/druid/query/TableDataSource.java:
##########
@@ -125,13 +119,7 @@ public DataSource withPolicies(Map<String, Optional<Policy>> policyMap)
@Override
public byte[] getCacheKey()
{
- return new byte[0];
- }
-
- @Override
- public DataSourceAnalysis getAnalysis()
- {
- return new DataSourceAnalysis(this, null, null, Collections.emptyList(), null);
+ return getName().getBytes(StandardCharsets.UTF_8);
Review Comment:
With cache keys, to prevent possible collisions between keys of different implementations, there should always be some kind of type code. For example consider starting all `table` cache keys with `0x01`. For convenience, these type codes can be stored in a holder class, such as how we include `DimFilter` type codes in `DimFilterUtils`.
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalDataSource.java:
##########
@@ -118,9 +117,10 @@ public boolean isGlobal()
}
@Override
- public boolean isConcrete()
+ public boolean isProcessable()
{
- return false;
+ // FIXME return false seemed illogical
Review Comment:
agree that `true` makes more sense, since the `external` datasource refers directly to physical files.
##########
processing/src/main/java/org/apache/druid/query/cache/CacheKeyBuilder.java:
##########
@@ -308,6 +312,9 @@ private void appendItem(byte typeKey, byte[] input)
public byte[] build()
Review Comment:
`@Nullable` please
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnionDataSource;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Represents the native engine's execution vertex.
+ *
+ * Most likely due to evolutional purposes there are some concepts which went
+ * beyond their inital design:
+ *
+ * Multiple queries might be executed in one stage: <br/>
+ * The GroupBy query could be collapsed at exection time).
+ *
+ * Dag of datasources: <br/>
+ * an execution may process an entire dag of datasource in some cases
+ * (joindatasource) ; or collapse some into the execution (filter)
+ */
+public class ExecutionVertex
+{
+ protected final Query<?> topQuery;
+ protected final DataSource baseDataSource;
+ protected final QuerySegmentSpec querySegmentSpec;
+ protected final List<String> joinPrefixes;
Review Comment:
The field `joinPrefixes` is questionable IMO, since it feels too join-specific. I wonder if we can get rid of it. It's used in a couple places:
- `isJoin()`, which is only used by MSQ to determine if a segment map function is expensive to compute. Seems we could replace this with something like `isSegmentMapFunctionExpensive()`.
- `isBaseColumn()`, which is used during segment pruning to ensure we only prune based on filters that apply to base columns. In theory any datasource can add or shadow underlying columns during segment mapping. Actually, even today datasources other than join can add columns. For example, the `unnest` datasource adds a column specified by `virtualColumn`. So rather than being based on `joinPrefixes` specifically, there should be some mechanism for DataSources generally to say what columns they add or shadow.
By the way, I recognize that this field and the methods that use it are inherited from `DataSourceAnalysis`. It's ok to keep it like this for this PR. I am making these comments in the hopes that things can be further improved in future PRs.
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -0,0 +1,311 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Druids;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.UnionDataSource;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Represents the native engine's execution vertex.
+ *
+ * Most likely due to evolutional purposes there are some concepts which went
+ * beyond their inital design:
+ *
+ * Multiple queries might be executed in one stage: <br/>
+ * The GroupBy query could be collapsed at exection time).
+ *
+ * Dag of datasources: <br/>
+ * an execution may process an entire dag of datasource in some cases
+ * (joindatasource) ; or collapse some into the execution (filter)
+ */
Review Comment:
The javadoc could use some improvement, such as:
- Define "execution vertex" more clearly. It can be difficult to find the right words. Examples are probably the best way to clearly explain it.
- Please don't discuss history of how we got here ("evolution") in the javadoc. That should be for git history and PR descriptions. Readers of the javadoc should be focused on the current state only.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2022864932
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -90,33 +93,22 @@ public interface DataSource
boolean isGlobal();
/**
- * Returns true if this datasource can be the base datasource of query processing.
- * <p>
- * Base datasources drive query processing. If the base datasource is {@link TableDataSource}, for example, queries
- * are processed in parallel on data servers. If the base datasource is {@link InlineDataSource}, queries are
- * processed on the Broker. See {@link DataSourceAnalysis#getBaseDataSource()} for further discussion.
+ * Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Datasources that are *not* concrete must be pre-processed in some way before they can be processed by the main
- * query stack. For example, {@link QueryDataSource} must be executed first and substituted with its results.
- *
- * @see DataSourceAnalysis#isConcreteBased() which uses this
- * @see DataSourceAnalysis#isConcreteAndTableBased() which uses this
+ * Processable examples are: Tables / Inline datasources.
Review Comment:
This really helped!
Thank you for condensing all those together.... I never feeled like this will be the final apidoc.
The connection to `segmentMapFunction` is very important - and makes it much more clear what this method will answer!
Shouldn't the method be renamed? I've no good candidates:
* `isProcessable`
* `isSegmentMappable`
* `isDirectlyReadable`
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #17763:
URL: https://github.com/apache/druid/pull/17763
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2022775416
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -548,13 +506,15 @@ private static DataSourceAnalysis constructAnalysis(final JoinDataSource dataSou
@Nullable
private static DimFilter validateLeftFilter(final DataSource leftDataSource, @Nullable final DimFilter leftFilter)
{
+ if (leftFilter == null || TrueDimFilter.instance().equals(leftFilter)) {
+ return null;
+ }
// Currently we only support leftFilter when applied to concrete leaf datasources (ones with no children).
// Note that this mean we don't support unions of table, even though this would be reasonable to add in the future.
Preconditions.checkArgument(
- leftFilter == null || (leftDataSource.isConcrete() && leftDataSource.getChildren().isEmpty()),
+ leftDataSource.isProcessable() && leftDataSource.getChildren().isEmpty(),
Review Comment:
From what I know the `leftFilter` pulls out the `filter` which was pushable below the join to be executed *after* the join will be processed...although that might not hurt in some cases - but it could increase the amount of work the join does.
I believe the right fix would be to utilize a `ScanDatasource` ; but in that case there won't be such questions anymore...
since I was backtracking to this problem from a [branch](https://github.com/kgyrtkirk/druid/pull/613) I was exploring to introduce that - I feel like that might be the easier long-term solution
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2023711585
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -98,13 +97,16 @@ public boolean isCacheable(boolean isBroker)
@Override
public boolean isGlobal()
{
+ // FIXME: this was query.getDataSource().isGlobal()
Review Comment:
Hmm, if the only thing that fails is an analysis test, then it might be OK to change it. I would be more worried if a query test failed. We don't need to change it in this PR, but I would consider changing it soon.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2023686545
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -548,13 +506,15 @@ private static DataSourceAnalysis constructAnalysis(final JoinDataSource dataSou
@Nullable
private static DimFilter validateLeftFilter(final DataSource leftDataSource, @Nullable final DimFilter leftFilter)
{
+ if (leftFilter == null || TrueDimFilter.instance().equals(leftFilter)) {
+ return null;
+ }
// Currently we only support leftFilter when applied to concrete leaf datasources (ones with no children).
// Note that this mean we don't support unions of table, even though this would be reasonable to add in the future.
Preconditions.checkArgument(
- leftFilter == null || (leftDataSource.isConcrete() && leftDataSource.getChildren().isEmpty()),
+ leftDataSource.isProcessable() && leftDataSource.getChildren().isEmpty(),
Review Comment:
IIRC, the `leftFilter` is pushed down to the underlying table. If the left-hand child was a scan datasource with a builtin filter, that would work just as well as an alternative to `leftFilter`.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2027794769
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -79,29 +86,22 @@ public interface DataSource extends Cacheable
* Decides if this datasource can be accessed globally.
*
* This means that all servers have a full copy of this datasource.
- * True for things like inline, lookup, etc, or for queries of those.
- * <p>
- * Currently this is coupled with joinability - if this returns true then the query engine expects there exists a
- * {@link org.apache.druid.segment.join.JoinableFactory} which might build a
- * {@link org.apache.druid.segment.join.Joinable} for this datasource directly. If a subquery 'inline' join is
- * required to join this datasource on the right hand side, then this value must be false for now.
- * <p>
- * In the future, instead of directly using this method, the query planner and engine should consider
- * {@link org.apache.druid.segment.join.JoinableFactory#isDirectlyJoinable(DataSource)} when determining if the
- * right hand side is directly joinable, which would allow decoupling this property from joins.
+ *
+ * Examples: inline table, lookup.
*/
boolean isGlobal();
/**
* Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Processable examples are: Tables / Inline datasources.
+ * A Processable datasource could pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
+ *
+ * Processable examples are: {@link TableDataSource}, {@link InlineDataSource}, {@link FilteredDataSource}.
* Non-processable ones are those which need further pre-processing before running them.
+ * examples are: {@link QueryDataSource} and join which are not supported directly.
* </p>
- * Processing boundaries are identified {@link ExecutionVertex}.
+ * Processing boundaries could be identified with {@link ExecutionVertex}.
Review Comment:
This sentence doesn't compute for me: what does "Processing boundaries could be identified with ExecutionVertex" mean? Perhaps a reword or some examples would help.
##########
processing/src/main/java/org/apache/druid/query/UnionDataSource.java:
##########
@@ -143,10 +144,12 @@ public boolean isProcessable()
@Override
public Function<SegmentReference, SegmentReference> createSegmentMapFunction(Query query)
{
- return Function.identity();
+ if (Iterables.all(dataSources, LeafDataSource.class::isInstance)) {
Review Comment:
It seems error-prone for an instanceof check to be used in this way. IMO, this should check `getChildren().isEmpty()` rather than checking if the datasource is an instance of `LeafDataSource`. That way, inheriting from `LeafDataSource` is more of a convenience for implementations of `DataSource`, not something that is actually meaningful to users of the `DataSource` object.
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -33,30 +33,61 @@
import org.apache.druid.query.scan.ScanQuery;
import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.SegmentReference;
+import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.join.JoinPrefixUtils;
import java.util.ArrayList;
import java.util.List;
+import java.util.function.Function;
/**
- * Represents the native engine's execution vertex.
+ * Represents the native engine's execution vertex - the execution unit it may execute in one go.
Review Comment:
we should define what "one go" means here. I think it means something like: one call of either `CachingClusteredClient` or `LocalQuerySegmentWalker`. (It definitely doesn't mean one run of `ClientQuerySegmentWalker`, because that could run _anything_ in one call.)
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -97,9 +98,6 @@ public boolean isCacheable(boolean isBroker)
@Override
public boolean isGlobal()
{
- // FIXME: this was query.getDataSource().isGlobal()
- // but it feeled dodgy to me
- // in general: I think isProcessable should be a precondition to isGblobal
return query.getDataSource().isGlobal();
Review Comment:
This (and the `isProcessable()` impl as well) does still feel sketchy, but we can leave it as-is for now. Could you please include a comment about that, though? Maybe something like:
```
// It would be more accurate if this method returned false, but returning false
// currently causes XYZ test case to fail.
```
(& mention the test case)
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -79,29 +86,22 @@ public interface DataSource extends Cacheable
* Decides if this datasource can be accessed globally.
*
* This means that all servers have a full copy of this datasource.
- * True for things like inline, lookup, etc, or for queries of those.
- * <p>
- * Currently this is coupled with joinability - if this returns true then the query engine expects there exists a
- * {@link org.apache.druid.segment.join.JoinableFactory} which might build a
- * {@link org.apache.druid.segment.join.Joinable} for this datasource directly. If a subquery 'inline' join is
- * required to join this datasource on the right hand side, then this value must be false for now.
- * <p>
- * In the future, instead of directly using this method, the query planner and engine should consider
- * {@link org.apache.druid.segment.join.JoinableFactory#isDirectlyJoinable(DataSource)} when determining if the
- * right hand side is directly joinable, which would allow decoupling this property from joins.
+ *
+ * Examples: inline table, lookup.
*/
boolean isGlobal();
/**
* Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Processable examples are: Tables / Inline datasources.
+ * A Processable datasource could pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
Review Comment:
Small thing, but I wanted to say that IMO the word choice is a little awkward here. "could" makes it sound to me like it's optional. But I think it's required. Clearer wording, IMO, would be something like:
- "Processable datasources pack the necessary logic"
- "Processable datasources must pack the necessary logic"
- "Processable datasources are able to pack the necessary logic"
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertex.java:
##########
@@ -33,30 +33,61 @@
import org.apache.druid.query.scan.ScanQuery;
import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.SegmentReference;
+import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.join.JoinPrefixUtils;
import java.util.ArrayList;
import java.util.List;
+import java.util.function.Function;
/**
- * Represents the native engine's execution vertex.
+ * Represents the native engine's execution vertex - the execution unit it may execute in one go.
*
- * Most likely due to evolutional purposes there are some concepts which went
- * beyond their inital design:
+ * This minimal Vertex a single {@link Query} with an input {@link DataSource}.
*
- * Multiple queries might be executed in one stage: <br/>
- * The GroupBy query could be collapsed at exection time).
+ * However there might be more complicated cases.
*
- * Dag of datasources: <br/>
- * an execution may process an entire dag of datasource in some cases
- * (joindatasource) ; or collapse some into the execution (filter)
+ * Multiple queries could be executed in one stage: <br/>
+ * the GroupBy query could be collapsed at execution time.
+ *
+ * For example: <br/>
+ * SELECT COUNT(*) FROM (SELECT string_first_added FROM druid.wikipedia_first_last GROUP BY 1)
+ *
+ * Will have 2 groupby queries; but they will be executed together.
Review Comment:
link here to `QueryToolChest#canPerformSubquery`, it would be helpful for readers to see that this is the place that determines what can be executed together.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2024778611
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -548,13 +506,15 @@ private static DataSourceAnalysis constructAnalysis(final JoinDataSource dataSou
@Nullable
private static DimFilter validateLeftFilter(final DataSource leftDataSource, @Nullable final DimFilter leftFilter)
{
+ if (leftFilter == null || TrueDimFilter.instance().equals(leftFilter)) {
+ return null;
+ }
// Currently we only support leftFilter when applied to concrete leaf datasources (ones with no children).
// Note that this mean we don't support unions of table, even though this would be reasonable to add in the future.
Preconditions.checkArgument(
- leftFilter == null || (leftDataSource.isConcrete() && leftDataSource.getChildren().isEmpty()),
+ leftDataSource.isProcessable() && leftDataSource.getChildren().isEmpty(),
Review Comment:
I was taking a closer look and I was wrong; When I wrote the above comment I was thinking that it works like this:
* `leftFilter` is recognized when there is a filter beneath the left side of the join
* `JoinDataSource` is storing it in the `leftFilter` field
* `DruidQuery#getFiltration` pulls it out and adds it to `Filtration` object
* `DruidQuery#toScanQuery` and similar `filtration.getDimFilter` is used to put it into the query's filter
* `CachingClusteredClient` reacts to `query#getFilter` so it could take also the `leftFilter` into consideration
* I think a re-check of the `leftFilter` condition in the `Query` remains
* `CursorFactory#makeCursorHolder` also communicates it back down when the cursor is created
...but taking a closer look at `DruidQuery#getFiltration`: it doesn't let the full `DimFilter` out to the query - only the `interval` ...so I guess `CachingClusteredClient` may not take advantage of the `leftFilter` in all cases.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2031111949
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -79,29 +86,22 @@ public interface DataSource extends Cacheable
* Decides if this datasource can be accessed globally.
*
* This means that all servers have a full copy of this datasource.
- * True for things like inline, lookup, etc, or for queries of those.
- * <p>
- * Currently this is coupled with joinability - if this returns true then the query engine expects there exists a
- * {@link org.apache.druid.segment.join.JoinableFactory} which might build a
- * {@link org.apache.druid.segment.join.Joinable} for this datasource directly. If a subquery 'inline' join is
- * required to join this datasource on the right hand side, then this value must be false for now.
- * <p>
- * In the future, instead of directly using this method, the query planner and engine should consider
- * {@link org.apache.druid.segment.join.JoinableFactory#isDirectlyJoinable(DataSource)} when determining if the
- * right hand side is directly joinable, which would allow decoupling this property from joins.
+ *
+ * Examples: inline table, lookup.
*/
boolean isGlobal();
/**
* Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Processable examples are: Tables / Inline datasources.
+ * A Processable datasource could pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
Review Comment:
changed to:
```
A Processable datasource must pack the necessary logic into the {@link DataSource#createSegmentMapFunction(Query)}.
```
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2017447398
##########
processing/src/main/java/org/apache/druid/query/JoinDataSource.java:
##########
@@ -548,13 +506,15 @@ private static DataSourceAnalysis constructAnalysis(final JoinDataSource dataSou
@Nullable
private static DimFilter validateLeftFilter(final DataSource leftDataSource, @Nullable final DimFilter leftFilter)
{
+ if (leftFilter == null || TrueDimFilter.instance().equals(leftFilter)) {
+ return null;
+ }
// Currently we only support leftFilter when applied to concrete leaf datasources (ones with no children).
// Note that this mean we don't support unions of table, even though this would be reasonable to add in the future.
Preconditions.checkArgument(
- leftFilter == null || (leftDataSource.isConcrete() && leftDataSource.getChildren().isEmpty()),
+ leftDataSource.isProcessable() && leftDataSource.getChildren().isEmpty(),
Review Comment:
The comments and error message don't seem right here. For example, a FilteredDataSource on top of a TableDataSource would return `true` for `isProcessable`, but that wouldn't be "one with no children" nor would it be "direct table access".
This problem predates the PR, because even in the prior code, `isConcrete` would return `true`.
Is this problem something you can fix in this PR, though, or possibly fix in a follow-up?
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2023005736
##########
processing/src/main/java/org/apache/druid/query/QueryDataSource.java:
##########
@@ -98,13 +97,16 @@ public boolean isCacheable(boolean isBroker)
@Override
public boolean isGlobal()
{
+ // FIXME: this was query.getDataSource().isGlobal()
Review Comment:
yes - this was pretty surprising to me....this have a testcase which is checking if joins on top of a query on a lookup is concreate.
[testJoinSimpleLeftLeaning](https://github.com/apache/druid/blob/f042a3607946d7cd701d5e0eef87a853f72d670e/processing/src/test/java/org/apache/druid/query/planning/DataSourceAnalysisTest.java#L263-L282)
returning `false` here will make the old expectation change from:
```
Assert.assertTrue(analysis.isConcreteBased());
Assert.assertTrue(analysis.isTableBased());
Assert.assertTrue(analysis.isConcreteAndTableBased());
```
I feel like returning `false` is better in this case ; but I don't know if that will have any sideeffects or not...I've retained the original behavior as I didn't wanted to cause any regressions....
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2023692740
##########
processing/src/main/java/org/apache/druid/query/DataSource.java:
##########
@@ -90,33 +93,22 @@ public interface DataSource
boolean isGlobal();
/**
- * Returns true if this datasource can be the base datasource of query processing.
- * <p>
- * Base datasources drive query processing. If the base datasource is {@link TableDataSource}, for example, queries
- * are processed in parallel on data servers. If the base datasource is {@link InlineDataSource}, queries are
- * processed on the Broker. See {@link DataSourceAnalysis#getBaseDataSource()} for further discussion.
+ * Communicates that this {@link DataSource} can be directly used to run a {@link Query}.
* <p>
- * Datasources that are *not* concrete must be pre-processed in some way before they can be processed by the main
- * query stack. For example, {@link QueryDataSource} must be executed first and substituted with its results.
- *
- * @see DataSourceAnalysis#isConcreteBased() which uses this
- * @see DataSourceAnalysis#isConcreteAndTableBased() which uses this
+ * Processable examples are: Tables / Inline datasources.
Review Comment:
Naming is tough with this one. Actually I think `isProcessable` is the best of all the proposed names so far.
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2022816116
##########
processing/src/main/java/org/apache/druid/query/cache/CacheKeyBuilder.java:
##########
@@ -108,7 +107,6 @@ private static byte[] cacheableToByteArray(@Nullable Cacheable cacheable)
return EMPTY_BYTES;
} else {
final byte[] key = cacheable.getCacheKey();
- Preconditions.checkArgument(!Arrays.equals(key, EMPTY_BYTES), "cache key is equal to the empty key");
Review Comment:
I was bumping into this from the other direction - and it seemed like some `DataSources` thinked they have nothing to add to the cachekey ; looking at those right now make no-sense to me...
As a matter of fact they were returning the empty cachekey in the hope to invalidate the "cache" - however `null` should have been used to communicate that - which was introduced in this PR....
so....I've added back this check - and fixed cachekey return a few places to be `null` instead - I think this is the right direction
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r2024931645
##########
server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java:
##########
@@ -80,21 +81,28 @@ public LocalQuerySegmentWalker(
@Override
public <T> QueryRunner<T> getQueryRunnerForIntervals(final Query<T> query, final Iterable<Interval> intervals)
{
+ ExecutionVertex ev = ExecutionVertex.of(query);
final DataSource dataSourceFromQuery = query.getDataSource();
- final DataSourceAnalysis analysis = query.getDataSourceAnalysis();
- if (!analysis.isConcreteBased() || !dataSourceFromQuery.isGlobal()) {
+ if (!ev.canRunQueryUsingLocalWalker()) {
throw new IAE("Cannot query dataSource locally: %s", dataSourceFromQuery);
}
// wrap in ReferenceCountingSegment, these aren't currently managed by SegmentManager so reference tracking doesn't
// matter, but at least some or all will be in a future PR
final Iterable<ReferenceCountingSegment> segments =
- FunctionalIterable.create(segmentWrangler.getSegmentsForIntervals(analysis.getBaseDataSource(), intervals))
+ FunctionalIterable.create(segmentWrangler.getSegmentsForIntervals(ev.getBaseDataSource(), intervals))
.transform(ReferenceCountingSegment::wrapRootGenerationSegment);
final AtomicLong cpuAccumulator = new AtomicLong(0L);
+ if (dataSourceFromQuery instanceof QueryDataSource) {
+ // is this a possible bug? dataSourceFromQuery is QueryDataSource ; query is GBY with canPerformSubquery
+ // FIXME: let's not fix this
+ // throw DruidException.defensive("This might not work correctly");
+ }
+
+ // segmentMapFn will ignore all DS maps ass QDS will return identity
Review Comment:
yes; throwing an exception in that method is a great idea - I guess I should have done that earlier.
> strips stacks of query datasources that are handleable by the toolchest
I guess nothing unhandlable should arrive here.... but iirc there were something failing..
I have a feeling that this will mandate the other change I'm hesitating to make with `QueryDataSource#isGlobal` :)
I don't remember which test was affected by this - so I'll do a CI run :)
--
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: commits-unsubscribe@druid.apache.org
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
Re: [PR] Introduce ExecutionVertex (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17763:
URL: https://github.com/apache/druid/pull/17763#discussion_r1977062855
##########
processing/src/main/java/org/apache/druid/query/planning/ExecutionVertexShuttle.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.planning;
+
+import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Stack;
+
+/**
+ * Visitor for traversing the execution vertices.
+ */
+abstract class ExecutionVertexShuttle
+{
+ /**
+ * Execution vertex node.
+ *
+ * Can be a {@link Query} or a {@link DataSource}.
+ */
+ static class EVNode
+ {
+ final DataSource dataSource;
+ final Query<?> query;
+ final Integer index;
+
+ EVNode(DataSource dataSource, Integer index)
+ {
+ this.dataSource = dataSource;
+ this.query = null;
+ this.index = index;
+ }
+
+ EVNode(Query<?> query, Integer index)
+ {
+ this.dataSource = null;
+ this.query = query;
+ this.index = index;
+ }
+
+ public boolean isQuery()
+ {
+ return query != null;
+ }
+
+ public Query<?> getQuery()
+ {
+ Preconditions.checkNotNull(query, "query is null!");
+ return query;
+ }
+ }
+ protected Stack<ExecutionVertexShuttle.EVNode> parents = new Stack<>();
+
+ protected final Query<?> traverse(Query<?> query)
+ {
+ try {
+ parents.push(new EVNode(query, null));
+ if (!mayTraverseQuery(query)) {
+ return query;
+ }
+ if (query instanceof BaseQuery<?>) {
+ BaseQuery<?> baseQuery = (BaseQuery<?>) query;
+ DataSource oldDataSource = baseQuery.getDataSource();
+ DataSource newDataSource = traverse(oldDataSource, null);
+ if (oldDataSource != newDataSource) {
+ query = baseQuery.withDataSource(newDataSource);
+ }
+ } else {
+ throw DruidException.defensive("Can't traverse a query[%s]!", query);
+ }
+ return visitQuery(query);
+ }
+ finally {
+ parents.pop();
+ }
+ }
+
+ protected final DataSource traverse(DataSource dataSource, Integer index)
+ {
+ try {
+ parents.push(new EVNode(dataSource, index));
+ boolean traverse = mayTraverseDataSource(parents.peek());
+ if (dataSource instanceof QueryDataSource) {
+ QueryDataSource queryDataSource = (QueryDataSource) dataSource;
+ if (traverse) {
+ Query<?> oldQuery = queryDataSource.getQuery();
+ Query<?> newQuery = traverse(oldQuery);
+ if (oldQuery != newQuery) {
+ dataSource = new QueryDataSource(newQuery);
+ }
+ }
+ return visit(dataSource, !traverse);
+ } else {
+ List<DataSource> children = dataSource.getChildren();
+ List<DataSource> newChildren = new ArrayList<>();
+ boolean changed = false;
+ if (traverse) {
+ for (int i = 0; i < children.size(); i++) {
+ DataSource oldDS = children.get(i);
+ DataSource newDS = traverse(oldDS, i);
+ newChildren.add(newDS);
+ changed |= (oldDS != newDS);
+ }
+ }
+ DataSource newDataSource = changed ? dataSource.withChildren(newChildren) : dataSource;
+ return visit(newDataSource, !traverse);
+
+ }
+ }
+ finally {
+ parents.pop();
+ }
+ }
+
+ protected abstract boolean mayTraverseQuery(Query<?> query);
Review Comment:
## Useless parameter
The parameter 'query' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/8704)
##########
processing/src/main/java/org/apache/druid/query/planning/JoinDataSourceAnalysis.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.planning;
+
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.filter.DimFilters;
+import org.apache.druid.query.filter.TrueDimFilter;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import javax.annotation.Nullable;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Analysis of a datasource for purposes of deciding how to execute a particular query.
+ *
+ * The analysis breaks a datasource down in the following way:
+ *
+ * <pre>
+ *
+ * Q <-- Possible query datasource(s) [may be none, or multiple stacked]
+ * |
+ * Q <-- Base query datasource, returned by {@link #getBaseQuery()} if it exists
+ * |
+ * J <-- Possible join tree, expected to be left-leaning
+ * / \
+ * J Dj <-- Other leaf datasources
+ * Base datasource / \ which will be joined
+ * (bottom-leftmost) --> Db Dj <---- into the base datasource
+ *
+ * </pre>
+ *
+ * The base datasource (Db) is returned by {@link #getBaseDataSource()}. The other leaf datasources are returned by
+ * {@link #getPreJoinableClauses()}.
+ *
+ * The base datasource (Db) will never be a join, but it can be any other type of datasource (table, query, etc).
+ * Note that join trees are only flattened if they occur at the top of the overall tree (or underneath an outer query),
+ * and that join trees are only flattened to the degree that they are left-leaning. Due to these facts, it is possible
+ * for the base or leaf datasources to include additional joins.
+ *
+ * The base datasource is the one that will be considered by the core Druid query stack for scanning via
+ * {@link org.apache.druid.segment.Segment} and {@link org.apache.druid.segment.CursorFactory}. The other leaf
+ * datasources must be joinable onto the base data.
+ *
+ * The idea here is to keep things simple and dumb. So we focus only on identifying left-leaning join trees, which map
+ * neatly onto a series of hash table lookups at query time. The user/system generating the queries, e.g. the druid-sql
+ * layer (or the end user in the case of native queries), is responsible for containing the smarts to structure the
+ * tree in a way that will lead to optimal execution.
+ */
+public class JoinDataSourceAnalysis
+{
+ private final DataSource baseDataSource;
+ @Nullable
+ private final DimFilter joinBaseTableFilter;
+ private final List<PreJoinableClause> preJoinableClauses;
+
+ public JoinDataSourceAnalysis(
+ DataSource baseDataSource,
+ @Nullable Query<?> baseQuery,
+ @Nullable DimFilter joinBaseTableFilter,
+ List<PreJoinableClause> preJoinableClauses,
+ @Nullable
+ QuerySegmentSpec querySegmentSpec
Review Comment:
## Useless parameter
The parameter 'querySegmentSpec' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/8706)
##########
processing/src/main/java/org/apache/druid/query/planning/JoinDataSourceAnalysis.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.planning;
+
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.filter.DimFilters;
+import org.apache.druid.query.filter.TrueDimFilter;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.join.JoinPrefixUtils;
+
+import javax.annotation.Nullable;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Analysis of a datasource for purposes of deciding how to execute a particular query.
+ *
+ * The analysis breaks a datasource down in the following way:
+ *
+ * <pre>
+ *
+ * Q <-- Possible query datasource(s) [may be none, or multiple stacked]
+ * |
+ * Q <-- Base query datasource, returned by {@link #getBaseQuery()} if it exists
+ * |
+ * J <-- Possible join tree, expected to be left-leaning
+ * / \
+ * J Dj <-- Other leaf datasources
+ * Base datasource / \ which will be joined
+ * (bottom-leftmost) --> Db Dj <---- into the base datasource
+ *
+ * </pre>
+ *
+ * The base datasource (Db) is returned by {@link #getBaseDataSource()}. The other leaf datasources are returned by
+ * {@link #getPreJoinableClauses()}.
+ *
+ * The base datasource (Db) will never be a join, but it can be any other type of datasource (table, query, etc).
+ * Note that join trees are only flattened if they occur at the top of the overall tree (or underneath an outer query),
+ * and that join trees are only flattened to the degree that they are left-leaning. Due to these facts, it is possible
+ * for the base or leaf datasources to include additional joins.
+ *
+ * The base datasource is the one that will be considered by the core Druid query stack for scanning via
+ * {@link org.apache.druid.segment.Segment} and {@link org.apache.druid.segment.CursorFactory}. The other leaf
+ * datasources must be joinable onto the base data.
+ *
+ * The idea here is to keep things simple and dumb. So we focus only on identifying left-leaning join trees, which map
+ * neatly onto a series of hash table lookups at query time. The user/system generating the queries, e.g. the druid-sql
+ * layer (or the end user in the case of native queries), is responsible for containing the smarts to structure the
+ * tree in a way that will lead to optimal execution.
+ */
+public class JoinDataSourceAnalysis
+{
+ private final DataSource baseDataSource;
+ @Nullable
+ private final DimFilter joinBaseTableFilter;
+ private final List<PreJoinableClause> preJoinableClauses;
+
+ public JoinDataSourceAnalysis(
+ DataSource baseDataSource,
+ @Nullable Query<?> baseQuery,
Review Comment:
## Useless parameter
The parameter 'baseQuery' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/8705)
--
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: commits-unsubscribe@druid.apache.org
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