You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/10/29 01:00:26 UTC
[GitHub] [druid] paul-rogers opened a new pull request, #13283: Enhancements to the Calcite test framework
paul-rogers opened a new pull request, #13283:
URL: https://github.com/apache/druid/pull/13283
Follow-up to the earlier Calcite test framework PR. This version makes more of the components customizable, and provides additional options for the test builder. The need for these items, and verification of their functionality, comes from tests for a few private extensions.
This PR also standardizes various "Unauthorized" messages to avoid the persistent issues with security ITs where the expected message does not match the one Druid delivers, because Druid used multiple variations of the same message.
#### Release note
There are no user-visible changes in the PR. All changes relate to unit tests.
<hr>
This PR has:
- [X] been self-reviewed.
- [X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
--
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
[GitHub] [druid] rohangarg commented on a diff in pull request #13283: Enhancements to the Calcite test framework
Posted by 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
[GitHub] [druid] paul-rogers merged pull request #13283: Enhancements to the Calcite test framework
Posted by GitBox <gi...@apache.org>.
paul-rogers merged PR #13283:
URL: https://github.com/apache/druid/pull/13283
--
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