cryptoe commented on code in PR #12897: URL: https://github.com/apache/druid/pull/12897#discussion_r944663044
########## sql/src/main/java/org/apache/druid/sql/calcite/view/ViewSqlEngine.java: ########## @@ -0,0 +1,102 @@ +/* + * 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.view; + +import org.apache.calcite.rel.RelRoot; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.druid.java.util.common.IAE; +import org.apache.druid.sql.calcite.planner.PlannerContext; +import org.apache.druid.sql.calcite.run.EngineFeature; +import org.apache.druid.sql.calcite.run.QueryMaker; +import org.apache.druid.sql.calcite.run.SqlEngine; + +/** + * Engine used for getting the row type of views. Does not do any actual execution. + */ +public class ViewSqlEngine implements SqlEngine +{ + public static final ViewSqlEngine INSTANCE = new ViewSqlEngine(); + + private ViewSqlEngine() + { + // Singleton. + } + + @Override + public boolean feature(EngineFeature feature, PlannerContext plannerContext) + { + switch (feature) { + // Use most permissive set of features, since our goal is to get the row type of the view. + // Later on, the query involving the view will be executed with an actual engine with a different set of + // features, and planning may fail then. But we don't want it to fail now. + case ALLOW_BINDABLE_PLAN: + case CAN_READ_EXTERNAL_DATA: + case SCAN_CAN_ORDER_BY_NON_TIME: + return true; + + // Views can't sit on top of INSERTs. + case CAN_INSERT: + return false; + + // Simplify planning by sticking to basic query types. + case CAN_RUN_TOPN: + case CAN_RUN_TIMESERIES: + case CAN_RUN_TIME_BOUNDARY: + case SCAN_NEEDS_SIGNATURE: + return false; + + default: + throw new IAE("Unrecognized feature: %s", feature); + } + } + + @Override + public boolean isSystemContextParameter(String contextParameterName) + { + return false; + } + + @Override + public RelDataType resultTypeForSelect(RelDataTypeFactory typeFactory, RelDataType validatedRowType) + { + return validatedRowType; + } + + @Override + public RelDataType resultTypeForInsert(RelDataTypeFactory typeFactory, RelDataType validatedRowType) + { + // Can't have views of INSERT or REPLACE statements. + throw new UnsupportedOperationException(); Review Comment: Nit: I think we prefer UOE over java.lang.UnsupportedOperationException. Should we add a message also ? ########## 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: Nit: should we change this to CAN_INSERT_OR_REPLACE ? ########## 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.", + ExternalOperatorConversion.FUNCTION_NAME + ); + return false; } } @Override public void onMatch(final RelOptRuleCall call) { + if (!plannerContext.engineHasFeature(EngineFeature.CAN_READ_EXTERNAL_DATA)) { + // Not called because "matches" returns false. + throw new UnsupportedOperationException(); Review Comment: nit: UOE ########## 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)) { Review Comment: This looks much more readable!! ########## 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: Should this be parsed.getInsertOrReplace()!=null ? ########## 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: Should we add what is the current sql engine ? -- 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]
