paul-rogers commented on code in PR #12897:
URL: https://github.com/apache/druid/pull/12897#discussion_r944797807
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -422,8 +451,19 @@ private PlannerResult planWithDruidConvention(
if (explain != null) {
return planExplanation(druidRel, explain, true);
} else {
- final Supplier<Sequence<Object[]>> resultsSupplier = () -> {
+ // Compute row type.
+ final RelDataTypeFactory typeFactory =
rootQueryRel.rel.getCluster().getTypeFactory();
+ final RelDataType rowType;
+ if (parsed.isSelect()) {
+ rowType = engine.resultTypeForSelect(typeFactory,
rootQueryRel.validatedRowType);
+ } else {
+ assert parsed.insertOrReplace != null;
+ rowType = engine.resultTypeForInsert(typeFactory,
rootQueryRel.validatedRowType);
+ }
Review Comment:
This is exactly the kind of silliness that the handler refactoring will
address.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -319,31 +345,34 @@ public PlannerResult plan() throws ValidationException
rootQueryRel = planner.rel(validatedQueryNode);
}
+ final boolean hasBindableTables = hasBindableTables(rootQueryRel.rel);
+
// the planner's type factory is not available until after parsing
this.rexBuilder = new RexBuilder(planner.getTypeFactory());
state = State.PLANNED;
try {
- return planWithDruidConvention(rootQueryRel, parsed.getExplainNode(),
parsed.getInsertOrReplace());
+ if (hasBindableTables) {
+ // Consider BINDABLE convention if necessary. Used for metadata tables.
+ if (!parsed.isSelect()) {
Review Comment:
All this will change in a PR that will be added after #12845 is in. We'll
refactor this code into "handlers" for each statement type.
##########
sql/src/main/java/org/apache/druid/sql/calcite/run/EngineFeature.java:
##########
@@ -20,12 +20,18 @@
package org.apache.druid.sql.calcite.run;
import org.apache.druid.sql.calcite.external.ExternalDataSource;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
/**
- * Arguments to {@link QueryFeatureInspector#feature(QueryFeature)}.
+ * Arguments to {@link EngineFeatureInspector#feature(EngineFeature,
PlannerContext)}.
*/
-public enum QueryFeature
+public enum EngineFeature
{
+ /**
+ * Can execute INSERT and REPLACE statements.
+ */
+ CAN_INSERT,
Review Comment:
If we do, perhaps make this more granular for each statement type:
CAN_SELECT, CAN_INSERT, CAN_REPLACE, CAN_UPSERT, CAN_DELETE, ...
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -165,9 +170,26 @@ public void validate() throws SqlParseException,
ValidationException
{
Preconditions.checkState(state == State.START);
resetPlanner();
+
+ // Validate SQL query context parameters.
+ for (String contextParameterName :
plannerContext.getQueryContext().getMergedParams().keySet()) {
+ if (engine.isSystemContextParameter(contextParameterName)) {
+ throw new ValidationException(
+ StringUtils.format(
+ "Cannot execute query with context parameter [%s]",
+ contextParameterName
+ )
+ );
+ }
+ }
Review Comment:
Suggestion, move this into the engine as `validateContext()` so that code
here does not need to now what rules each engine might enforce.
##########
sql/src/main/java/org/apache/druid/sql/SqlLifecycleFactory.java:
##########
@@ -57,9 +58,10 @@ public SqlLifecycleFactory(
this.defaultQueryConfig = defaultQueryConfig.get();
}
- public SqlLifecycle factorize()
+ public SqlLifecycle factorize(final SqlEngine engine)
Review Comment:
With this change, engine selection is up to the client. In the PR #12845
we'll need to pass this into the three factory method for the three kinds of
statements. There is code that chooses the factory. It could be that factory
selection affects other attributes.
So, I wonder, should there be a "factory selector" that can sniff the sql
info and make a choice? Or, should there be a planner factory per engine?
##########
sql/src/main/java/org/apache/druid/sql/calcite/run/EngineFeature.java:
##########
@@ -36,6 +42,11 @@
*/
CAN_RUN_TOPN,
+ /**
+ * Queries of type {@link
org.apache.druid.query.timeboundary.TimeBoundaryQuery} are usable.
+ */
+ CAN_RUN_TIME_BOUNDARY,
Review Comment:
Nit: Since these items are logically part of a feature set, maybe just use
the feature name itself. TOPN_QUERY, SELECT, TIME_BOUNDARY_QUERY, etc.
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalTableScanRule.java:
##########
@@ -43,17 +43,26 @@ public ExternalTableScanRule(final PlannerContext
plannerContext)
@Override
public boolean matches(RelOptRuleCall call)
{
- if
(plannerContext.getQueryMaker().feature(QueryFeature.CAN_READ_EXTERNAL_DATA)) {
+ if (plannerContext.engineHasFeature(EngineFeature.CAN_READ_EXTERNAL_DATA))
{
return super.matches(call);
} else {
- plannerContext.setPlanningError("SQL query requires scanning external
datasources that is not suported.");
+ plannerContext.setPlanningError(
+ "Cannot use '%s' with the current SQL engine.",
Review Comment:
Perhaps at least include the class name and we can improve it later. Else,
we'll wonder what the "current engine" is if we get this error.
--
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]
