[
https://issues.apache.org/jira/browse/CALCITE-1527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15726126#comment-15726126
]
Christian Tzolov edited comment on CALCITE-1527 at 12/6/16 7:21 PM:
--------------------------------------------------------------------
Last commit [f57db60|
https://github.com/apache/calcite/pull/334/commits/f57db602274411c683089b786d18a510ffcc432f]
should address most of the remarks.
bq. 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.
bq. 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?
{quote}How about the commented lines doWithConnection(new SqlInsertFunction())?
Can you explain why @FixMethodOrder(MethodSorters.NAME_ASCENDING) is
needed?{quote}
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!
bq. The if (modify.getOperation().equals(Operation.INSERT)) block can be
converted to a switch. Can you do so.
done
bq. 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
was (Author: tzolov):
Last commit [f57db60|
https://github.com/apache/calcite/pull/334/commits/f57db602274411c683089b786d18a510ffcc432f]
should address most of the remarsk above.
bq. 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.
bq. 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?
{quote}How about the commented lines doWithConnection(new SqlInsertFunction())?
Can you explain why @FixMethodOrder(MethodSorters.NAME_ASCENDING) is
needed?{quote}
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!
bq. The if (modify.getOperation().equals(Operation.INSERT)) block can be
converted to a switch. Can you do so.
done
bq. 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
> Support DML in the JDBC adapter
> -------------------------------
>
> Key: CALCITE-1527
> URL: https://issues.apache.org/jira/browse/CALCITE-1527
> Project: Calcite
> Issue Type: Bug
> Components: jdbc-adapter
> Reporter: Christian Tzolov
> Assignee: Julian Hyde
>
> Currently the JDBC adapter does not support the DML operations: *INSERT*,
> *DELETE* and *UPDATE*.
> Solution needs to convert the parsed *Modify* and *Values* RelNodes into
> *JdbcTableModify*, *JdbcValues* ... such and then in turn into corresponding
> SqlInsert, SqlUpdate and SqlDelete.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
