Here is the first PR: https://github.com/apache/calcite/pull/334

• I had to align the SqlUpdate's RowType with the rest of DML operations https://github.com/apache/calcite/pull/334/commits/42a5461c6341ae89f444fdccb99f10e744fa4393. Note that the DML operations (update including) return rowcount instead of ResultSet
• UPDATE implementation requires access to the SET source expressions. While TableModify provides the SET column names it misses the set value expressions. Therefore I've added a new field: List<RexNode> sourceExpressionList to TableModify (and updated many impacted classes ) https://github.com/apache/calcite/pull/334/commits/9c8f141771709b2ab2d2461b7f37f6cb48d0a1c4.
  The sourceExpressionList is initialized from the SqlUpdate:

  private RelNode convertUpdate(SqlUpdate call) {
      final SqlValidatorScope scope = validator.getWhereScope(call.getSourceSelect());
      Blackboard bb = createBlackboard(scope, null, false);
      Builder<RexNode> rexNodeSourceExpressionListBuilder = ImmutableList.builder();
      for (SqlNode n: call.getSourceExpressionList()) {
        RexNode rn = bb.convertExpression(n);
        rexNodeSourceExpressionListBuilder.add(rn);
      }
  ....
  

  It works for the common use cases but failed with update sub-queries. Check the RelToSqlConverter#testUpdateSubQuery! I guess there is a better way to resolve the source expressions for sub-query?

• https://github.com/apache/calcite/pull/334/commits/658cc90461e3e822638405c19bfa463b5cbc1d77 Implements the Rel to SQL for the INSERT, UPDATE and DELETE operations. Note that DML returns row count instead of ResultSet. To support DML result i had to change the hardcoded statement.executeQuery(sql) (in ResultSetEnumerable) into boolean isResultSetProvided = statement.execute(sql) and handle the result according to the isResultSetProvided.
  To Support the Insert i had to make the JdbcTable a ModifiableTable - later is marked as experimental?
  Also i had to lower the JdbcTableModify cost (super.computeSelfCost(planner, mq).multiplyBy(.1)) to make it win over the EnumberableTableModify.

My main concern is that you have not implemented INSERT ... SELECT, only INSERT ... VALUES. Do I have that correct? Since Values is just a RelNode, I wonder whether you could push all supported RelNodes to the JDBC source; you'd get VALUES for free (so you could get rid of SQL_INSERT_VALUES_OPERATOR), and also many variants of SELECT (whatever that dialect of SQL supports).

Except for that, the change looks very good.

A few minor notes:

• Is the @Ignore("CALCITE-1527") needed?
• How about the commented lines doWithConnection(new SqlInsertFunction())?
• Can you explain why @FixMethodOrder(MethodSorters.NAME_ASCENDING) is needed?
• The if (modify.getOperation().equals(Operation.INSERT)) block can be converted to a switch. Can you do so.
• Can you add javadoc for sourceExpressionList in TableModify (either the field or the constructor). Is it required for UPDATE? Is it not allowed for INSERT and DELETE? Add a Preconditions.checkArgument call to enforce the constraint.

Last commit f57db60 should address most of the remarks.

My main concern is that you have not implemented INSERT ... SELECT, only INSERT ... VALUES. Do I have that correct? Since Values is just a RelNode, I wonder whether you could push all supported RelNodes to the JDBC source; you'd get VALUES for free (so you could get rid of SQL_INSERT_VALUES_OPERATOR), and also many variants of SELECT (whatever that dialect of SQL supports)

Now both INSERT ... SELECT and INSERT ... VALUES (ROW1), (ROW2), ... are supported. The INSERT...SELECT delegates to the RelToSqlConverter#visitChild. For the INSERT...VALUES (e.g. tableModify.getInput() instanceof Values) i've tried to re-use RelToSqlConverter#visitChild which in turn delegates to visit(Values) method. At first it seemed work. The vist(Values) converted the INSERT (A, B, C) VALUES (v1, v2, v3) into INSERT (A, B, C) (SELECT v1 AS A, v2 AS B, v3 AS C). Note that that it generates SELECT without a FROM section. Apparently this syntax is not supported by all SQL dialects. It works on Postgresql but fails with HSQLDB. Furthermore if you have VALUES (ROW1), (ROW2) ... the visit(Values) will generate an UNION of SELECTs. Later syntax fails on Posgresql because it is missing aliases for the nested SELECTs. So i made a detour and re-implemented the visit(Values) logic into RelToSqlConverter#covertValues(Values) method using the VALUES(...) syntax instead of nested {{SELECT}}s. Perhaps we should use this code inside the visti(Values) method? What do you think?
There are 2 additional INSERT tests to validate the special cases.

Is the @Ignore("CALCITE-1527") needed?

This test checks for UPDATE sub-queries. It use to work before adding the TableModify#sourceExpressionList field. But now there is not obvious way (for me) to resolve field expressions in update sub-queries and the test fails. So i set the annotation to acknowledge the issue. But my suggestion is to defer the resolution as a separate issue?

How about the commented lines doWithConnection(new SqlInsertFunction())?
Can you explain why @FixMethodOrder(MethodSorters.NAME_ASCENDING) is needed?

Apparently the AssertThat utils doesn't provide good support for DML operators that mutate the DB state. So i've experimented with different approaches to clean the state and insert one test record for some of the tests like update, insert-subquery or delete. After some experimentations it seems i've made it work but please review my test code too!

The if (modify.getOperation().equals(Operation.INSERT)) block can be converted to a switch. Can you do so.

done

Can you add javadoc for sourceExpressionList in TableModify (either the field or the constructor). Is it required for UPDATE? Is it not allowed for INSERT and DELETE? Add a Preconditions.checkArgument call to enforce the constraint.

done

Regarding VALUES. We may need to implement different syntax for different dialects, e.g. SELECT .. UNION .... But VALUES is the standard one, so we should use that if the dialect supports it.

Did you know that you can use VALUES outside of INSERT? For example, VALUES 1 + 2 is equivalent to SELECT 1 + 2. This is why I don't think we should treat INSERT ... VALUES separately; we'll get it if we implement INSERT ... <relational expression>. I think SQL_INSERT_VALUES_OPERATOR can go.

If tests are mutating state, I don't think that ordering tests is a good solution. It will break if we run tests in parallel, for instance. Could you use Schemas.uniqueTableName or a similar approach? If table names are unique then the tests won't bump into each other. The test suite should clean up after itself, and make sure it has a clean initial state before it runs.

Or maybe if tests don't commit, transaction isolation will deal with everything.

If we implement INSERT ... <relational expression> using visitChid(0, <relational expression>) only, it will work for non-VALUES expression. But for VALUES it will hit the existing vist(Values). Later always produces SELECT | SELECT ... UNION... for INSERT ... VALUES and the generated SQL doesn't work on HSQLDB or Postgresql (haven't tested it with other dialects). IMO this can be fixed if we change the visitValues implementation to something similar to covertValues(Values). But because of the VALUES-OUTSIDE-INSERT cases i didn't dare changing existing visit(Values) method. For the time being i've made it conditional inside the TableModify.INSERT method that delegates to covertValues(...) in case of INSERT ... VALUES

