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

JdbcResultSet returns incomplete Frame with "default" statement ID

    Details

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

      Description

      I noticed this case while playing around with sqlline, phoenix queryserver and avatica.

      When sqlline starts up, it, among other things, fetches the columns to do some autocompletion magic in its shell. I noticed that this was always resulting in an error in Avatica's code, saying that the statement id wasn't found (the "fake" one -1)

      I traced through this and believe there's a problem with how JdbcResultSet computes the maximum size of results put into a Frame. For the calls which use this fake statement id (getColumns, getCatalogs, getTableTypes, etc), a result set returned by the wrapped JDBC driver which is larger than 100 results will cause a Frame to be sent back to the client which informs it to fetch another frame (via the done member).

      Concretely, if there are more than 100 columns to return in getColumns, the client will receive a Frame with done=false and 100 rows which will cause it to try to fetch another Frame of results. This will fail because we gave a fake statement ID which causes an exception in the server.

      As I see it, there are two solutions:

      1. Create and cache a statement on these calls so that the fetch logic works as intended (the good solution)
      2. Send all of the results back in one frame (the quick hacky thing I just tested).

        Activity

        Hide
        elserj Josh Elser added a comment -

        Happy to submit a patch for this. Looking for some confirmation that I'm approaching this from the correct angle

        Show
        elserj Josh Elser added a comment - Happy to submit a patch for this. Looking for some confirmation that I'm approaching this from the correct angle
        Hide
        julianhyde Julian Hyde added a comment -

        Yes, 1 is the right solution. The -1 doesn't mean "there is no statement" it means "please create a statement, because I'm trying to do this in one round-trip". It is an artifact of how the JDBC API works. It makes sense to have a statement on the server, so that you can (say) kill the statement if it runs too long. It even makes sense to have a statement on the client - and the client can get it from the result set, by calling say

        connection.getMetaData().getTables(null, null, null, null).getStatement()

        . The statement is closed implicitly when the result set is closed.

        Show
        julianhyde Julian Hyde added a comment - Yes, 1 is the right solution. The -1 doesn't mean "there is no statement" it means "please create a statement, because I'm trying to do this in one round-trip". It is an artifact of how the JDBC API works. It makes sense to have a statement on the server, so that you can (say) kill the statement if it runs too long. It even makes sense to have a statement on the client - and the client can get it from the result set, by calling say connection.getMetaData().getTables( null , null , null , null ).getStatement() . The statement is closed implicitly when the result set is closed.
        Hide
        elserj Josh Elser added a comment -

        The -1 doesn't mean "there is no statement" it means "please create a statement, because I'm trying to do this in one round-trip". It is an artifact of how the JDBC API works.

        Ahh, I see now! Thanks for the pointer.

        Show
        elserj Josh Elser added a comment - The -1 doesn't mean "there is no statement" it means "please create a statement, because I'm trying to do this in one round-trip". It is an artifact of how the JDBC API works. Ahh, I see now! Thanks for the pointer.
        Hide
        bruno Bruno Dumon added a comment -

        I attached a patch for this on CALCITE-912 (because it works on top of the changes I propose over there).

        Show
        bruno Bruno Dumon added a comment - I attached a patch for this on CALCITE-912 (because it works on top of the changes I propose over there).
        Hide
        elserj Josh Elser added a comment - - edited

        How about we just assign this issue to you then, Bruno Dumon. You've beat me to the punch

        edit: Hrm, it seems like you must not have the right JIRA karma to get assigned issues. Perhaps Julian Hyde would rectify that for you.

        Show
        elserj Josh Elser added a comment - - edited How about we just assign this issue to you then, Bruno Dumon . You've beat me to the punch edit: Hrm, it seems like you must not have the right JIRA karma to get assigned issues. Perhaps Julian Hyde would rectify that for you.
        Hide
        elserj Josh Elser added a comment -

        Bruno's Patch which applies on top of CALCITE-912

        Show
        elserj Josh Elser added a comment - Bruno's Patch which applies on top of CALCITE-912
        Hide
        julianhyde Julian Hyde added a comment -

        Bruno Dumon, I've given you contributor privileges and assigned the issue to you.

        Can you please start using pull requests rather than patches? Then you won't need to say "this is based on ...". Also, it will be easier for others to rebase if a contribution is under development for a long time.

        Show
        julianhyde Julian Hyde added a comment - Bruno Dumon , I've given you contributor privileges and assigned the issue to you. Can you please start using pull requests rather than patches? Then you won't need to say "this is based on ...". Also, it will be easier for others to rebase if a contribution is under development for a long time.
        Hide
        julianhyde Julian Hyde added a comment - - edited

        Bruno Dumon It seems that CALCITE-912 is close to ready but you have not updated the patch for CALCITE-871 attached to that issue. Can you add a commit to https://github.com/apache/incubator-calcite/pull/150. You still need to address 'new Random().nextInt()'.

        Show
        julianhyde Julian Hyde added a comment - - edited Bruno Dumon It seems that CALCITE-912 is close to ready but you have not updated the patch for CALCITE-871 attached to that issue. Can you add a commit to https://github.com/apache/incubator-calcite/pull/150 . You still need to address 'new Random().nextInt()'.
        Hide
        bruno Bruno Dumon added a comment -

        Julian Hyde, I was going to make a separate pull request for that once CALCITE-912 got in. I'll replace the Random usage with the new JdbcMeta.statementIdGenerator.

        Show
        bruno Bruno Dumon added a comment - Julian Hyde , I was going to make a separate pull request for that once CALCITE-912 got in. I'll replace the Random usage with the new JdbcMeta.statementIdGenerator.
        Hide
        elserj Josh Elser added a comment -

        I'll replace the Random usage with the new JdbcMeta.statementIdGenerator.

        FYI, I saw your patch no longer applied so I took the liberty of applying the changes by hand (took ~30s). I'll include it in a pull-request if that's ok with you.

        Show
        elserj Josh Elser added a comment - I'll replace the Random usage with the new JdbcMeta.statementIdGenerator. FYI, I saw your patch no longer applied so I took the liberty of applying the changes by hand (took ~30s). I'll include it in a pull-request if that's ok with you.
        Hide
        bruno Bruno Dumon added a comment -

        Josh Elser, yes of course, go ahead.

        Show
        bruno Bruno Dumon added a comment - Josh Elser , yes of course, go ahead.
        Hide
        julianhyde Julian Hyde added a comment -

        Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/00517e7a. Thanks for the patch, Bruno Dumon, and thanks for re-basing it, Josh Elser!

        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/00517e7a . Thanks for the patch, Bruno Dumon , and thanks for re-basing it, Josh Elser !
        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:
            bruno Bruno Dumon
            Reporter:
            elserj Josh Elser
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development