[
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:18 PM:
--------------------------------------------------------------------
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#srouceExpression}} field. But now there is not obvious way (for
me) to resolve field expressions in sub-queries and the test fails now. So i
put the annotation to acknowledge that we have an issue to resolve. My
suggestion is to defer the resolution of this in 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. For the former (e.g. SELECT) i'm delegating to the
{{RelToSqlConverter#visitChild}}. For the VALUES inputs (e.g.
{{tableModify.getInput() instanceof Values}}) first i've tried to re-use
{{RelToSqlConverter#visitChild}} which in turn delegates to {{visit(Values)}}
method. At first it seemed fine. The {{vist(Values)}} converts the {{INSERT (A,
B, C) VALUES (v1, v2, v3)}} into {{INSERT (A, B, C) (SELECT v1 AS A, v2 AS B,
v3 AS C)}} without a {{FROM}} . 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) ... visit(Values) generates an UNION of SELECTs
which fails on Posgresql as well (missing aliases for the nested SELECTS). So
i made a detour and re-implemented the {{visit(Values)}} logic into
{{RelToSqlConverter#covertValues(Values)}} method. Later generates
{{VALUES(...)}} rather nested {{SELECT}}s. Perhaps we should use this code
inside the visti(Values) instead? 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#srouceExpression}} field. But now there is not obvious way (for
me) to resolve field expressions in sub-queries and the test fails now. So i
put the annotation to acknowledge that we have an issue to resolve. My
suggestion is to defer the resolution of this in 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)
