[GitHub] [druid] clintropolis commented on a diff in pull request #12965: Modular Calcite Test Framework

[GitBox <gi...@apache.org>]

clintropolis commented on code in PR #12965:
URL: https://github.com/apache/druid/pull/12965#discussion_r995163315


##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModule.java:
##########
@@ -35,6 +35,7 @@ public class CalcitePlannerModule implements Module
   public void configure(Binder binder)
   {
     JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class);
+    JsonConfigProvider.bind(binder, "druid.sql.planner", SegmentMetadataCacheConfig.class);

Review Comment:
   heh, this actually works binding them both to the same thing? I guess looking closer the call to bind will make a new json config provider to get the stuff from property base, so probably is ok, and the actual bind part should be fine, just not sure i've seen it before or at least haven't noticed 😅 
   
   This is probably worth a comment though



##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java:
##########
@@ -193,27 +176,27 @@ public boolean equals(final Object o)
            useApproximateCountDistinct == that.useApproximateCountDistinct &&
            useApproximateTopN == that.useApproximateTopN &&
            requireTimeCondition == that.requireTimeCondition &&
-           awaitInitializationOnStart == that.awaitInitializationOnStart &&
            metadataSegmentCacheEnable == that.metadataSegmentCacheEnable &&
            metadataSegmentPollPeriod == that.metadataSegmentPollPeriod &&

Review Comment:
   why are `metadataSegmentCacheEnable` and `metadataSegmentCachePollPeriod` not also moved to `SegmentMetadataCacheConfig`?



##########
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java:
##########
@@ -830,163 +760,93 @@ public void testQuery(
       @Nullable final Consumer<ExpectedException> expectedExceptionInitializer
   )
   {
-    log.info("SQL: %s", sql);
-
-    final List<String> vectorizeValues = new ArrayList<>();
-
-    vectorizeValues.add("false");
-
-    if (!skipVectorize) {
-      vectorizeValues.add("force");
-    }
-
-    for (final String vectorize : vectorizeValues) {
-      queryLogHook.clearRecordedQueries();
-
-      final Map<String, Object> theQueryContext = new HashMap<>(queryContext);
-      theQueryContext.put(QueryContexts.VECTORIZE_KEY, vectorize);
-      theQueryContext.put(QueryContexts.VECTORIZE_VIRTUAL_COLUMNS_KEY, vectorize);
-
-      if (!"false".equals(vectorize)) {
-        theQueryContext.put(QueryContexts.VECTOR_SIZE_KEY, 2); // Small vector size to ensure we use more than one.
-      }
-
-      final List<Query<?>> theQueries = new ArrayList<>();
-      for (Query<?> query : expectedQueries) {
-        theQueries.add(recursivelyOverrideContext(query, theQueryContext));
-      }
-
-      if (cannotVectorize && "force".equals(vectorize)) {
-        expectedException.expect(RuntimeException.class);
-        expectedException.expectMessage("Cannot vectorize");
-      } else if (expectedExceptionInitializer != null) {
-        expectedExceptionInitializer.accept(expectedException);
-      }
-
-      final Pair<RowSignature, List<Object[]>> plannerResults =
-          getResults(plannerConfig, theQueryContext, parameters, sql, authenticationResult);
-      verifyResults(sql, theQueries, plannerResults, expectedResultsVerifier);
-    }
+    testBuilder()
+        .plannerConfig(plannerConfig)
+        .queryContext(queryContext)
+        .parameters(parameters)
+        .sql(sql)
+        .authResult(authenticationResult)
+        .expectedQueries(expectedQueries)
+        .expectedResults(expectedResultsVerifier)
+        .expectedException(expectedExceptionInitializer)
+        .run();
   }
 