I think SQL_INSERT_VALUES_OPERATOR can go.

Do you mean to keep it or remove it? Did you had the chance to review the latest version
Regarding the tests with the last commit i've removed the @FixMethodOrder by trying to build the test fixtures in the doWithConnection instead.

Im getting below exception.

Exception in thread "main" java.sql.SQLException: Error while executing SQL "INSERT INTO "DevRelease_1.8_98.dbo"."survey" (
"id","name" ) values (1,'nameValue')": Node [rel#9:Subset#1.ENUMERABLE.[]] could not be implemented; planner state:

Root: rel#9:Subset#1.ENUMERABLE.[]
Original rel:

Sets:
Set#0, type: RecordType(INTEGER id, VARCHAR(30) name)
rel#2:Subset#0.NONE.[], best=null, importance=0.81
rel#0:LogicalValues.NONE.[[0, 1], [1]](type=RecordType(INTEGER id, VARCHAR(30) name),tuples=[

]), rowcount=1.0, cumulative cost=

Set#1, type: RecordType(BIGINT ROWCOUNT)
rel#4:Subset#1.NONE.[], best=null, importance=0.9
rel#3:LogicalTableModify.NONE.[](input=rel#2:Subset#0.NONE.[],table=[DevRelease_1.8_98.dbo, survey],operation=INSERT,updateColumnList=[],flattened=false), rowcount=1.0, cumulative cost=

