Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-705

DML in Avatica, and split Execute out from Fetch request

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5.0
    • Component/s: avatica
    • Labels:
      None

      Issue Links

        Activity

        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde,

        I am attempting to resolve the return boolean value for the AvaticaStatement#execute method.

        However from CalcitePrepareImpl#prepare2_, there is a CASE STATEMENT which evaluates with SqlNode#getKind then create the appropriate RelDataType Object. For the case of INSERT, DELETE, UPDATE and EXPLAIN, all will invoke the RelOptUtil#createDmlRowType which creates a single element list consisting a Pair identified with ROWCOUNT.

        Therefore upon DML execution, the above gets bubbled up as a ResultSet having one column (ROWCOUNT) with one row (ROWCOUNT's value).

        From JDBC perspective, when user executes the statement#execute("INSERT INTO test values (1, 'ABC')") the result should return the boolean false since the DML does not expect ResultSet.

        However currently a ResultSet with column "ROWCOUNT" is returned causing the statement#execute to return true. Which does not follow the JDBC standard.

        Can you shed some on around this matter? As DML should cause the statement#execute to return false.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde , I am attempting to resolve the return boolean value for the AvaticaStatement#execute method. However from CalcitePrepareImpl#prepare2_, there is a CASE STATEMENT which evaluates with SqlNode#getKind then create the appropriate RelDataType Object. For the case of INSERT, DELETE, UPDATE and EXPLAIN, all will invoke the RelOptUtil#createDmlRowType which creates a single element list consisting a Pair identified with ROWCOUNT. Therefore upon DML execution, the above gets bubbled up as a ResultSet having one column (ROWCOUNT) with one row (ROWCOUNT's value). From JDBC perspective, when user executes the statement#execute("INSERT INTO test values (1, 'ABC')") the result should return the boolean false since the DML does not expect ResultSet. However currently a ResultSet with column "ROWCOUNT" is returned causing the statement#execute to return true. Which does not follow the JDBC standard. Can you shed some on around this matter? As DML should cause the statement#execute to return false. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        You are correct. DML statements return an updateCount and no result set. SELECT returns updateCount = -1 and a result set.

        Run the test RemoteDriverTest.testCreateInsertUpdateDrop and you will see that this works. in particular, "statement.execute(insert)" returns false. Put a breakpoint in the constructor of Service.ResultSetResponse and you will see how the updateCount and firstFrame fields are set differently for DML, DDL and queries.

        This proves that RemoteMeta does the right thing. Now we need to make CalciteMeta correct also.

        At a low level, it is fine that a Calcite DML statement returns a "result set" containing a count in a ROWCOUNT column. But somewhere at a higher point in the stack needs to present that as an updateCount and not a result set. It should notice that we are running a DML statement. Can you figure out where to put that code?

        Your implementation must call Meta.PrepareCallback.assign with updateCount = 1, firstFrame = null.

        Show
        julianhyde Julian Hyde added a comment - You are correct. DML statements return an updateCount and no result set. SELECT returns updateCount = -1 and a result set. Run the test RemoteDriverTest.testCreateInsertUpdateDrop and you will see that this works. in particular, "statement.execute(insert)" returns false. Put a breakpoint in the constructor of Service.ResultSetResponse and you will see how the updateCount and firstFrame fields are set differently for DML, DDL and queries. This proves that RemoteMeta does the right thing. Now we need to make CalciteMeta correct also. At a low level, it is fine that a Calcite DML statement returns a "result set" containing a count in a ROWCOUNT column. But somewhere at a higher point in the stack needs to present that as an updateCount and not a result set. It should notice that we are running a DML statement. Can you figure out where to put that code? Your implementation must call Meta.PrepareCallback.assign with updateCount = 1, firstFrame = null.
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde,

        I have attempted some preliminary implementation on my local. However below are a few items that I may need your guidance,

        1. The ResultSet with "ROWCOUNT=<int>" is only available after the call of callback.execute within CalciteMetaImpl#prepareAndExecute. Therefore at the point of callback.assign, we would not have the value for updateCount. I am looking at if the execution sequence for the Meta.PrepareCallback needs to be altered?

        2. To have CalciteMetaImpl to identify if the execution is a DML or Query, I intend to introduce say CalciteSignature#isDml which can be facilitated during the CalcitePrepareImpl#prepare2_ but that would be mean to overload the constructor CalciteSignature, do you think this may be beneficial in the long run, say when DDL comes into the picture?

        Let me know what do you think.

        Thank you very much!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde , I have attempted some preliminary implementation on my local. However below are a few items that I may need your guidance, 1. The ResultSet with "ROWCOUNT=<int>" is only available after the call of callback.execute within CalciteMetaImpl#prepareAndExecute. Therefore at the point of callback.assign, we would not have the value for updateCount. I am looking at if the execution sequence for the Meta.PrepareCallback needs to be altered? 2. To have CalciteMetaImpl to identify if the execution is a DML or Query, I intend to introduce say CalciteSignature#isDml which can be facilitated during the CalcitePrepareImpl#prepare2_ but that would be mean to overload the constructor CalciteSignature, do you think this may be beneficial in the long run, say when DDL comes into the picture? Let me know what do you think. Thank you very much!
        Hide
        julianhyde Julian Hyde added a comment -

        Regarding 1. PrepareCallback#assign is only to assign the prepare result. I think you should keep it at the same place in the execution sequence. But if ExecuteResult (created from the results of executing) contains a row count, that should supersede.

        Regarding 2. Makes sense. But can you generalize a bit, and put

        enum StatementType {
          SELECT, INSERT, UPDATE, DELETE, UPSERT, MERGE, OTHER_DML,
          CREATE, DROP, ALTER, OTHER_DDL, CALL,
        } 

        in Avatica? Add an overloaded constructor to Meta.Signature and CalciteSignature, and mark the previous constructor deprecated.

        Show
        julianhyde Julian Hyde added a comment - Regarding 1. PrepareCallback#assign is only to assign the prepare result. I think you should keep it at the same place in the execution sequence. But if ExecuteResult (created from the results of executing) contains a row count, that should supersede. Regarding 2. Makes sense. But can you generalize a bit, and put enum StatementType { SELECT, INSERT, UPDATE, DELETE, UPSERT, MERGE, OTHER_DML, CREATE, DROP, ALTER, OTHER_DDL, CALL, } in Avatica? Add an overloaded constructor to Meta.Signature and CalciteSignature, and mark the previous constructor deprecated.
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        I have amended my implementation and the current work-in-progress can be found at,

        https://github.com/yeongwei/incubator-calcite/commit/ae2aa52508d027e8f42f47af98b7a5be0c741f6b?diff=split

        Feel free to take a look.

        There are 3 Unit tests that may be worth to run,
        1. CalciteRemoteDriverTest#testInsert - Test insert operation remotely via CalciteMetaImpl
        2. JdbcFrontLinqBackTest#testInsert3 - Test insert with Calcite local connection
        3. RemoteDriverTest#testCreateInsertUpdateDrop - Previously pointed out by you

        Further thoughts,
        1. It seems that it is worth while to put Meta.Signature / CalciteSignature into AvaticaStatement instead of AvaticaPreparedStatement
        2. WIth #1, we may attempt to do a AvaticaStatement#setSignature during prepareCallback#assign then use it during the prepareCallback#execute

        Let me know your thoughts.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde I have amended my implementation and the current work-in-progress can be found at, https://github.com/yeongwei/incubator-calcite/commit/ae2aa52508d027e8f42f47af98b7a5be0c741f6b?diff=split Feel free to take a look. There are 3 Unit tests that may be worth to run, 1. CalciteRemoteDriverTest#testInsert - Test insert operation remotely via CalciteMetaImpl 2. JdbcFrontLinqBackTest#testInsert3 - Test insert with Calcite local connection 3. RemoteDriverTest#testCreateInsertUpdateDrop - Previously pointed out by you Further thoughts, 1. It seems that it is worth while to put Meta.Signature / CalciteSignature into AvaticaStatement instead of AvaticaPreparedStatement 2. WIth #1, we may attempt to do a AvaticaStatement#setSignature during prepareCallback#assign then use it during the prepareCallback#execute Let me know your thoughts. Thanks!
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        I have updated the implementation and the current work-in-progress can be found at,

        https://github.com/yeongwei/incubator-calcite/commit/9807c4a68429040a564f18bdfb1c9d9d2497b40c?diff=split

        Highlights,
        1) Signature as a class field with AvaticaStatement
        2) StatementType enum within Meta.java and UPDATE_CAPABLE list within StatementType Object
        3) IsUpdateCapable to handle DML response for both LOCAL and REMOTE
        4) Implemented only the INSERT

        UnitTests,
        1) LOCAL Statement INSERT
        JdbcFrontLinqBackTest#testInsert3
        2) LOCAL PreparedStatement INSERT
        JdbcFrontLinqBackTest#testPreparedStatementInsert
        3) REMOTE Statement INSERT
        CalciteRemoteDriverTest#testInsert
        4) REMOTE PreparedStatement INSERT
        CalciteRemoteDriverTest#testRemotePreparedStatementInsert

        Let me know you comments.

        Currently will continue to work on the UPDATE and DELETE that should piggy back on the implementation shown above

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde I have updated the implementation and the current work-in-progress can be found at, https://github.com/yeongwei/incubator-calcite/commit/9807c4a68429040a564f18bdfb1c9d9d2497b40c?diff=split Highlights, 1) Signature as a class field with AvaticaStatement 2) StatementType enum within Meta.java and UPDATE_CAPABLE list within StatementType Object 3) IsUpdateCapable to handle DML response for both LOCAL and REMOTE 4) Implemented only the INSERT UnitTests, 1) LOCAL Statement INSERT JdbcFrontLinqBackTest#testInsert3 2) LOCAL PreparedStatement INSERT JdbcFrontLinqBackTest#testPreparedStatementInsert 3) REMOTE Statement INSERT CalciteRemoteDriverTest#testInsert 4) REMOTE PreparedStatement INSERT CalciteRemoteDriverTest#testRemotePreparedStatementInsert Let me know you comments. Currently will continue to work on the UPDATE and DELETE that should piggy back on the implementation shown above Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        Looks pretty good.

        Can you remove Meta.StatementType.UPDATE_CAPABLE? I don't like the idea of a mutable list. How about

        enum StatementType {
          ...
        
          public boolean canUpdate() {
            switch (this) {
            case INSERT:
              return true;
            default:
              return false;
            }
          }
        

        In Signature, make statementType public, mandatory and final. If you don't know, assume SELECT. Modify the existing constructors for Signature and CalciteSignature, don't add new ones.

        In LocalService, don't assume that the count is a Long. Just assume that it is a Number, and call Number.intValue().

        Show
        julianhyde Julian Hyde added a comment - Looks pretty good. Can you remove Meta.StatementType.UPDATE_CAPABLE? I don't like the idea of a mutable list. How about enum StatementType { ... public boolean canUpdate() { switch ( this ) { case INSERT: return true ; default : return false ; } } In Signature, make statementType public, mandatory and final. If you don't know, assume SELECT. Modify the existing constructors for Signature and CalciteSignature, don't add new ones. In LocalService, don't assume that the count is a Long. Just assume that it is a Number, and call Number.intValue().
        Hide
        yeongwei YeongWei added a comment - - edited

        Hi Julian Hyde,

        I have made the changes as per your guidance, the Work-In-Progress can be found at the following link,

        https://github.com/yeongwei/incubator-calcite/commit/1b654b5b58bdea680bce35011638d05b55563f79?diff=split

        Highlights,
        1. JdbcMeta#prepare needs to handle connection from both underlying data sources (HSQLDB, MYSQL, etc) and also CalciteConnections. To facilitata StatementType information at this level, I have create the AvaticaStatement#getStatementType. Therefore, if the PreparedStatement is a wrapped of AvaticaPreparedStatement then proceed to unwrap then get the StatementType else will treat as null.

        2. DDL / DatabaseMetadata scenarios will have the AvaticaStatement#signature as null, therefore additional check added in AvaticaConnection#isUpdateCapable

        3. At various point where the routine needs to construct the Signature Objects. If there is existing signature then attempt to reuse the statementType else assumes SELECT.

        Let me know what do you think.

        Thanks!

        Show
        yeongwei YeongWei added a comment - - edited Hi Julian Hyde , I have made the changes as per your guidance, the Work-In-Progress can be found at the following link, https://github.com/yeongwei/incubator-calcite/commit/1b654b5b58bdea680bce35011638d05b55563f79?diff=split Highlights, 1. JdbcMeta#prepare needs to handle connection from both underlying data sources (HSQLDB, MYSQL, etc) and also CalciteConnections. To facilitata StatementType information at this level, I have create the AvaticaStatement#getStatementType. Therefore, if the PreparedStatement is a wrapped of AvaticaPreparedStatement then proceed to unwrap then get the StatementType else will treat as null. 2. DDL / DatabaseMetadata scenarios will have the AvaticaStatement#signature as null, therefore additional check added in AvaticaConnection#isUpdateCapable 3. At various point where the routine needs to construct the Signature Objects. If there is existing signature then attempt to reuse the statementType else assumes SELECT. Let me know what do you think. Thanks!
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde,

        I have updated the implementation around AvaticaConnection#isUpdateCapable to handle when Signature#statementType is NULL.

        https://github.com/yeongwei/incubator-calcite/commit/29f235ee2d0807c664b7b4a2d4fa5f2697dd4a02?diff=split

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde , I have updated the implementation around AvaticaConnection#isUpdateCapable to handle when Signature#statementType is NULL. https://github.com/yeongwei/incubator-calcite/commit/29f235ee2d0807c664b7b4a2d4fa5f2697dd4a02?diff=split Thanks!
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde,

        I revisited the implementation around JdbcMeta, I come to realise some challenges to support DML.

        1. JdbcMeta#prepare
        If the PreparedStatement belongs to an underlying data source (E.g. MySQL, HSQLDB, etc), it is unable to deduce the Signature#statementType. Only the ones belong to Calcite could be deduced.

        This includes the list of ColumnMetaData that needs to be responded back to client which has to be deduced during the prepare stage.

        2. JdbcMeta#fetch
        If the preparedStatement.execute returns a FALSE then proceed invoke a JdbcResultSet#Count which takes the preparedStatement.getUpdateCount as argument

        Currently #2 is in place but #1 seemed challenging.

        Hope to hear from you.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde , I revisited the implementation around JdbcMeta, I come to realise some challenges to support DML. 1. JdbcMeta#prepare If the PreparedStatement belongs to an underlying data source (E.g. MySQL, HSQLDB, etc), it is unable to deduce the Signature#statementType. Only the ones belong to Calcite could be deduced. This includes the list of ColumnMetaData that needs to be responded back to client which has to be deduced during the prepare stage. 2. JdbcMeta#fetch If the preparedStatement.execute returns a FALSE then proceed invoke a JdbcResultSet#Count which takes the preparedStatement.getUpdateCount as argument Currently #2 is in place but #1 seemed challenging. Hope to hear from you. Thanks!
        Hide
        xhoong Xavier FH Leong added a comment -

        I think this calls for additional RPC for ExecuteRequest, so that the execution of statement can be separated from the FetchRequest.

        Another option is to use prepareAndExecute with sql == null, since now that the prepareAndExecute also takes StatementHandle (where it can then grab the cached statement, check if it's a preparedStatement), and execute

        Show
        xhoong Xavier FH Leong added a comment - I think this calls for additional RPC for ExecuteRequest, so that the execution of statement can be separated from the FetchRequest. Another option is to use prepareAndExecute with sql == null, since now that the prepareAndExecute also takes StatementHandle (where it can then grab the cached statement, check if it's a preparedStatement), and execute
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        After a some reviews around the source code, it is decided that adding the ExecuteRequest may be a more feasible approach.

        The Work-In-Progress can be found at,

        https://github.com/yeongwei/incubator-calcite/commit/19fee91095a9323b09d55b1aba4ceec07df95c05?diff=split

        Let me know what do you think.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde After a some reviews around the source code, it is decided that adding the ExecuteRequest may be a more feasible approach. The Work-In-Progress can be found at, https://github.com/yeongwei/incubator-calcite/commit/19fee91095a9323b09d55b1aba4ceec07df95c05?diff=split Let me know what do you think. Thanks!
        Hide
        ndimiduk Nick Dimiduk added a comment -

        Bulk edit assigning avatica component to obvious issues.

        Show
        ndimiduk Nick Dimiduk added a comment - Bulk edit assigning avatica component to obvious issues.
        Hide
        yeongwei YeongWei added a comment - - edited

        Hi Julian,

        I apologize that the previous WIP that I have present has all the related works as separate commits. This may cause difficulty for you to review them.

        I have made an attempt to consolidate them into one commit as the WIP below,

        https://github.com/yeongwei/incubator-calcite/commit/e6667e44f93e694c8cb129cff105682fb5fcbc24?diff=split

        Thanks!

        Show
        yeongwei YeongWei added a comment - - edited Hi Julian, I apologize that the previous WIP that I have present has all the related works as separate commits. This may cause difficulty for you to review them. I have made an attempt to consolidate them into one commit as the WIP below, https://github.com/yeongwei/incubator-calcite/commit/e6667e44f93e694c8cb129cff105682fb5fcbc24?diff=split Thanks!
        Hide
        julianhyde Julian Hyde added a comment - - edited

        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.

        Show
        julianhyde Julian Hyde added a comment - - edited 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.
        Hide
        julianhyde Julian Hyde added a comment -

        YeongWei, do you think you can get this patch complete by Wednesday (7/22) as we are stabilizing for a 1.4 release?

        Show
        julianhyde Julian Hyde added a comment - YeongWei , do you think you can get this patch complete by Wednesday (7/22) as we are stabilizing for a 1.4 release?
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde,

        Apologise for the delay.

        I will not be able to complete the patch by Wednesday (7/22).

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde , Apologise for the delay. I will not be able to complete the patch by Wednesday (7/22). Thanks!
        Hide
        yeongwei YeongWei added a comment -

        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.

        Thanks!

        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.

        YW: Done

        8. There are javadoc errors. Run "mvn site".

        YW: Done. Errors are not seen

        9. JdbcResultSet.empty and .count methods need javadoc.

        YW: Done

        10. Remove mention of CalcitePrepareImpl from StatementType's javadoc.

        YW: Done

        11. isUpdateCapable needs better javadoc.

        YW: Done. Updated with more details

        Show
        yeongwei YeongWei added a comment - 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. Thanks! 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. YW: Done 8. There are javadoc errors. Run "mvn site". YW: Done. Errors are not seen 9. JdbcResultSet.empty and .count methods need javadoc. YW: Done 10. Remove mention of CalcitePrepareImpl from StatementType's javadoc. YW: Done 11. isUpdateCapable needs better javadoc. YW: Done. Updated with more details
        Hide
        yeongwei YeongWei added a comment - - edited

        Hi Julian Hyde

        If all is good, I have created a Pull Request with the title "DML + ExecuteRequest #108".

        https://github.com/apache/incubator-calcite/pull/108

        Thanks!

        Show
        yeongwei YeongWei added a comment - - edited Hi Julian Hyde If all is good, I have created a Pull Request with the title "DML + ExecuteRequest #108". https://github.com/apache/incubator-calcite/pull/108 Thanks!
        Hide
        julianhyde Julian Hyde added a comment - - edited

        On JDK 1.8, using https://github.com/yeongwei/incubator-calcite/commit/6fa18a93f472d5e4138df52d35f69b189d1c1cc9, "mvn clean && mvn test" fails for me, with

        Running org.apache.calcite.avatica.RemoteDriverTest
        Tests run: 27, Failures: 1, Errors: 0, Skipped: 7, Time elapsed: 4.353 sec <<< FAILURE! - in org.apache.calcite.avatica.RemoteDriverTest
        testPrepareBindExecuteFetch(org.apache.calcite.avatica.RemoteDriverTest)  Time elapsed: 0.01 sec  <<< FAILURE!
        java.lang.AssertionError: 
        Expected: "exception while executing query: unbound parameter"
             but: was "Error while execute"
        	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        	at org.junit.Assert.assertThat(Assert.java:865)
        	at org.junit.Assert.assertThat(Assert.java:832)
        	at org.apache.calcite.avatica.RemoteDriverTest.checkPrepareBindExecuteFetch(RemoteDriverTest.java:708)
        	at org.apache.calcite.avatica.RemoteDriverTest.testPrepareBindExecuteFetch(RemoteDriverTest.java:687)
        
        
        Results :
        
        Failed tests: 
          RemoteDriverTest.testPrepareBindExecuteFetch:687->checkPrepareBindExecuteFetch:708 
        Expected: "exception while executing query: unbound parameter"
             but: was "Error while execute"
        

        Do you get this error?

        Show
        julianhyde Julian Hyde added a comment - - edited On JDK 1.8, using https://github.com/yeongwei/incubator-calcite/commit/6fa18a93f472d5e4138df52d35f69b189d1c1cc9 , "mvn clean && mvn test" fails for me, with Running org.apache.calcite.avatica.RemoteDriverTest Tests run: 27, Failures: 1, Errors: 0, Skipped: 7, Time elapsed: 4.353 sec <<< FAILURE! - in org.apache.calcite.avatica.RemoteDriverTest testPrepareBindExecuteFetch(org.apache.calcite.avatica.RemoteDriverTest) Time elapsed: 0.01 sec <<< FAILURE! java.lang.AssertionError: Expected: "exception while executing query: unbound parameter" but: was "Error while execute" at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20) at org.junit.Assert.assertThat(Assert.java:865) at org.junit.Assert.assertThat(Assert.java:832) at org.apache.calcite.avatica.RemoteDriverTest.checkPrepareBindExecuteFetch(RemoteDriverTest.java:708) at org.apache.calcite.avatica.RemoteDriverTest.testPrepareBindExecuteFetch(RemoteDriverTest.java:687) Results : Failed tests: RemoteDriverTest.testPrepareBindExecuteFetch:687->checkPrepareBindExecuteFetch:708 Expected: "exception while executing query: unbound parameter" but: was "Error while execute" Do you get this error?
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde,

        Sincerely apologize for the long delay on this matter! I am very sorry about it!

        Please refer to the details below.

        *Fixed Reported Exception during "mvn test"*
        I have made attempts to look at the exception you have encountered about. It failed with the Unit Test(s) which attempts to test PreparedStatement without setting the bind variables, you may refer to the test method at `org.apache.calcite.avatica.RemoteDriverTest#testPrepareBindExecuteFetch`. I initially did not capture because my environment was using the JDK 1.7 and according to the method mentioned, it skips the test if it sees a JDK 1.7.

        The exception bubbles up from a NPE. This is because when the bind variables werenot set, the `TypeValue[] parameterValue` variable will have null elements that is results in method invocation on null value.

        The fix to this matter, a static method as `MetaImpl#checkParameterValueHasNul`l that is used in both `JdbcMeta#execute` and `CalciteConnectionImpl#enumerable` to check if parameterValue has null element then throw the exception of "unbound parameter"

        *Fixes related to Exception propagation*
        I have also fixed the sections below that I believe was not creation exception correctly,

        `org.apache.calcite.avatica.AvaticaConnection#executeQueryInternal` should use the `e.getMessage()` during the `helper.createException(..)` call

        `org.apache.calcite.avatica.jdbc.JdbcMeta#propagate` should create use `RuntimeException(e.getMessage())`

        *Ignored Unit Test*
        The Unit Tests below are ignore because it is using the `CalciteAssert#assertQuery` that uses the `executeQuery` to evaluate the DML s. This will now cause problems below the `executeQuery` strictly expects a `resultSet` but with the DML implementation here there will be no `resultSet` returned for DML. The proposal here is to change the `CalciteAssert#assertQuery` to used `execute()` instead of `executeQuery()`

        1. `org.apache.calcite.test.JdbcFrontLinqBackTest#testInsert`
        2. `org.apache.calcite.test.JdbcFrontLinqBackTest#testInsert2`

        I have re-commited into the github, the same pull request can be used,
        https://github.com/apache/incubator-calcite/pull/108

        The commit is at,
        https://github.com/yeongwei/incubator-calcite/commit/638947bd27f5fce1c7318661e6f5bc283f62bb2a?diff=split

        I am currently using JDK 1.8 as below,
        java version "1.8.0_60"
        Java(TM) SE Runtime Environment (build 1.8.0_60-b27)
        Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode)

        Look forward to hear from you.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde , Sincerely apologize for the long delay on this matter! I am very sorry about it! Please refer to the details below. * Fixed Reported Exception during "mvn test" * I have made attempts to look at the exception you have encountered about. It failed with the Unit Test(s) which attempts to test PreparedStatement without setting the bind variables, you may refer to the test method at `org.apache.calcite.avatica.RemoteDriverTest#testPrepareBindExecuteFetch`. I initially did not capture because my environment was using the JDK 1.7 and according to the method mentioned, it skips the test if it sees a JDK 1.7. The exception bubbles up from a NPE. This is because when the bind variables werenot set, the `TypeValue[] parameterValue` variable will have null elements that is results in method invocation on null value. The fix to this matter, a static method as `MetaImpl#checkParameterValueHasNul`l that is used in both `JdbcMeta#execute` and `CalciteConnectionImpl#enumerable` to check if parameterValue has null element then throw the exception of "unbound parameter" * Fixes related to Exception propagation * I have also fixed the sections below that I believe was not creation exception correctly, `org.apache.calcite.avatica.AvaticaConnection#executeQueryInternal` should use the `e.getMessage()` during the `helper.createException(..)` call `org.apache.calcite.avatica.jdbc.JdbcMeta#propagate` should create use `RuntimeException(e.getMessage())` * Ignored Unit Test * The Unit Tests below are ignore because it is using the `CalciteAssert#assertQuery` that uses the `executeQuery` to evaluate the DML s. This will now cause problems below the `executeQuery` strictly expects a `resultSet` but with the DML implementation here there will be no `resultSet` returned for DML. The proposal here is to change the `CalciteAssert#assertQuery` to used `execute()` instead of `executeQuery()` 1. `org.apache.calcite.test.JdbcFrontLinqBackTest#testInsert` 2. `org.apache.calcite.test.JdbcFrontLinqBackTest#testInsert2` I have re-commited into the github, the same pull request can be used, https://github.com/apache/incubator-calcite/pull/108 The commit is at, https://github.com/yeongwei/incubator-calcite/commit/638947bd27f5fce1c7318661e6f5bc283f62bb2a?diff=split I am currently using JDK 1.8 as below, java version "1.8.0_60" Java(TM) SE Runtime Environment (build 1.8.0_60-b27) Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode) Look forward to hear from you. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        YeongWei, Good to hear from you - I knew you would get back to this eventually! I will review.

        Show
        julianhyde Julian Hyde added a comment - YeongWei , Good to hear from you - I knew you would get back to this eventually! I will review.
        Hide
        julianhyde Julian Hyde added a comment -

        I get javadoc errors when I run "mvn site". Can you fix please?

        Also you'll need to rebase onto master. CALCITE-840 is a pretty major change.

        Show
        julianhyde Julian Hyde added a comment - I get javadoc errors when I run "mvn site". Can you fix please? Also you'll need to rebase onto master. CALCITE-840 is a pretty major change.
        Hide
        elserj Josh Elser added a comment -

        Also you'll need to rebase onto master. CALCITE-840 is a pretty major change.

        LMK if this proves difficult. I can help.

        Show
        elserj Josh Elser added a comment - Also you'll need to rebase onto master. CALCITE-840 is a pretty major change. LMK if this proves difficult. I can help.
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        I will look into it.

        Hi Josh Elser

        Thank you for the offer. Let me look into it and get back to you guys.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde I will look into it. Hi Josh Elser Thank you for the offer. Let me look into it and get back to you guys. Thanks!
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        Quick update, I am in the mist of merging the code with master as upstream; In the mean time going through the Protobuf stuffs which needs to be implemented.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde Quick update, I am in the mist of merging the code with master as upstream; In the mean time going through the Protobuf stuffs which needs to be implemented. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        Thanks for the update. Glad you are making progress.

        As you may have seen on the dev list we are thinking of making a release in ~2 weeks. I think this change could comfortably make it into the release.

        Show
        julianhyde Julian Hyde added a comment - Thanks for the update. Glad you are making progress. As you may have seen on the dev list we are thinking of making a release in ~2 weeks. I think this change could comfortably make it into the release.
        Hide
        yeongwei YeongWei added a comment -

        Hi Josh Elser

        I needed to add new schema into the requests.proto as below,

        // Request for Meta#execute(Meta.ConnectionHandle, list, long)
        message ExecuteRequest {
          uint32 statement_id = 1;
          repeated TypedValue parameter_values = 2;
          uint64 offset = 3;
        }
        

        Then I proceeded with the,

        $ cd avatica 
        $ ./src/main/scripts/generate-protobuf.sh
        

        However, Eclipse is unable to compile, then I proceed to inspected the generated codes but found out that there are a lot differences as compared to master, example as below,

        -    private ExecuteResponse(com.google.protobuf.GeneratedMessage.Builder builder) {
        +    private ExecuteResponse(com.google.protobuf.GeneratedMessage.Builder<?> builder) {
               super(builder);
             }
             private ExecuteResponse() {
        @@ -1090,8 +1102,7 @@ private ExecuteResponse() {
             }
             private ExecuteResponse(
                 com.google.protobuf.CodedInputStream input,
        -        com.google.protobuf.ExtensionRegistryLite extensionRegistry)
        -        throws com.google.protobuf.InvalidProtocolBufferException {
        +        com.google.protobuf.ExtensionRegistryLite extensionRegistry) {
               this();
               int mutable_bitField0_ = 0;
               try {
        
             public static org.apache.calcite.avatica.proto.Responses.ResultSetResponse getDefaultInstance() {
        -      return defaultInstance;
        +      return DEFAULT_INSTANCE;
        +    }
        
        

        Especially on the exceptions that most should be throwing the RuntimeException

        Below is my protoc version,

        root@sherpa:/opt/java# cd ~
        root@sherpa:~# which protoc
        /usr/local/bin/protoc
        root@sherpa:~# /usr/local/bin/protoc --version
        libprotoc 3.0.0
        root@sherpa:~# 
        

        Did I miss something anywhere?

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Josh Elser I needed to add new schema into the requests.proto as below, // Request for Meta#execute(Meta.ConnectionHandle, list, long ) message ExecuteRequest { uint32 statement_id = 1; repeated TypedValue parameter_values = 2; uint64 offset = 3; } Then I proceeded with the, $ cd avatica $ ./src/main/scripts/generate-protobuf.sh However, Eclipse is unable to compile, then I proceed to inspected the generated codes but found out that there are a lot differences as compared to master, example as below, - private ExecuteResponse(com.google.protobuf.GeneratedMessage.Builder builder) { + private ExecuteResponse(com.google.protobuf.GeneratedMessage.Builder<?> builder) { super (builder); } private ExecuteResponse() { @@ -1090,8 +1102,7 @@ private ExecuteResponse() { } private ExecuteResponse( com.google.protobuf.CodedInputStream input, - com.google.protobuf.ExtensionRegistryLite extensionRegistry) - throws com.google.protobuf.InvalidProtocolBufferException { + com.google.protobuf.ExtensionRegistryLite extensionRegistry) { this (); int mutable_bitField0_ = 0; try { public static org.apache.calcite.avatica.proto.Responses.ResultSetResponse getDefaultInstance() { - return defaultInstance; + return DEFAULT_INSTANCE; + } Especially on the exceptions that most should be throwing the RuntimeException Below is my protoc version, root@sherpa:/opt/java# cd ~ root@sherpa:~# which protoc /usr/local/bin/protoc root@sherpa:~# /usr/local/bin/protoc --version libprotoc 3.0.0 root@sherpa:~# Did I miss something anywhere? Thanks!
        Hide
        elserj Josh Elser added a comment -

        YeongWei, which release of protobuf did you actually install? Perhaps you ran into CALCITE-908. If you didn't, can you try to install 3.0.0-alpha-2 and see if that works?

        Show
        elserj Josh Elser added a comment - YeongWei , which release of protobuf did you actually install? Perhaps you ran into CALCITE-908 . If you didn't, can you try to install 3.0.0-alpha-2 and see if that works?
        Hide
        yeongwei YeongWei added a comment -

        Hi Josh Elser

        I installed the protobuf-3.0.0-beta-1 that gave the reported problem. With protobuf-3.0.0-alpha-2, it resolved the compilation problems.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Josh Elser I installed the protobuf-3.0.0-beta-1 that gave the reported problem. With protobuf-3.0.0-alpha-2, it resolved the compilation problems. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        Is this patch ready to be tested? Let me know.

        Show
        julianhyde Julian Hyde added a comment - Is this patch ready to be tested? Let me know.
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        I am still working on this. Currently fixing according to a few failed test cases.

        Will keep you posted.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde I am still working on this. Currently fixing according to a few failed test cases. Will keep you posted. Thanks!
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        I have rebased onto master, please refer to the commit below,

        https://github.com/yeongwei/incubator-calcite/commit/1dce44ad24301e4dac16e1f248c499ffbd8ff445

        Additionally I have created a pull request,

        https://github.com/apache/incubator-calcite/pull/145

        1 item that may require you attention, please refer to AbstractCursor#BinaryAccessor, during the serialization with PROTOBUF, upon invoking the resultSet.getBytes the underlying object is already of type byte[], upon casting it to ByteString gives error, I have modified as below,

            public byte[] getBytes() {
              Object obj = getObject();
              try {
                final ByteString o = (ByteString) obj;
                return o == null ? null : o.getBytes();
              } catch (Exception ex) {
                return obj == null ? null : (byte[]) obj;
              }
            }
        

        Previously, on JDBC the finagle method did the job of handling the types properly. Let me know what do you think.

        This WIP has passed the "mvn test" and "mvn site".

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde I have rebased onto master, please refer to the commit below, https://github.com/yeongwei/incubator-calcite/commit/1dce44ad24301e4dac16e1f248c499ffbd8ff445 Additionally I have created a pull request, https://github.com/apache/incubator-calcite/pull/145 1 item that may require you attention, please refer to AbstractCursor#BinaryAccessor, during the serialization with PROTOBUF, upon invoking the resultSet.getBytes the underlying object is already of type byte[], upon casting it to ByteString gives error, I have modified as below, public byte [] getBytes() { Object obj = getObject(); try { final ByteString o = (ByteString) obj; return o == null ? null : o.getBytes(); } catch (Exception ex) { return obj == null ? null : ( byte []) obj; } } Previously, on JDBC the finagle method did the job of handling the types properly. Let me know what do you think. This WIP has passed the "mvn test" and "mvn site". Thanks!
        Hide
        elserj Josh Elser added a comment -

        Previously, on JDBC the finagle method did the job of handling the types properly. Let me know what do you think.

        Hrm, I don't remember with certainty now, but I'm guessing I didn't do this in finagle in hopes to prevent yet-another-copy of a byte array (although doing this translation in the `finagle`s would be a more sensible place to do this). Location aside, your change looks reasonable. Could potentially do an instanceof check instead of making two assumptions as to the "type" of bytes (byte[] or ByteString).

        Show
        elserj Josh Elser added a comment - Previously, on JDBC the finagle method did the job of handling the types properly. Let me know what do you think. Hrm, I don't remember with certainty now, but I'm guessing I didn't do this in finagle in hopes to prevent yet-another-copy of a byte array (although doing this translation in the `finagle`s would be a more sensible place to do this). Location aside, your change looks reasonable. Could potentially do an instanceof check instead of making two assumptions as to the "type" of bytes ( byte[] or ByteString ).
        Hide
        julianhyde Julian Hyde added a comment -

        Avatica's standard for binary data is ByteString. It is preferred over byte[] because it is immutable and therefore does not need to be defensively copied. I suppose you could create ByteArrayAccessor, which would expect its input to be a byte[], but I think it would simplify things if we stuck to just ByteString throughout in Avatica.

        Note that the getBytes method is part of the JDBC public API and MUST do a defensive copy regardless of whether the source is byte[] or ByteString.

        Show
        julianhyde Julian Hyde added a comment - Avatica's standard for binary data is ByteString. It is preferred over byte[] because it is immutable and therefore does not need to be defensively copied. I suppose you could create ByteArrayAccessor, which would expect its input to be a byte[], but I think it would simplify things if we stuck to just ByteString throughout in Avatica. Note that the getBytes method is part of the JDBC public API and MUST do a defensive copy regardless of whether the source is byte[] or ByteString.
        Hide
        julianhyde Julian Hyde added a comment -

        I reviewed this, rebased and did some cosmetic cleanups. See https://github.com/julianhyde/incubator-calcite/tree/705-execute.

        Looks good, and all tests pass. 3 small remaining issues:

        • why is JdbcFrontLinqBackTest.testInsert disabled?
        • why is JdbcFrontLinqBackTest.testInsert2 disabled?
        • Fine to disable JsonHandlerTest.testFetchRequestWithNumberParameter
          but you need to add testExecuteRequestWithNumberParameter.

        I'll let Josh Elser decide how to resolve the byte[] vs ByteString issue, and I think it can after this change is committed.

        Show
        julianhyde Julian Hyde added a comment - I reviewed this, rebased and did some cosmetic cleanups. See https://github.com/julianhyde/incubator-calcite/tree/705-execute . Looks good, and all tests pass. 3 small remaining issues: why is JdbcFrontLinqBackTest.testInsert disabled? why is JdbcFrontLinqBackTest.testInsert2 disabled? Fine to disable JsonHandlerTest.testFetchRequestWithNumberParameter but you need to add testExecuteRequestWithNumberParameter. I'll let Josh Elser decide how to resolve the byte[] vs ByteString issue, and I think it can after this change is committed.
        Hide
        elserj Josh Elser added a comment -

        I'll let Josh Elser decide how to resolve the byte[] vs ByteString issue, and I think it can after this change is committed.

        ACK'ed and agree.

        Show
        elserj Josh Elser added a comment - I'll let Josh Elser decide how to resolve the byte[] vs ByteString issue, and I think it can after this change is committed. ACK'ed and agree.
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        Please refer below about the remaining items,

        1. why is JdbcFrontLinqBackTest.testInsert disabled?
        2. why is JdbcFrontLinqBackTest.testInsert2 disabled?

        Both 1 and 2, currently the CalciteAssert uses the executeQuery, but for DML this will cause the exception of "Expecting Result Set" to be thrown, since the DML no longer returns the value of "ROWCOUNT=<INT>".

        May I suggest that to fix the CalciteAssert via a separate JIRA? As the fix could impact a lot more unit test.

        3. Fine to disable JsonHandlerTest.testFetchRequestWithNumberParameter
        but you need to add testExecuteRequestWithNumberParameter

        Let me look into this and submit the Unit Test

        Let me know what you think.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde Please refer below about the remaining items, 1. why is JdbcFrontLinqBackTest.testInsert disabled? 2. why is JdbcFrontLinqBackTest.testInsert2 disabled? Both 1 and 2, currently the CalciteAssert uses the executeQuery, but for DML this will cause the exception of "Expecting Result Set" to be thrown, since the DML no longer returns the value of "ROWCOUNT=<INT>". May I suggest that to fix the CalciteAssert via a separate JIRA? As the fix could impact a lot more unit test. 3. Fine to disable JsonHandlerTest.testFetchRequestWithNumberParameter but you need to add testExecuteRequestWithNumberParameter Let me look into this and submit the Unit Test Let me know what you think. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        YeongWei, I've fixed 1+2 on my branch. When you have written testExecuteRequestWithNumberParameter we can commit.

        Show
        julianhyde Julian Hyde added a comment - YeongWei , I've fixed 1+2 on my branch. When you have written testExecuteRequestWithNumberParameter we can commit.
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        The testExecuteRequestWithNumberParameter commit can be found here https://github.com/yeongwei/incubator-calcite/commit/f7f393c22797f311c0361266c1616672f9d265e7?diff=split.

        I have also created a pull request at https://github.com/apache/incubator-calcite/pull/154.

        Let me know what you think.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde The testExecuteRequestWithNumberParameter commit can be found here https://github.com/yeongwei/incubator-calcite/commit/f7f393c22797f311c0361266c1616672f9d265e7?diff=split . I have also created a pull request at https://github.com/apache/incubator-calcite/pull/154 . Let me know what you think. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/cee8e844. YeongWei, Thank you for the patches and months of persistence to get this completed!

        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/cee8e844 . YeongWei , Thank you for the patches and months of persistence to get this completed!
        Hide
        yeongwei YeongWei added a comment -

        Hi Julian Hyde

        Appreciate with you guidance and patience.

        Thanks!

        Show
        yeongwei YeongWei added a comment - Hi Julian Hyde Appreciate with you guidance and patience. Thanks!
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.5.0 (2015-11-10)

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.5.0 (2015-11-10)

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            yeongwei YeongWei
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development