-  public Pair<RowSignature, List<Object[]>> getResults(
-      final PlannerConfig plannerConfig,
-      final Map<String, Object> queryContext,
-      final List<SqlParameter> parameters,
-      final String sql,
-      final AuthenticationResult authenticationResult
-  )
+  protected QueryTestBuilder testBuilder()
   {
-    return getResults(
-        plannerConfig,
-        queryContext,
-        parameters,
-        sql,
-        authenticationResult,
-        createOperatorTable(),
-        createMacroTable(),
-        CalciteTests.TEST_AUTHORIZER_MAPPER,
-        queryJsonMapper
-    );
+    return new QueryTestBuilder(new CalciteTestConfig())
+        .cannotVectorize(cannotVectorize)
+        .skipVectorize(skipVectorize);
   }
 
-  private Pair<RowSignature, List<Object[]>> getResults(
-      final PlannerConfig plannerConfig,
-      final Map<String, Object> queryContext,
-      final List<SqlParameter> parameters,
-      final String sql,
-      final AuthenticationResult authenticationResult,
-      final DruidOperatorTable operatorTable,
-      final ExprMacroTable macroTable,
-      final AuthorizerMapper authorizerMapper,
-      final ObjectMapper objectMapper
-  )
+  public class CalciteTestConfig implements QueryTestBuilder.QueryTestConfig
   {
-    final SqlStatementFactory sqlStatementFactory = getSqlStatementFactory(
-        plannerConfig,
-        new AuthConfig(),
-        operatorTable,
-        macroTable,
-        authorizerMapper,
-        objectMapper
-    );
-    final DirectStatement stmt = sqlStatementFactory.directStatement(
-        SqlQueryPlus.builder(sql)
-            .context(queryContext)
-            .sqlParameters(parameters)
-            .auth(authenticationResult)
-            .build()
-    );
-    Sequence<Object[]> results = stmt.execute().getResults();
-    RelDataType rowType = stmt.prepareResult().getReturnedRowType();
-    return new Pair<>(
-        RowSignatures.fromRelDataType(rowType.getFieldNames(), rowType),
-        results.toList()
-    );
-  }
+    @Override
+    public QueryTestRunner analyze(QueryTestBuilder builder)
+    {
+      if (builder.expectedResultsVerifier == null && builder.expectedResults != null) {
+        builder.expectedResultsVerifier = defaultResultsVerifier(
+            builder.expectedResults,
+            builder.expectedResultSignature
+        );
+      }
+      final List<QueryTestRunner.QueryRunStep> runSteps = new ArrayList<>();
+      final List<QueryTestRunner.QueryVerifyStep> verifySteps = new ArrayList<>();
+      if (builder.expectedResources != null) {
+        Preconditions.checkArgument(
+            builder.expectedResultsVerifier == null,
+            "Cannot check both results and resources"
+        );
+        QueryTestRunner.PrepareQuery execStep = new QueryTestRunner.PrepareQuery(builder);
+        runSteps.add(execStep);
+        verifySteps.add(new QueryTestRunner.VerifyResources(execStep));
+      } else {
+        QueryTestRunner.ExecuteQuery execStep = new QueryTestRunner.ExecuteQuery(builder);
+        runSteps.add(execStep);
+        if (builder.expectedResultsVerifier != null) {
+          verifySteps.add(new QueryTestRunner.VerifyResults(execStep));
+        }
+        if (builder.expectedQueries != null) {
+          verifySteps.add(new QueryTestRunner.VerifyNativeQueries(execStep));
+        }

Review Comment:
   while we are here, can we swap this to verify the native query before verifying the results? I know this is how it used to be, but it seems better to verify that the queries are correct before the results are correct :shrug:



##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java:
##########
@@ -97,7 +97,10 @@ public PlannerFactory(
   /**
    * Create a Druid query planner from an initial query context
    */
-  public DruidPlanner createPlanner(final SqlEngine engine, final String sql, final QueryContext queryContext)
+  public DruidPlanner createPlanner(
+      final SqlEngine engine,
+      final String sql,
+      final QueryContext queryContext)

Review Comment:
   nit: ')' should be on a new line, iirc this is a limitation of the style that it can't enforce this



##########
sql/src/test/java/org/apache/druid/sql/calcite/util/QueryFramework.java:
##########
@@ -0,0 +1,439 @@
+/*
+ * 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.sql.calcite.util;
+
+import com.fasterxml.jackson.databind.Module;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
+import com.google.inject.Binder;
+import com.google.inject.Injector;
+import org.apache.druid.guice.DruidInjectorBuilder;
+import org.apache.druid.initialization.DruidModule;
+import org.apache.druid.java.util.common.RE;
+import org.apache.druid.java.util.common.io.Closer;
+import org.apache.druid.math.expr.ExprMacroTable;
+import org.apache.druid.query.QueryRunnerFactoryConglomerate;
+import org.apache.druid.query.lookup.LookupSerdeModule;
+import org.apache.druid.query.topn.TopNQueryConfig;
+import org.apache.druid.server.QueryLifecycle;
+import org.apache.druid.server.QueryLifecycleFactory;
+import org.apache.druid.server.QueryStackTests;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthorizerMapper;
+import org.apache.druid.sql.SqlStatementFactory;
+import org.apache.druid.sql.calcite.external.ExternalDataSource;
+import org.apache.druid.sql.calcite.planner.CalciteRulesManager;
+import org.apache.druid.sql.calcite.planner.DruidOperatorTable;
+import org.apache.druid.sql.calcite.planner.PlannerConfig;
+import org.apache.druid.sql.calcite.planner.PlannerFactory;
+import org.apache.druid.sql.calcite.run.NativeSqlEngine;
+import org.apache.druid.sql.calcite.run.SqlEngine;
+import org.apache.druid.sql.calcite.schema.DruidSchemaCatalog;
+import org.apache.druid.sql.calcite.schema.NoopDruidSchemaManager;
+import org.apache.druid.sql.calcite.view.DruidViewMacroFactory;
+import org.apache.druid.sql.calcite.view.InProcessViewManager;
+import org.apache.druid.timeline.DataSegment;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Builds the infrastructure needed to run Calcite tests. Building splits into
+ * two parts: a constant part and a per-test part. The constant part includes
+ * the congolmerate and walker (that is, the implementation of the test data),
+ * while the per-query part includes the schema, planner factory and SQL
+ * statement factory. The key reason for the split is that the per-test part
+ * depends on the value of the {@link PlannerConfig} object, which varies between
+ * tests.
+ * <p>
+ * The builder accepts the injector to use. "Calcite tests" use the injector
+ * defined in {@link CalciteTests#INJECTOR}, while other tests can pass in the
+ * preferred injector.
+ * <p>
+ * The framework allows the test to customize many of the framework components.
+ * Since those components tend to depend on other framework components, we use
+ * an indirection, {@link QueryFramework.QueryComponentSupplier QueryComponentSupplier}
+ * to build those components. The methods of this interface match the methods
+ * in @link org.apache.druid.sql.calcite.BaseCalciteQueryTest BaseCalciteQueryTest}
+ * so that those tests can customize the framework just by overriding methods.
+ * (This was done to maintain backward compatibility with earlier versions of the
+ * code.) Other tests can implement {@code QueryComponentSupplier} directly by
+ * extending {@link QueryFramework.StandardComponentSupplier StandardComponentSupplier}.
+ * <p>
+ * The framework should be built once per test class (not once per test method.)
+ * Then, per method, call {@link #statementFactory(PlannerConfig, AuthConfig)} to
+ * obtain a the test-specific planner and wrapper classes for that test. After
+ * that, tests use the various SQL statement classes to run tests. For tests
+ * based on {@code BaseCalciteQueryTest}, the statements are wrapped by the
+ * various {@code testQuery()} methods.
+ * <p>
+ * The framework holds on to the framework components. You can obtain the injector,
+ * object mapper and other items by calling the various methods. The objects
+ * are those created by the provided injector, or in this class, using objects
+ * from that injector.
+ */
+public class QueryFramework

Review Comment:
   nit: should we call this `QueryTestFramework` or `SqlTestFramework` or something?



-- 
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