paul-rogers commented on code in PR #13625:
URL: https://github.com/apache/druid/pull/13625#discussion_r1070698729
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java:
##########
@@ -637,7 +637,7 @@ public IngestTester testIngestQuery()
return new IngestTester();
}
- private ObjectMapper setupObjectMapper(Injector injector)
+ public static ObjectMapper setupObjectMapper(Injector injector)
Review Comment:
This method is deprecated. (I should add a `@Deprecated` annotation.). The
preferred solution is to register the module that contains the item here, or
create a "stub" module with the item needed, and use the methods to register
Druid modules.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryTestMSQ.java:
##########
@@ -0,0 +1,741 @@
+/*
+ * 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.test;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import org.apache.druid.guice.DruidInjectorBuilder;
+import org.apache.druid.msq.exec.WorkerMemoryParameters;
+import org.apache.druid.msq.sql.MSQTaskSqlEngine;
+import org.apache.druid.query.groupby.TestGroupByBuffers;
+import org.apache.druid.server.QueryLifecycleFactory;
+import org.apache.druid.sql.calcite.CalciteQueryTest;
+import org.apache.druid.sql.calcite.QueryTestBuilder;
+import org.apache.druid.sql.calcite.run.SqlEngine;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+
+/**
+ * Runs {@link CalciteQueryTest} but with MSQ engine
+ */
+public class CalciteSelectQueryTestMSQ extends CalciteQueryTest
+{
+
+ private MSQTestOverlordServiceClient indexingServiceClient;
+ private TestGroupByBuffers groupByBuffers;
+
+ @Before
+ public void setup2()
+ {
+ groupByBuffers = TestGroupByBuffers.createDefault();
+ }
+
+ @After
+ public void teardown2()
+ {
+ groupByBuffers.close();
+ }
+
+ @Override
+ public void configureGuice(DruidInjectorBuilder builder)
+ {
+ super.configureGuice(builder);
+ builder.addModules(CalciteMSQTestsHelper.fetchModules(temporaryFolder,
groupByBuffers).toArray(new Module[0]));
+ }
+
+
+ @Override
+ public SqlEngine createEngine(
+ QueryLifecycleFactory qlf,
+ ObjectMapper queryJsonMapper,
+ Injector injector
+ )
+ {
+ final WorkerMemoryParameters workerMemoryParameters =
+ WorkerMemoryParameters.createInstance(
+ WorkerMemoryParameters.PROCESSING_MINIMUM_BYTES * 50,
+ WorkerMemoryParameters.PROCESSING_MINIMUM_BYTES * 50,
+ 2,
+ 10,
+ 2
+ );
+ indexingServiceClient = new MSQTestOverlordServiceClient(
+ queryJsonMapper,
+ injector,
+ new MSQTestTaskActionClient(queryJsonMapper),
+ workerMemoryParameters
+ );
+ return new MSQTaskSqlEngine(indexingServiceClient, queryJsonMapper);
+ }
+
+ @Override
+ protected QueryTestBuilder testBuilder()
+ {
+ return new QueryTestBuilder(new CalciteTestConfig())
+ .addCustomVerification(new VerifyMSQSupportedNativeQueriesFactory())
+ .addCustomRunner(new ExtractResultsFactory(() ->
(MSQTestOverlordServiceClient) ((MSQTaskSqlEngine)
queryFramework().engine()).overlordClient()))
+ .skipVectorize(true)
+ .verifyNativeQueries(false);
+ }
+
+ @Ignore("Union datasource not supported by MSQ")
+ @Override
+ public void testUnionAllSameTableTwiceWithSameMapping()
+ {
+
+ }
Review Comment:
This approach seems clunky. There are hundreds of test, each of which will
need a stub here. Someone will modify the Calcite tests, not know about this,
and will then encounter test failures on the build during MSQ tests that they
can't easily understand.
I wonder, can we do it this way? For a test that _does not_ support MSQ:
```java
@Test
public void someClassicOnlyCalciteTest()
{
// Actual test here
}
```
And, for tests that are validated to work with MSQ:
```java
@Test
public void someMSQCompatibleCalciteTest()
{
msqCompatible();
// Actual test here
}
```
The `msqCompatible` function is defined as:
```java
class BaseCalciteQueryTest
{
// ...
private boolean msqCompatibleFlag;
protected void msqCompabible()
{
msqCompatibleFlag = true;
}
```
Finally, the info is passed to the test builder:
```java
protected QueryTestBuilder testBuilder()
{
return new QueryTestBuilder(new CalciteTestConfig())
.cannotVectorize(cannotVectorize)
.skipVectorize(skipVectorize)
.msqCompatible(msqCompatibleFlag); // New line
}
```
Now, the test runner can check the per-test MSQ compatible flag (in the test
builder) and skip the test if the framework is running MSQ and the test is not
compatible. (To be clear: there are two flags: the one shown above that avoids
the need to pass another argument to `testQuery()`, and a second one, hinted at
above, in the test builder.)
With this model, all Calcite tests can be run on MSQ, and will do nothing by
default. We only need an entry (that one method call) for tests that _can_ run
under MSQ.
With this, adding a new test is safe: MSQ will ignore it by default. The MSQ
team can occasionally look for new test cases, check if they can run under MSQ,
and if so, add the needed marker.
Note that the same could be done with an annotation, but that would require
a custom test runner. The above is a crude-but-effective solution that avoids
the need for any fancy stuff.
--
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]
