gianm commented on code in PR #17895:
URL: https://github.com/apache/druid/pull/17895#discussion_r2068337067
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/QueryDefinition.java:
##########
@@ -150,13 +173,15 @@ public boolean equals(Object o)
return false;
}
QueryDefinition that = (QueryDefinition) o;
- return Objects.equals(stageDefinitions, that.stageDefinitions) &&
Objects.equals(finalStage, that.finalStage);
+ return Objects.equals(stageDefinitions, that.stageDefinitions) &&
+ Objects.equals(finalStage, that.finalStage) &&
+ Objects.equals(context, that.context);
}
@Override
public int hashCode()
{
- return Objects.hash(stageDefinitions, finalStage);
+ return Objects.hash(stageDefinitions, finalStage, context);
}
@Override
Review Comment:
add `context` to `toString`
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/LegacyMSQSpec.java:
##########
@@ -0,0 +1,208 @@
+/*
+ * 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.msq.indexing;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.msq.indexing.destination.MSQDestination;
+import org.apache.druid.msq.indexing.destination.TaskReportMSQDestination;
+import org.apache.druid.msq.kernel.QueryDefinition;
+import org.apache.druid.msq.kernel.WorkerAssignmentStrategy;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryContext;
+import org.apache.druid.sql.calcite.planner.ColumnMappings;
+
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * Old MSQSpec with a native query in it.
+ *
+ * Usage of this class should be avoided in favor of {@link MSQSpec}.
+ */
+public class LegacyMSQSpec extends MSQSpec
Review Comment:
I wonder what the usefulness is of this and `MSQSpec` being different
classes. It seems to me that this class is all we need - it already has both
`Query` (directly) and `QueryDefinition` (through inheritance), and already
requires that only one of them be set.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQQueryKitSpecFactory.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.msq.sql;
+
+import com.google.inject.Inject;
+import org.apache.druid.msq.exec.QueryKitSpecFactory;
+import org.apache.druid.msq.indexing.MSQTuningConfig;
+import org.apache.druid.msq.kernel.controller.ControllerQueryKernelConfig;
+import org.apache.druid.msq.querykit.QueryKit;
+import org.apache.druid.msq.querykit.QueryKitSpec;
+import org.apache.druid.msq.util.MultiStageQueryContext;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryContext;
+
+public class MSQQueryKitSpecFactory implements QueryKitSpecFactory
Review Comment:
This is task-specific so it should ideally be called either
`IndexerQueryKitSpecFactory` or `MSQTaskQueryKitSpecFactory`.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/DartQueryKitSpecFactory.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.msq.sql;
+
+import org.apache.druid.msq.dart.controller.DartControllerContext;
+import org.apache.druid.msq.exec.QueryKitSpecFactory;
+import org.apache.druid.msq.indexing.MSQTuningConfig;
+import org.apache.druid.msq.kernel.controller.ControllerQueryKernelConfig;
+import org.apache.druid.msq.querykit.QueryKit;
+import org.apache.druid.msq.querykit.QueryKitSpec;
+import org.apache.druid.msq.util.MultiStageQueryContext;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryContext;
+
+public class DartQueryKitSpecFactory implements QueryKitSpecFactory
+{
+ @Override
+ public QueryKitSpec makeQueryKitSpec(QueryKit<Query<?>> queryKit, String
queryId, MSQTuningConfig tuningConfig,
Review Comment:
please break down multi-line parameter lists into a single parameter per
line. The style checker might catch this too, it looks like it isn't passing
yet.
##########
processing/src/main/java/org/apache/druid/query/spec/QuerySegmentSpec.java:
##########
@@ -37,6 +38,8 @@
})
public interface QuerySegmentSpec
{
+ public static QuerySegmentSpec DEFAULT = new
MultipleIntervalSegmentSpec(Intervals.ONLY_ETERNITY);
Review Comment:
IMO, `ETERNITY` is a better name than `DEFAULT` because it's more obvious
what it means. Although, this doesn't appear to be used anyway - is it needed?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/sql/DartSqlEngine.java:
##########
@@ -70,6 +72,21 @@ public class DartSqlEngine implements SqlEngine
private final DartControllerConfig controllerConfig;
private final ExecutorService controllerExecutor;
+ @Inject
Review Comment:
Is this still a singleton after removing the `@LazySingleton`? If it's only
getting injected into a single object, it will still be fine; I just want to
make sure there is only a single `DartSqlEngine` ever. It's important because
it owns the executor that controllers run in.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -135,13 +142,27 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
final List<Pair<SqlTypeName, ColumnType>> typeList = getTypes(druidQuery,
fieldMapping, plannerContext);
+ ResultsContext resultsContext = new ResultsContext(
Review Comment:
please make this `final`; IMO it's confusing to mix `final` and nonfinal
declarations near each other; it suggests the nonfinal variable may need to be
reassigned. (although here, it doesn't seem like it will be reassigned.)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/QueryDefinition.java:
##########
@@ -49,18 +51,31 @@ public class QueryDefinition
{
private final Map<StageId, StageDefinition> stageDefinitions;
private final StageId finalStage;
+ @JsonInclude(JsonInclude.Include.NON_DEFAULT)
+ private final QueryContext context;
+
+ private QueryDefinition() {
+ this.stageDefinitions = null;
+ this.finalStage = null;
+ this.context = QueryContext.empty();
+ }
private QueryDefinition(
final Map<StageId, StageDefinition> stageDefinitions,
- final StageId finalStage
+ final StageId finalStage,
+ final QueryContext context
)
{
this.stageDefinitions = stageDefinitions;
this.finalStage = finalStage;
+ this.context = context;
}
@JsonCreator
- static QueryDefinition create(@JsonProperty("stages") final
List<StageDefinition> stageDefinitions)
+ static QueryDefinition create(
+ @JsonProperty("stages") final List<StageDefinition> stageDefinitions,
+ @JsonProperty("context") QueryContext context
Review Comment:
`final` on both please (it should be on both or neither; mixing `final` and
nonfinal is confusing unless the nonfinal variables actually need to be
reassigned).
`@Nullable` on `context` please. It helps to see at a glance what parameters
in the JSON are required and what aren't.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/QueryKitSpecFactory.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.msq.exec;
+
+import org.apache.druid.msq.indexing.MSQTuningConfig;
+import org.apache.druid.msq.kernel.controller.ControllerQueryKernelConfig;
+import org.apache.druid.msq.querykit.QueryKit;
+import org.apache.druid.msq.querykit.QueryKitSpec;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryContext;
+
+public interface QueryKitSpecFactory
+{
+ /**
+ * Create a {@link QueryKitSpec}. This method provides controller contexts a
way to customize parameters around the
Review Comment:
This comment is no longer accurate, since it isn't used by the context
anymore. Please update it, since in general the comment is a useful one, as it
explains why this function needs to exist and be configurable.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -236,17 +244,26 @@
// This flag is to ensure backward compatibility, as brokers are upgraded
after indexers/middlemanagers.
nativeQueryContextOverrides.put(MultiStageQueryContext.WINDOW_FUNCTION_OPERATOR_TRANSFORMATION,
true);
+ boolean isReindex =
MSQControllerTask.isReplaceInputDataSourceTask(druidQuery.getQuery(),
destination);
Review Comment:
This seems like a legitimate alert from CodeQL. Here we dereference
`druidQuery`; in a few lines there is a check `druidQuery == null ? null :
...`. Either the later check isn't needed, or the dereference here is possibly
going to throw NPE.
--
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]