at org.apache.calcite.avatica.Helper.createException(Helper.java:56)
at org.apache.calcite.avatica.Helper.createException(Helper.java:41)
at org.apache.calcite.avatica.AvaticaStatement.executeInternal(AvaticaStatement.java:156)
at org.apache.calcite.avatica.AvaticaStatement.executeLargeUpdate(AvaticaStatement.java:236)
at org.apache.calcite.avatica.AvaticaStatement.executeUpdate(AvaticaStatement.java:231)
at com.nanobi.calcite.CalciteMSSQL.main(CalciteMSSQL.java:133)
Caused by: org.apache.calcite.plan.RelOptPlanner$CannotPlanException: Node [rel#9:Subset#1.ENUMERABLE.[]] could not be implemented; planner state:

Christian Tzolov, Reviewing your changes now, and making some modifications (e.g. I've found a way to handle INSERT ... VALUES and INSERT ... SELECT uniformly). Should be able to commit tomorrow.

My changes (un-squashed) are in https://github.com/julianhyde/calcite/tree/1527-jdbc-dml. Testing now.

Nice! Looks like you've replaced visit(Values e) by the convertValues(e) one. Is the conversion of INSERT...VALUES into INSERT (SELECT vx As Cx...) case dropped?
I guess in RelToSqlConverter the line ((SqlSelect) input.node).getWhere() should be input.asStatement() instead?

Regarding INSERT (SELECT vx As Cx...). Yes, that's dropped. In my view, Values is a core relational expression. (Every real language has literals, right?) If a particular SQL dialect doesn't support VALUES then we can work around it using SELECT ... UNION or something.

Regarding input.asSelect() in UPDATE and DELETE. The constructor parameter for each requires a SqlSelect (not a VALUES, nor a UNION) because SQL UPDATE and DELETE syntax handle the source relational expression differently than INSERT does. I propose we leave it as asSelect. If you can find a test case that breaks this, bring it on!

Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/02752fe7. Thanks for the PR, Christian Tzolov!

Christian Tzolov, The test is flapping on JDK 1.8 (stable on 1.7, for some reason). I get intermittent failures like this:

Tests run: 29, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.083 sec <<< FAILURE! - in org.apache.calcite.test.JdbcAdapterTest
testTableModifyDelete(org.apache.calcite.test.JdbcAdapterTest)  Time elapsed: 0.097 sec  <<< FAILURE!
java.lang.AssertionError:

Expected: is <1>
     but: was <2>
        at org.apache.calcite.test.JdbcAdapterTest.testTableModifyDelete(JdbcAdapterTest.java:763)

Can you take a look, please?

Sure let me try to reproduce and fix this. Does it fails when your run integration test on the entire project or only on this test?

We discussed the subject earlier that unlike the other JdbcAdapterTest tests the DML tests mutate the state and it seems that the test suite doesn't always insulate the DB between the tests in the suite.

I see it running the whole suite: (mvn test); mvn -Dtest=JdbcAdapterTest -DfailIfNoTests=false test also reproduces the problem.

I think I've fixed it in https://github.com/julianhyde/calcite/commit/f684c6edc615386e94f2570ad9876f2c8567989c. Testing now.

Resolved in release 1.11.0 (2017-01-11).