Hi Julian Hyde
My latest WIP can be found at https://github.com/yeongwei/incubator-calcite/commit/6fa18a93f472d5e4138df52d35f69b189d1c1cc9?diff=split. They are based on your feedback previously, please refer below for other responses too.
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?
YW: Instead of added the "int fetchCount" parameter which is in conflict with the a current overloaded JdbcResultSet#create method. I have added the JdbcMeta#UNLIMITED_COUNT set to -2 instead
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.
YW: I have defaulted the AvaticaStatement#fetchSize to 100 and added test at RemoteDriverTest#testFetchSize for both Statement and PreparedStatement
3. What kinds of statement require an explicit Execute request?
YW: The situation at the JdbcMeta, there is a need to identify if the execution is a Query or DML/DDL kind from there the appropriate ResultSet(normal resultSet / empty / updateCount) could be created which could clearly indicate to the client the nature of the execution. For cases if the underlying datasource is a not a Calcite Connection, then the logic is to rely on the Statement#execute / PreparedStatement#execute to identify the statementType.
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?
YW: To clarify, does this mean during the Execute, client has the option to take all the records at 1 time?
5. Does your change fix https://issues.apache.org/jira/browse/CALCITE-778?
YW: This should fix the
CALCITE-778 because the AvaticaConnection#prepareAndExecuteInternal and AvaticaConnection#executeQueryInternal would make use the Execute Request result and set the appropriate fields of the Statement Object.
6. Our testing framework has thread safety issues, and several tests are disabled. See
CALCITE-687. Can you help us fix that?
YW: I have not done this. This shall be planned.
7. Remove unused "final MetaResultSet metaResultSet;" in JdbcMeta.
8. There are javadoc errors. Run "mvn site".
YW: Done. Errors are not seen
9. JdbcResultSet.empty and .count methods need javadoc.
10. Remove mention of CalcitePrepareImpl from StatementType's javadoc.
11. isUpdateCapable needs better javadoc.
YW: Done. Updated with more details