[ https://issues.apache.org/jira/browse/DRILL-8303?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17604787#comment-17604787 ]
ASF GitHub Bot commented on DRILL-8303: --------------------------------------- jnturton commented on code in PR #2646: URL: https://github.com/apache/drill/pull/2646#discussion_r969561053 ########## exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java: ########## @@ -219,11 +220,14 @@ private static PhysicalPlan getQueryPlan(QueryContext context, String sql, Point handler = new SetOptionHandler(context); Review Comment: It looks like this case could be merged with default case on line 227. But then the code would be less explicit so I'm not requesting a change, just making a note. ########## exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/TableModify.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.drill.exec.physical.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeName; +import org.apache.drill.exec.physical.base.AbstractSingle; +import org.apache.drill.exec.physical.base.PhysicalOperator; +import org.apache.drill.exec.physical.base.PhysicalVisitor; +import org.apache.drill.exec.record.BatchSchema; + +@JsonTypeName("table_modify") +public class TableModify extends AbstractSingle { Review Comment: Is this operator called table_modify rather than, say, table_insert because it might be applied for other operations e.g. an upsert? ########## exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/TableModify.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.drill.exec.physical.config; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeName; +import org.apache.drill.exec.physical.base.AbstractSingle; +import org.apache.drill.exec.physical.base.PhysicalOperator; +import org.apache.drill.exec.physical.base.PhysicalVisitor; +import org.apache.drill.exec.record.BatchSchema; + +@JsonTypeName("table_modify") +public class TableModify extends AbstractSingle { Review Comment: Oh I see, the name table_modify comes from Calcite. ########## contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/DrillJdbcConvention.java: ########## @@ -98,4 +98,31 @@ public Set<RelOptRule> getRules() { public JdbcStoragePlugin getPlugin() { return plugin; } + + private static List<RelOptRule> rules( + RelTrait inputTrait, JdbcConvention out) { + ImmutableList.Builder<RelOptRule> b = ImmutableList.builder(); + foreachRule(out, r -> + b.add(r.config + .as(ConverterRule.Config.class) + .withConversion(r.getOperand().getMatchedClass(), inputTrait, out, r.config.description()) + .withRelBuilderFactory(DrillRelFactories.LOGICAL_BUILDER) + .toRule())); + return b.build(); + } + + private static void foreachRule(JdbcConvention out, Review Comment: Could you give a short explanation of how this change relates to adding INSERT support? I can't trace it. ########## exec/java-exec/src/main/java/org/apache/drill/exec/planner/index/generators/NonCoveringIndexPlanGenerator.java: ########## @@ -188,7 +188,7 @@ public RelNode convertChild(final RelNode topRel, final RelNode input) throws In if (indexDesc.getCollation() != null && !settings.isIndexForceSortNonCovering()) { collation = IndexPlanUtils.buildCollationNonCoveringIndexScan(indexDesc, indexScanRowType, dbscanRowType, indexContext); - if (restrictedScanTraitSet.contains(RelCollationTraitDef.INSTANCE)) { // replace existing trait + if (restrictedScanTraitSet.getTrait(RelCollationTraitDef.INSTANCE) != null) { // replace existing trait Review Comment: I'm curious about the benefit of rewriting this test this way... ########## exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/InsertHandler.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.drill.exec.planner.sql.handlers; + +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.tools.RelConversionException; +import org.apache.calcite.tools.ValidationException; +import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.drill.common.exceptions.UserException; +import org.apache.drill.exec.planner.sql.SchemaUtilites; +import org.apache.drill.exec.store.StoragePluginRegistry; +import org.apache.drill.exec.util.Pointer; +import org.apache.drill.exec.work.foreman.ForemanSetupException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Constructs plan to be executed for collecting metadata and storing it to the Metastore. Review Comment: I don't think this comment belongs here. ########## contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/utils/CreateTableStmtBuilder.java: ########## @@ -18,126 +18,130 @@ package org.apache.drill.exec.store.jdbc.utils; +import org.apache.calcite.schema.ColumnStrategy; +import org.apache.calcite.sql.SqlBasicTypeNameSpec; +import org.apache.calcite.sql.SqlDataTypeSpec; import org.apache.calcite.sql.SqlDialect; +import org.apache.calcite.sql.SqlIdentifier; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlNodeList; +import org.apache.calcite.sql.ddl.SqlDdlNodes; import org.apache.calcite.sql.dialect.PostgresqlSqlDialect; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.commons.collections4.CollectionUtils; import org.apache.drill.common.exceptions.UserException; -import org.apache.drill.common.types.TypeProtos.MinorType; -import org.apache.drill.exec.store.jdbc.JdbcRecordWriter; -import org.apache.parquet.Strings; +import org.apache.drill.common.types.TypeProtos; +import org.apache.drill.common.types.Types; +import org.apache.drill.exec.record.MaterializedField; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.sql.JDBCType; +import java.util.List; public class CreateTableStmtBuilder { Review Comment: Thanks for all the refactoring and clean up in this area. > Add support for inserts into JDBC storage > ----------------------------------------- > > Key: DRILL-8303 > URL: https://issues.apache.org/jira/browse/DRILL-8303 > Project: Apache Drill > Issue Type: Sub-task > Reporter: Vova Vysotskyi > Assignee: Vova Vysotskyi > Priority: Major > > Allow inserting into JDBC tables and pushing down complete insert statement > where possible. -- This message was sent by Atlassian Jira (v8.20.10#820010)