gianm commented on code in PR #12636: URL: https://github.com/apache/druid/pull/12636#discussion_r898761962
########## sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java: ########## @@ -0,0 +1,432 @@ +/* + * 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.planner; + +import com.google.common.collect.ImmutableList; +import org.apache.calcite.adapter.java.JavaTypeFactory; +import org.apache.calcite.config.CalciteConnectionConfig; +import org.apache.calcite.config.CalciteConnectionConfigImpl; +import org.apache.calcite.config.CalciteConnectionProperty; +import org.apache.calcite.config.CalciteSystemProperty; +import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.plan.Context; +import org.apache.calcite.plan.ConventionTraitDef; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptPlanner; +import org.apache.calcite.plan.RelOptTable.ViewExpander; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.plan.RelTraitDef; +import org.apache.calcite.plan.RelTraitSet; +import org.apache.calcite.plan.volcano.VolcanoPlanner; +import org.apache.calcite.prepare.CalciteCatalogReader; +import org.apache.calcite.rel.RelCollationTraitDef; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.RelRoot; +import org.apache.calcite.rel.metadata.CachingRelMetadataProvider; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeSystem; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexExecutor; +import org.apache.calcite.runtime.Hook; +import org.apache.calcite.schema.SchemaPlus; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlOperatorTable; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.validate.SqlConformance; +import org.apache.calcite.sql.validate.SqlValidator; +import org.apache.calcite.sql2rel.RelDecorrelator; +import org.apache.calcite.sql2rel.SqlRexConvertletTable; +import org.apache.calcite.sql2rel.SqlToRelConverter; +import org.apache.calcite.tools.FrameworkConfig; +import org.apache.calcite.tools.Planner; +import org.apache.calcite.tools.Program; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.ValidationException; +import org.apache.calcite.util.Pair; + +import javax.annotation.Nullable; + +import java.io.Reader; +import java.util.List; +import java.util.Properties; + +/** + * Calcite planner. Clone of Calcite's {@code SqlPlannerImpl}, Review Comment: Clone of Calcite's SqlPlannerImpl as of what version? Including that here will make it easier for people in the future to see if there's any useful changes from SqlPlannerImpl that need to be merged back into our version. Also I think it's called PlannerImpl. ########## sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java: ########## @@ -0,0 +1,432 @@ +/* + * 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.planner; + +import com.google.common.collect.ImmutableList; +import org.apache.calcite.adapter.java.JavaTypeFactory; +import org.apache.calcite.config.CalciteConnectionConfig; +import org.apache.calcite.config.CalciteConnectionConfigImpl; +import org.apache.calcite.config.CalciteConnectionProperty; +import org.apache.calcite.config.CalciteSystemProperty; +import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.plan.Context; +import org.apache.calcite.plan.ConventionTraitDef; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptPlanner; +import org.apache.calcite.plan.RelOptTable.ViewExpander; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.plan.RelTraitDef; +import org.apache.calcite.plan.RelTraitSet; +import org.apache.calcite.plan.volcano.VolcanoPlanner; +import org.apache.calcite.prepare.CalciteCatalogReader; +import org.apache.calcite.rel.RelCollationTraitDef; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.RelRoot; +import org.apache.calcite.rel.metadata.CachingRelMetadataProvider; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeSystem; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexExecutor; +import org.apache.calcite.runtime.Hook; +import org.apache.calcite.schema.SchemaPlus; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlOperatorTable; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.validate.SqlConformance; +import org.apache.calcite.sql.validate.SqlValidator; +import org.apache.calcite.sql2rel.RelDecorrelator; +import org.apache.calcite.sql2rel.SqlRexConvertletTable; +import org.apache.calcite.sql2rel.SqlToRelConverter; +import org.apache.calcite.tools.FrameworkConfig; +import org.apache.calcite.tools.Planner; +import org.apache.calcite.tools.Program; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.ValidationException; +import org.apache.calcite.util.Pair; + +import javax.annotation.Nullable; + +import java.io.Reader; +import java.util.List; +import java.util.Properties; + +/** + * Calcite planner. Clone of Calcite's {@code SqlPlannerImpl}, + * but with the validator made accessible. + */ +public class CalcitePlanner implements Planner, ViewExpander Review Comment: I'm surprised checkstyle accepted this file. We must have pretty loose rules, since it doesn't look to me like a normal Druid source file. I suppose that's OK. ########## sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java: ########## @@ -333,6 +371,9 @@ private PlannerResult planWithDruidConvention( .filter(action -> action.getAction() == Action.READ) .collect(Collectors.toSet()); + // TODO: This is not really a state check since there is a race condition. Review Comment: What's the TODO here — I don't see what change needs to be made later? And, whatever that change is, why not make it now? ########## sql/src/main/java/org/apache/druid/sql/calcite/view/DruidViewMacro.java: ########## @@ -58,6 +58,7 @@ public TranslatableTable apply(final List<Object> arguments) { final RelDataType rowType; try (final DruidPlanner planner = plannerFactory.createPlanner(viewSql, new QueryContext())) { + planner.validate(false); Review Comment: I didn't realize that view expansion was done with an empty query context. I suppose this makes sense: it ensures the view is the same for everyone. IMO, it's better to set this to always `true` instead of always `false`. Right now, it doesn't matter either way, because the context is empty. But if it's ever made nonempty, then `true` is a safer constant value because it defaults to the more restrictive security mode. If that doesn't make sense in the future, then the future person that is adding the context parameters can sort out what's best. A comment would be helpful too, like: ``` // Since the context is empty, it doesn't matter if the authorizeContextParams flag is true or false. ``` ########## sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlParameterizerShuttle.java: ########## @@ -50,34 +63,36 @@ public SqlParameterizerShuttle(PlannerContext plannerContext) public SqlNode visit(SqlDynamicParam param) { try { - if (plannerContext.getParameters().size() > param.getIndex()) { - TypedValue paramBinding = plannerContext.getParameters().get(param.getIndex()); - if (paramBinding == null) { - throw new IAE("Parameter at position[%s] is not bound", param.getIndex()); - } - if (paramBinding.value == null) { - return SqlLiteral.createNull(param.getParserPosition()); - } - SqlTypeName typeName = SqlTypeName.getNameForJdbcType(paramBinding.type.typeId); - if (SqlTypeName.APPROX_TYPES.contains(typeName)) { - return SqlLiteral.createApproxNumeric(paramBinding.value.toString(), param.getParserPosition()); - } - if (SqlTypeName.TIMESTAMP.equals(typeName) && paramBinding.value instanceof Long) { - return SqlLiteral.createTimestamp( - TimestampString.fromMillisSinceEpoch((Long) paramBinding.value), - 0, - param.getParserPosition() - ); - } - - return typeName.createLiteral(paramBinding.value, param.getParserPosition()); - } else { - throw new IAE("Parameter at position[%s] is not bound", param.getIndex()); + if (plannerContext.getParameters().size() <= param.getIndex()) { + throw new IAE("Parameter at position [%s] is not bound", param.getIndex()); + } + TypedValue paramBinding = plannerContext.getParameters().get(param.getIndex()); + if (paramBinding == null) { + throw new IAE("Parameter at position [%s] is not bound", param.getIndex()); + } + if (paramBinding.value == null) { + return SqlLiteral.createNull(param.getParserPosition()); } + SqlTypeName typeName = SqlTypeName.getNameForJdbcType(paramBinding.type.typeId); + if (SqlTypeName.APPROX_TYPES.contains(typeName)) { + return SqlLiteral.createApproxNumeric(paramBinding.value.toString(), param.getParserPosition()); + } + if (SqlTypeName.TIMESTAMP.equals(typeName) && paramBinding.value instanceof Long) { + return SqlLiteral.createTimestamp( + TimestampString.fromMillisSinceEpoch((Long) paramBinding.value), + 0, + param.getParserPosition() + ); + } + + // This throws ClassCastException for a DATE parameter given as Review Comment: If this is the only place that's expected to throw a ClassCastException, how about wrapping only this particular line in the try/catch? -- 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]
