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

AvaticaStatement execute method broken over remote JDBC

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.0-incubating
    • Fix Version/s: 1.2.0-incubating
    • Component/s: None
    • Labels:

      Description

      1) Tested with Java source code.
      2) Tested with Apache Squirrel.

      On AvaticaStatement.execute routine, it is not calling the prepareAndExecuteInternal instead it is calling the executeQueryInternal which misses the remote execution call.

        Activity

        Hide
        xhoong Xavier FH Leong added a comment -

        This is also fix in CALCITE-636, but it's a copy and pasted of executeQuery, should instead call executeQuery direct and check the resultSet to return BOOLEAN, to follow DRY

        Show
        xhoong Xavier FH Leong added a comment - This is also fix in CALCITE-636 , but it's a copy and pasted of executeQuery, should instead call executeQuery direct and check the resultSet to return BOOLEAN, to follow DRY
        Hide
        yeongwei YeongWei added a comment -

        Added test into CalciteRemoteDriverTest.java.

        Show
        yeongwei YeongWei added a comment - Added test into CalciteRemoteDriverTest.java.
        Hide
        julianhyde Julian Hyde added a comment -

        Doesn't execute need to handle other statements besides queries, including those that do not return result sets? E.g. INSERT and CREATE TABLE.

        Show
        julianhyde Julian Hyde added a comment - Doesn't execute need to handle other statements besides queries, including those that do not return result sets? E.g. INSERT and CREATE TABLE.
        Hide
        yeongwei YeongWei added a comment -

        1) Execute() will need to know the statement types (E.g. INSERT, UPDATE, DELETE, CREATE, etc) to determine if it should fire the executeQuery() or executeUpdate(). Therefore the execute() method should be a higher abstraction compared to executeQuery() and executeUpdate(). Looking at the commits from CALCITE-636, the AvaticaStatement.execute does not seems to be doing such statement type check and it relies on the ResultSet#isClosed to return the valid boolean result.
        2) Currently exploring the SqlNode#getKind to evaluate if it is sufficient enough inform the statement type. Which may be obtained during the CalciteConnectionImpl#parseQuery routine.

        Any comments ?

        Show
        yeongwei YeongWei added a comment - 1) Execute() will need to know the statement types (E.g. INSERT, UPDATE, DELETE, CREATE, etc) to determine if it should fire the executeQuery() or executeUpdate(). Therefore the execute() method should be a higher abstraction compared to executeQuery() and executeUpdate(). Looking at the commits from CALCITE-636 , the AvaticaStatement.execute does not seems to be doing such statement type check and it relies on the ResultSet#isClosed to return the valid boolean result. 2) Currently exploring the SqlNode#getKind to evaluate if it is sufficient enough inform the statement type. Which may be obtained during the CalciteConnectionImpl#parseQuery routine. Any comments ?
        Hide
        julianhyde Julian Hyde added a comment -

        Are you maybe running into a limitation of the JDBC driver you are talking to? I don't see an inherent problem with Avatica just saying "execute this statement, i don't know what type it is, but I want it to return a row count".

        (2) is only possible in an environment which has a SQL parser, i.e. core Calcite, but not Avatica. That's why I don't want to go there.

        Show
        julianhyde Julian Hyde added a comment - Are you maybe running into a limitation of the JDBC driver you are talking to? I don't see an inherent problem with Avatica just saying "execute this statement, i don't know what type it is, but I want it to return a row count". (2) is only possible in an environment which has a SQL parser, i.e. core Calcite, but not Avatica. That's why I don't want to go there.
        Hide
        julianhyde Julian Hyde added a comment -

        Does your patch solve the problem for SELECT statements?

        If so, let's commit this, and can you open another jira case for executing DML, DDL, with and without parameters. That case will need tests in RemoteDriverTest and optionally also in CalciteRemoteDriverTest.

        Show
        julianhyde Julian Hyde added a comment - Does your patch solve the problem for SELECT statements? If so, let's commit this, and can you open another jira case for executing DML, DDL, with and without parameters. That case will need tests in RemoteDriverTest and optionally also in CalciteRemoteDriverTest.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/0d80fd25 .
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.2.0-incubating (2015-04-16)

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.2.0-incubating (2015-04-16)

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development