[GitHub] [druid] rohangarg commented on a diff in pull request #13283: Enhancements to the Calcite test framework

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

rohangarg commented on code in PR #13283:
URL: https://github.com/apache/druid/pull/13283#discussion_r1008740754


##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -127,6 +142,14 @@ SqlEngine createEngine(
     void configureJsonMapper(ObjectMapper mapper);
 
     void configureGuice(DruidInjectorBuilder builder);
+
+    Set<ExtensionCalciteRuleProvider> calciteRules();

Review Comment:
   could be `extensionCalciteRules`



##########
sql/src/test/java/org/apache/druid/sql/calcite/QueryTestBuilder.java:
##########
@@ -200,13 +202,46 @@ public QueryTestBuilder authConfig(AuthConfig authConfig)
     return this;
   }
 
+  /**
+   * By default, every test case creates its own planner based on the planner
+   * and auth config provided. If, however, a test wants to control setup, and
+   * run multiple test queries against the same setup, use this method to pass
+   * in the pre-built planner to use. If not set, the standard one is created
+   * per test.
+   */
+  public QueryTestBuilder plannerFixture(PlannerFixture plannerFixture)
+  {
+    this.plannerFixture = plannerFixture;
+    return this;
+  }
+
   public QueryTestRunner build()
   {
     return config.analyze(this);
   }
 
+  /**
+   * Internal method to return the cached planner config, or create a new one
+   * based on the configs provided. Note: does not cache the newly created
+   * config: doing so would confuse the "please use mine" vs. "create a new
+   * one each time" semantics.
+   */
+  protected PlannerFixture plannerFixture()
+  {
+    if (plannerFixture != null) {
+      return plannerFixture;
+    } else {
+      return config.plannerFixture(plannerConfig, authConfig);
+    }
+  }
+
   public void run()

Review Comment:
   is there any reason to have the run and results methods in `runner` and `builder` both? also, should the builder be called `QueryTestRunnerBuilder`? 



##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -240,50 +348,107 @@ public SqlTestFramework build()
     }
   }
 
+  /**
+   * Builds the statement factory, which also builds all the infrastructure
+   * behind the factory by calling methods on this test class. As a result, each
+   * factory is specific to one test and one planner config. This method can be
+   * overridden to control the objects passed to the factory.
+   */
+  public static class PlannerFixture
+  {
+    private final ViewManager viewManager;
+    private final PlannerFactory plannerFactory;
+    private final SqlStatementFactory statementFactory;
+
+    public PlannerFixture(
+        final SqlTestFramework framework,
+        final PlannerConfig plannerConfig,
+        final AuthConfig authConfig
+    )
+    {
+      final QueryComponentSupplier componentSupplier = framework.componentSupplier;
+      this.viewManager = componentSupplier.createViewManager();
+      final DruidSchemaCatalog rootSchema = QueryFrameworkUtils.createMockRootSchema(
+          framework.injector,
+          framework.conglomerate(),
+          framework.walker(),
+          plannerConfig,
+          viewManager,
+          componentSupplier.createSchemaManager(),
+          framework.authorizerMapper
+      );
+
+      this.plannerFactory = new PlannerFactory(
+          rootSchema,
+          framework.operatorTable(),
+          framework.macroTable(),
+          plannerConfig,
+          framework.authorizerMapper,
+          framework.queryJsonMapper(),
+          CalciteTests.DRUID_SCHEMA_NAME,
+          new CalciteRulesManager(componentSupplier.calciteRules()),
+          CalciteTests.createJoinableFactoryWrapper()
+      );
+      this.statementFactory = QueryFrameworkUtils.createSqlStatementFactory(
+          framework.engine,
+          plannerFactory,
+          authConfig
+      );
+      componentSupplier.populateViews(viewManager, plannerFactory);
+    }
+
+    public ViewManager viewManager()

Review Comment:
   where would these get used? incase they are not used, can we call this `SqlStatementFactoryFixture`? 



##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -349,83 +514,12 @@ public QueryRunnerFactoryConglomerate conglomerate()
    * factory is specific to one test and one planner config. This method can be
    * overridden to control the objects passed to the factory.
    */
-  public SqlStatementFactory statementFactory(
+  public PlannerFixture plannerFixture(

Review Comment:
   the documentation seems a bit more related to the old method



##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -127,6 +142,14 @@ SqlEngine createEngine(
     void configureJsonMapper(ObjectMapper mapper);
 
     void configureGuice(DruidInjectorBuilder builder);
+
+    Set<ExtensionCalciteRuleProvider> calciteRules();
+
+    ViewManager createViewManager();
+
+    void populateViews(ViewManager viewManager, PlannerFactory plannerFactory);

Review Comment:
   should this be a part of the `SqlQueryFramework` instead? 



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