DRILL-8303: Add support for inserts into JDBC storage #2646
Conversation
| @@ -219,11 +220,14 @@ private static PhysicalPlan getQueryPlan(QueryContext context, String sql, Point | |||
| handler = new SetOptionHandler(context); | |||
There was a problem hiding this 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.
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Constructs plan to be executed for collecting metadata and storing it to the Metastore. |
There was a problem hiding this comment.
I don't think this comment belongs here.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.sql.JDBCType; | ||
| import java.util.List; | ||
|
|
||
| public class CreateTableStmtBuilder { |
There was a problem hiding this comment.
Thanks for all the refactoring and clean up in this area.
| import org.apache.drill.exec.record.BatchSchema; | ||
|
|
||
| @JsonTypeName("table_modify") | ||
| public class TableModify extends AbstractSingle { |
There was a problem hiding this comment.
Is this operator called table_modify rather than, say, table_insert because it might be applied for other operations e.g. an upsert?
There was a problem hiding this comment.
Oh I see, the name table_modify comes from Calcite.
| @@ -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 | |||
There was a problem hiding this comment.
I'm curious about the benefit of rewriting this test this way...
There was a problem hiding this comment.
The existing code wouldn't work as expected, trait set doesn't contain RelCollationTraitDef instances.
| return b.build(); | ||
| } | ||
|
|
||
| private static void foreachRule(JdbcConvention out, |
There was a problem hiding this comment.
Could you give a short explanation of how this change relates to adding INSERT support? I can't trace it.
There was a problem hiding this comment.
It is related not only to INSERT but to general JDBC functionality. With this change, rules will match not only to the NONE convention but also to the DRILL_LOGICAL one, so as a result, Drill will be able to push down more operators for complex queries, where possible.
vvysotskyi
force-pushed
the
DRILL-8303
branch
from
52b1eeb
to
bb46246
Compare
DRILL-8303: Add support for inserts into JDBC storage
Description
This PR contains changes required for adding support for inserts into JDBC storage.
Inserting data is implemented based on generating and executing
INSERT INTO ... VALUES()statements.It is also possible to push down the complete
INSERT INTOstatement where possible (when using insert from values or another JDBC table).Rewritten existing code for JDBC CTAS to use Calcite for generating
CREATE TABLEandINSERT INTOstatements, simplified reading values to write.Added ability to match JDBC rules to both
NONEandDRILL_LOGICALconventions to allow the planner to push down more cases than before.Documentation
Insert statement syntax completely matches Calcites definition and is the following:
{ INSERT | UPSERT } INTO tablePrimary [ '(' column [, column ]* ')' ] queryWhere
queryis an arbitrary SQL query.Testing
Added unit tests for inserts, existing tests pass.