[
https://issues.apache.org/jira/browse/CALCITE-705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14623103#comment-14623103
]
Julian Hyde edited comment on CALCITE-705 at 7/11/15 12:36 AM:
---------------------------------------------------------------
And I apologize that it took me so long to review this. Thanks for squashing
commits -- that helps. I have a few questions/comments:
1. In JdbcResultSet: "-1 still limits to 100 but -2 does not limit to any
number" isn't very clean. Would it be cleaner if you just added a fetchCount
parameter?
2. The value "100" is in several places. I think you should be using
Statement.fetchSize. Maybe that field should have a default value of 100. Add a
test for Statement.setFetchSize.
3. What kinds of statement require an explicit Execute request?
4. Would it be possible to (maybe in future) to have a combined Execute+Fetch
request that fetches the first N rows in one network round trip?
5. Does your change fix https://issues.apache.org/jira/browse/CALCITE-778?
6. Our testing framework has thread safety issues, and several tests are
disabled. See CALCITE-687. Can you help us fix that?
7. Remove unused "final MetaResultSet metaResultSet;" in JdbcMeta.
8. There are javadoc errors. Run "mvn site".
9. JdbcResultSet.empty and .count methods need javadoc.
10. Remove mention of CalcitePrepareImpl from StatementType's javadoc.
11. isUpdateCapable needs better javadoc.
was (Author: julianhyde):
And I apologize that it took me so long to review this.
1. In JdbcResultSet: "-1 still limits to 100 but -2 does not limit to any
number" isn't very clean. Would it be cleaner if you just added a fetchCount
parameter?
2. The value "100" is in several places. I think you should be using
Statement.fetchSize. Maybe that field should have a default value of 100. Add a
test for Statement.setFetchSize.
3. What kinds of statement require an explicit Execute request?
4. Would it be possible to (maybe in future) to have a combined Execute+Fetch
request that fetches the first N rows in one network round trip?
5. Does your change fix https://issues.apache.org/jira/browse/CALCITE-778?
6. Our testing framework has thread safety issues, and several tests are
disabled. See CALCITE-687. Can you help us fix that?
7. Remove unused "final MetaResultSet metaResultSet;" in JdbcMeta.
8. There are javadoc errors. Run "mvn site".
9. JdbcResultSet.empty and .count methods need javadoc.
10. Remove mention of CalcitePrepareImpl from StatementType's javadoc.
11. isUpdateCapable needs better javadoc.
> AvaticaStatement execute method to support DML
> ----------------------------------------------
>
> Key: CALCITE-705
> URL: https://issues.apache.org/jira/browse/CALCITE-705
> Project: Calcite
> Issue Type: New Feature
> Components: avatica
> Reporter: YeongWei
> Assignee: Julian Hyde
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
