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

Avatica can't serialize java.sql.Array

    Details

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

      Description

      As far as I can see, there is no way to serialize arrays in the Avatica RPC.

        Issue Links

          Activity

          Hide
          elserj Josh Elser added a comment -

          This is done by Frame for JSON and ColumnValue for Protobufs. For JSON, each value in the row attribute of a frame might be an array instead of a scalar value.

          I have noticed in CALCITE-1040, however, that the parsing of scalar or array values is busted for arrays of length 1 with protobuf. Apparently I never wrote up a test for this. I'm currently working on adding an array_value and scalar_value to ColumnValue for protobuf to handle this conversion correctly.

          Does this make sense to you?

          Show
          elserj Josh Elser added a comment - This is done by Frame for JSON and ColumnValue for Protobufs. For JSON, each value in the row attribute of a frame might be an array instead of a scalar value. I have noticed in CALCITE-1040 , however, that the parsing of scalar or array values is busted for arrays of length 1 with protobuf. Apparently I never wrote up a test for this. I'm currently working on adding an array_value and scalar_value to ColumnValue for protobuf to handle this conversion correctly. Does this make sense to you?
          Hide
          lukaslalinsky Lukas Lalinsky added a comment -

          Unfortunately, it doesn't. In order to submit ExecuteRequest, I need to get a list of TypedValue and I don't see anything in the code that would be parsing or serializing arrays. Do you have an example how to send an array to TypedValue in JSON?

          I can't really use the protobuf interface, because it's using the beta version of proto3, which would make my code very hard to install.

          Show
          lukaslalinsky Lukas Lalinsky added a comment - Unfortunately, it doesn't. In order to submit ExecuteRequest , I need to get a list of TypedValue and I don't see anything in the code that would be parsing or serializing arrays. Do you have an example how to send an array to TypedValue in JSON? I can't really use the protobuf interface, because it's using the beta version of proto3, which would make my code very hard to install.
          Hide
          elserj Josh Elser added a comment -

          In order to submit ExecuteRequest, I need to get a list of TypedValue

          So, you're saying that you cannot execute a prepared query with an array substituted in? I'd like to make sure I understand exactly what you're trying to do before digging into this. The more clarification you can provide on what exactly you are trying to do would be extremely helpful.

          I can't really use the protobuf interface, because it's using the beta version of proto3, which would make my code very hard to install.

          Sorry to hear that. There are published tarballs for the protobuf releases, but I guess you're lamenting packages provided by your operating system? Even so, you could build the python libs and include them in your python module, no? It seems like something that could be solved in the packaging of your phoenixdb project. Am I misrepresenting this problem? Protobuf is the way of the future as far as Avatica is concerned. I'd like to make sure the path forward isn't unnecessarily difficult.

          Show
          elserj Josh Elser added a comment - In order to submit ExecuteRequest, I need to get a list of TypedValue So, you're saying that you cannot execute a prepared query with an array substituted in? I'd like to make sure I understand exactly what you're trying to do before digging into this. The more clarification you can provide on what exactly you are trying to do would be extremely helpful. I can't really use the protobuf interface, because it's using the beta version of proto3, which would make my code very hard to install. Sorry to hear that. There are published tarballs for the protobuf releases, but I guess you're lamenting packages provided by your operating system? Even so, you could build the python libs and include them in your python module, no? It seems like something that could be solved in the packaging of your phoenixdb project. Am I misrepresenting this problem? Protobuf is the way of the future as far as Avatica is concerned. I'd like to make sure the path forward isn't unnecessarily difficult.
          Hide
          lukaslalinsky Lukas Lalinsky added a comment - - edited

          Let's say I have a prepared statement, created like this:

          {
            "request": "prepare",
            "connectionId": "...",
            "sql": "UPSERT INTO foo (id, array_column) VALUES (?, ?)",
            "maxRowCount": 0
          }
          

          Now I need to execute the statement like this:

          {
            "request": "execute",
            "statementHandle": { ... },
            "parameterValues": [
              {"type": "INTEGER", "value": "1"},
              {"type": "???", "value": ???}
            ],
            "maxRowCount": 0
          }
          

          I don't know what to put instead of ??? for the array parameter to work. ColumnMetaData.Rep does have an ARRAY type, but it's either not used in TypedValue or I'm submitting the data in the wrong format. Initially I expected "type": "ARRAY", "value": [1,2,3] to work, but it doesn't.

          Show
          lukaslalinsky Lukas Lalinsky added a comment - - edited Let's say I have a prepared statement, created like this: { "request" : "prepare" , "connectionId" : "..." , "sql" : "UPSERT INTO foo (id, array_column) VALUES (?, ?)" , "maxRowCount" : 0 } Now I need to execute the statement like this: { "request" : "execute" , "statementHandle" : { ... }, "parameterValues" : [ { "type" : "INTEGER" , "value" : "1" }, { "type" : "???" , "value" : ???} ], "maxRowCount" : 0 } I don't know what to put instead of ??? for the array parameter to work. ColumnMetaData.Rep does have an ARRAY type, but it's either not used in TypedValue or I'm submitting the data in the wrong format. Initially I expected "type": "ARRAY", "value": [1,2,3] to work, but it doesn't.
          Hide
          elserj Josh Elser added a comment -

          Thanks for writing this up Lukas Lalinsky. This is a great example. I see what you mean now.

          On the "query results" side of things, we have Frame which which contains a List<Object> of rows. For an array, the element in the rows would be a List itself, which triggers it. I'm guessing that it was just missing on this side.

          Let me start by writing some tests to work through this. Thanks again for the bug report.

          Show
          elserj Josh Elser added a comment - Thanks for writing this up Lukas Lalinsky . This is a great example. I see what you mean now. On the "query results" side of things, we have Frame which which contains a List<Object> of rows . For an array, the element in the rows would be a List itself, which triggers it. I'm guessing that it was just missing on this side. Let me start by writing some tests to work through this. Thanks again for the bug report.
          Hide
          elserj Josh Elser added a comment -

          Ok, this is most assuredly missing the implementation. Calling setArray on the AvaticaPrepareStatement, ends up calling AvaticaSite#setArray:

            public void setArray(Array x) {
            }
          

          I need to give this some thought about how to implement, but I'm a bit scared about trying to tackle this so close to the 1.6.0 release. I may punt this to 1.7.0 since it would require a bit of code-change and testing.

          Show
          elserj Josh Elser added a comment - Ok, this is most assuredly missing the implementation. Calling setArray on the AvaticaPrepareStatement, ends up calling AvaticaSite#setArray : public void setArray(Array x) { } I need to give this some thought about how to implement, but I'm a bit scared about trying to tackle this so close to the 1.6.0 release. I may punt this to 1.7.0 since it would require a bit of code-change and testing.
          Hide
          jamestaylor James Taylor added a comment -

          Josh Elser - is this one still on your radar? Over in Phoenix/Calcite integration land, this one would help a lot.

          Show
          jamestaylor James Taylor added a comment - Josh Elser - is this one still on your radar? Over in Phoenix/Calcite integration land, this one would help a lot.
          Hide
          elserj Josh Elser added a comment -

          Yes, it is James Taylor. Sorry I haven't gotten to finishing it yet..

          Show
          elserj Josh Elser added a comment - Yes, it is James Taylor . Sorry I haven't gotten to finishing it yet..
          Hide
          elserj Josh Elser added a comment - - edited

          Just wanted to drop a line: I've had some time to work on this yesterday and today. I think I'm close.

          /me grins

          0: jdbc:avatica:remote:url=http://hw10447.loc> select * from arr;
          +-------+------------+-------------------------+
          | name  |  int_arr   |        schedule         |
          +-------+------------+-------------------------+
          | josh  | [1, 2, 3]  | [[a, b, c], [b, c, d]]  |
          +-------+------------+-------------------------+
          
          Show
          elserj Josh Elser added a comment - - edited Just wanted to drop a line: I've had some time to work on this yesterday and today. I think I'm close. /me grins 0: jdbc:avatica:remote:url=http://hw10447.loc> select * from arr; +-------+------------+-------------------------+ | name | int_arr | schedule | +-------+------------+-------------------------+ | josh | [1, 2, 3] | [[a, b, c], [b, c, d]] | +-------+------------+-------------------------+
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user joshelser opened a pull request:

          https://github.com/apache/calcite-avatica/pull/2

          CALCITE-1050 Array support

          Tested against HSQLDB, Postgres, and Apache Phoenix

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/joshelser/calcite-avatica 1050-arrays

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/calcite-avatica/pull/2.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #2


          commit 34d1016ea23492abb1e879cd3f560741e69338bc
          Author: Josh Elser <elserj@apache.org>
          Date: 2016-01-12T21:40:18Z

          CALCITE-1050 Array support for Avatica

          As best as possible, works around the limitations of JDBC's
          Array class to handle arbitrarily nested Arrays. Nested-array
          support differs from DB to DB, so functionality is primarily
          driven from structure, rather than metadata.

          commit 1097142106478605741dff77063a59bbd5f50909
          Author: Josh Elser <elserj@apache.org>
          Date: 2017-03-30T20:34:33Z

          Further cleanup

          • Remove dead code
          • Remove Compiler warnings
          • Clarify some comments

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user joshelser opened a pull request: https://github.com/apache/calcite-avatica/pull/2 CALCITE-1050 Array support Tested against HSQLDB, Postgres, and Apache Phoenix You can merge this pull request into a Git repository by running: $ git pull https://github.com/joshelser/calcite-avatica 1050-arrays Alternatively you can review and apply these changes as the patch at: https://github.com/apache/calcite-avatica/pull/2.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2 commit 34d1016ea23492abb1e879cd3f560741e69338bc Author: Josh Elser <elserj@apache.org> Date: 2016-01-12T21:40:18Z CALCITE-1050 Array support for Avatica As best as possible, works around the limitations of JDBC's Array class to handle arbitrarily nested Arrays. Nested-array support differs from DB to DB, so functionality is primarily driven from structure, rather than metadata. commit 1097142106478605741dff77063a59bbd5f50909 Author: Josh Elser <elserj@apache.org> Date: 2017-03-30T20:34:33Z Further cleanup Remove dead code Remove Compiler warnings Clarify some comments
          Hide
          elserj Josh Elser added a comment -

          https://github.com/apache/calcite-avatica/pull/2 includes some basic array support. I have tested this one against HSQLDB (functionally and via unit tests), Postgresql (notably for its support of multi-dimensional array support), and Apache Phoenix (as there was interest/need here).

          James Taylor, Rajeshbabu Chintaguntla, can either of you put this through some paces with the Calcite-Phoenix integration? I've just done some rudimentary tests with Phoenix so far.

          Julian Hyde, another good one if you have some time to devote to a review

          Show
          elserj Josh Elser added a comment - https://github.com/apache/calcite-avatica/pull/2 includes some basic array support. I have tested this one against HSQLDB (functionally and via unit tests), Postgresql (notably for its support of multi-dimensional array support), and Apache Phoenix (as there was interest/need here). James Taylor , Rajeshbabu Chintaguntla , can either of you put this through some paces with the Calcite-Phoenix integration? I've just done some rudimentary tests with Phoenix so far. Julian Hyde , another good one if you have some time to devote to a review
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Josh Elser Thank you very much for your work. Sure will take a look at it.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Josh Elser Thank you very much for your work. Sure will take a look at it.
          Hide
          jamestaylor James Taylor added a comment -

          Maryann Xue - would you have a few spare cycles to review this patch?

          Show
          jamestaylor James Taylor added a comment - Maryann Xue - would you have a few spare cycles to review this patch?
          Hide
          elserj Josh Elser added a comment -

          If it doesn't go without saying, do let me know if there's anything I can do to help make this review from the Phoenix side easier.

          I've just done simple testing via the Phoenix Query Server, but I could push a SNAPSHOT build of Avatica and Calcite up to repository.a.o which you could switch the phoenix-calcite branch to use.

          Show
          elserj Josh Elser added a comment - If it doesn't go without saying, do let me know if there's anything I can do to help make this review from the Phoenix side easier. I've just done simple testing via the Phoenix Query Server, but I could push a SNAPSHOT build of Avatica and Calcite up to repository.a.o which you could switch the phoenix-calcite branch to use.
          Hide
          elserj Josh Elser added a comment -

          Argh, just realized that I broke some stuff in Calcite. Looking into those...

          Show
          elserj Josh Elser added a comment - Argh, just realized that I broke some stuff in Calcite. Looking into those...
          Hide
          elserj Josh Elser added a comment -

          Argh, just realized that I broke some stuff in Calcite. Looking into those...

          Pushed an update to the PR which at least gets a Calcite mvn verify passing again.

          Show
          elserj Josh Elser added a comment - Argh, just realized that I broke some stuff in Calcite. Looking into those... Pushed an update to the PR which at least gets a Calcite mvn verify passing again.
          Hide
          maryannxue Maryann Xue added a comment -

          Thank you very much for doing the Array support, Josh Elser! One important thing though, can you please add the interface level support as well, such as AvaticaConnection.createArrayOf?

          Show
          maryannxue Maryann Xue added a comment - Thank you very much for doing the Array support, Josh Elser ! One important thing though, can you please add the interface level support as well, such as AvaticaConnection.createArrayOf ?
          Hide
          elserj Josh Elser added a comment -

          One important thing though, can you please add the interface level support as well, such as AvaticaConnection.createArrayOf?

          Sure thing, I can look at implementing that method! Any others on Connection?

          What about the other methods on Array? Only some of the getArray(..) and getResultSet(..) methods are implemented currently. It's been enough to at least run against sqlline with a couple of databases. Just curious if you know of any specifically that are higher priority to implement (also not sure how much work each will be).

          Show
          elserj Josh Elser added a comment - One important thing though, can you please add the interface level support as well, such as AvaticaConnection.createArrayOf? Sure thing, I can look at implementing that method! Any others on Connection? What about the other methods on Array ? Only some of the getArray(..) and getResultSet(..) methods are implemented currently. It's been enough to at least run against sqlline with a couple of databases. Just curious if you know of any specifically that are higher priority to implement (also not sure how much work each will be).
          Hide
          maryannxue Maryann Xue added a comment - - edited

          Not sure, but I think other than createArrayOf, setArray, and getArray are enough for most test cases in Phoenix. And they have already been implemented in the PR, thanks! One thing is that ResultSet.getString is also used on array fields in Phoenix tests to get the "toString" value of an array, I am not sure if it's SQL standard or if Avatica supports it that way. James Taylor, do you have any comment on this point?

          Another problem I found by running the Phoenix array tests was that literal string array values were taken as CHAR ARRAY type and could not be inserted into VARCHAR ARRAY columns. I think this is a Calcite issue and I'll open a separate JIRA.

          Show
          maryannxue Maryann Xue added a comment - - edited Not sure, but I think other than createArrayOf , setArray , and getArray are enough for most test cases in Phoenix. And they have already been implemented in the PR, thanks! One thing is that ResultSet.getString is also used on array fields in Phoenix tests to get the "toString" value of an array, I am not sure if it's SQL standard or if Avatica supports it that way. James Taylor , do you have any comment on this point? Another problem I found by running the Phoenix array tests was that literal string array values were taken as CHAR ARRAY type and could not be inserted into VARCHAR ARRAY columns. I think this is a Calcite issue and I'll open a separate JIRA.
          Hide
          julianhyde Julian Hyde added a comment - - edited

          Thanks so much for this, Josh Elser!

          Review comments:

          • In jdbcToSerial it doesn't look as if it handles null Timestamp values.
          • In AvaticaUtils, the getNonPrimitiveRep and getSerializedRep methods can be moved to Rep, and maybe rationalized a bit.
          Show
          julianhyde Julian Hyde added a comment - - edited Thanks so much for this, Josh Elser ! Review comments: In jdbcToSerial it doesn't look as if it handles null Timestamp values. In AvaticaUtils , the getNonPrimitiveRep and getSerializedRep methods can be moved to Rep , and maybe rationalized a bit.
          Hide
          elserj Josh Elser added a comment -

          Not sure, but I think other than createArrayOf, setArray, and getArray are enough for most test cases in Phoenix. And they have already been implemented in the PR, thanks!

          Excellent . Thanks for digging in, Maryann.

          One thing is that ResultSet.getString is also used on array fields in Phoenix tests to get the "toString" value of an array, I am not sure if it's SQL standard or if Avatica supports it that way

          Yup, this is pretty normal. Essentially, just the same as you can get a String representation of a value of a column in the current row of a ResultSet, you can treat a ResultSet generated from an Array in the same manner. Makes the user-facing parsing a bit more consumable (thankfully, as the Array API leaves a bit to be desired).

          Another problem I found by running the Phoenix array tests was that literal string array values were taken as CHAR ARRAY type and could not be inserted into VARCHAR ARRAY columns. I think this is a Calcite issue and I'll open a separate JIRA.

          Ok, let me know if you find otherwise, or have a test case for it.

          Review comments:

          Thanks for looking, Julian. I need to write some test cases to catch the Calcite failures I fixed last night in Avatica (to prevent the "surprise, calcite's busted" situation). I'll look at these two suggestions as well!

          Show
          elserj Josh Elser added a comment - Not sure, but I think other than createArrayOf, setArray, and getArray are enough for most test cases in Phoenix. And they have already been implemented in the PR, thanks! Excellent . Thanks for digging in, Maryann. One thing is that ResultSet.getString is also used on array fields in Phoenix tests to get the "toString" value of an array, I am not sure if it's SQL standard or if Avatica supports it that way Yup, this is pretty normal. Essentially, just the same as you can get a String representation of a value of a column in the current row of a ResultSet, you can treat a ResultSet generated from an Array in the same manner. Makes the user-facing parsing a bit more consumable (thankfully, as the Array API leaves a bit to be desired). Another problem I found by running the Phoenix array tests was that literal string array values were taken as CHAR ARRAY type and could not be inserted into VARCHAR ARRAY columns. I think this is a Calcite issue and I'll open a separate JIRA. Ok, let me know if you find otherwise, or have a test case for it. Review comments: Thanks for looking, Julian. I need to write some test cases to catch the Calcite failures I fixed last night in Avatica (to prevent the "surprise, calcite's busted" situation). I'll look at these two suggestions as well!
          Hide
          elserj Josh Elser added a comment -

          Just pushed an update to the existing PR which, I believe, addresses the comments from Julian and Maryann (thanks so much, you two).

          Along the way, I also found CALCITE-1756, and fixed an existing bug with Array.getArray(long, int). I'd like to get this committed (as it's rather large) and then go into bug-fix mode with any subsequent issues.

          Show
          elserj Josh Elser added a comment - Just pushed an update to the existing PR which, I believe, addresses the comments from Julian and Maryann (thanks so much, you two). Along the way, I also found CALCITE-1756 , and fixed an existing bug with Array.getArray(long, int) . I'd like to get this committed (as it's rather large) and then go into bug-fix mode with any subsequent issues.
          Hide
          elserj Josh Elser added a comment -

          Looks like I broke some more stuff in Calcite due to interface changes . Will see if I can avoid forcing a Calcite code-change.

          Show
          elserj Josh Elser added a comment - Looks like I broke some more stuff in Calcite due to interface changes . Will see if I can avoid forcing a Calcite code-change.
          Hide
          elserj Josh Elser added a comment - - edited

          Will see if I can avoid forcing a Calcite code-change.

          While some of the API/interface changes I made in Avatica were simplifications (unused arguments in method signatures), I reverted the removals to make the upgrade to avatica 1.10.0 less painful in Calcite.

          There is one failing test out of the box (JdbcTest.testPreparedStatement) which expects a very specific exception message. Due to some unnecessary method-calls (as alluded to in the above sentence), an Exception is triggered in a different part of the code, and generates a different error message. I think we should just change the test itself in this case.

          Show
          elserj Josh Elser added a comment - - edited Will see if I can avoid forcing a Calcite code-change. While some of the API/interface changes I made in Avatica were simplifications (unused arguments in method signatures), I reverted the removals to make the upgrade to avatica 1.10.0 less painful in Calcite. There is one failing test out of the box ( JdbcTest.testPreparedStatement ) which expects a very specific exception message. Due to some unnecessary method-calls (as alluded to in the above sentence), an Exception is triggered in a different part of the code, and generates a different error message. I think we should just change the test itself in this case.
          Hide
          elserj Josh Elser added a comment -

          I've been poking at ArrayIT in Phoenix and it seems like Phoenix was making some assumptions that it would get a PhoenixArray as the bound parameter value whereas, with these changes, it's actually get an Java array (e.g. Object[]). This is a result of PhoenixCalciteConnection calling toJdbc(..) on TypedValue. Per the Javadoc (that predates me), the JDBC form of an ARRAY would be an Object[]. Thus, the RuntimeContextImpl generates Object[]'s for the Array's, and Phoenix later tries to cast the Object[] to an Array (granted, a PhoenixArray which wouldn't work at all – but an Array would also fail). So, I'm not sure there's much more we can glean from Phoenix to assert correctness here (requires more work in Phoenix).

          But, before I push this in, I am left wondering: Julian Hyde, is the JDBC form of an Array an Object[] correct? (for context: https://github.com/joshelser/calcite-avatica/blob/1050-arrays/core/src/main/java/org/apache/calcite/avatica/remote/TypedValue.java#L112-L113). The method signature of PreparedStatement.setArray(int, Array) as opposed to PreparedStatement.setArray(int, Object) (or some form of a Java array, Object or primitive).

          Show
          elserj Josh Elser added a comment - I've been poking at ArrayIT in Phoenix and it seems like Phoenix was making some assumptions that it would get a PhoenixArray as the bound parameter value whereas, with these changes, it's actually get an Java array (e.g. Object[] ). This is a result of PhoenixCalciteConnection calling toJdbc(..) on TypedValue . Per the Javadoc (that predates me), the JDBC form of an ARRAY would be an Object[] . Thus, the RuntimeContextImpl generates Object[] 's for the Array 's, and Phoenix later tries to cast the Object[] to an Array (granted, a PhoenixArray which wouldn't work at all – but an Array would also fail). So, I'm not sure there's much more we can glean from Phoenix to assert correctness here (requires more work in Phoenix). But, before I push this in, I am left wondering: Julian Hyde , is the JDBC form of an Array an Object[] correct? (for context: https://github.com/joshelser/calcite-avatica/blob/1050-arrays/core/src/main/java/org/apache/calcite/avatica/remote/TypedValue.java#L112-L113 ). The method signature of PreparedStatement.setArray(int, Array) as opposed to PreparedStatement.setArray(int, Object) (or some form of a Java array, Object or primitive).
          Hide
          jamestaylor James Taylor added a comment -

          A JDBC array is represented by the java.sql.Array interface (https://docs.oracle.com/javase/7/docs/api/java/sql/Array.html). It's entirely likely that Phoenix unit tests assume the backing implementation is a PhoenixArray.

          Show
          jamestaylor James Taylor added a comment - A JDBC array is represented by the java.sql.Array interface ( https://docs.oracle.com/javase/7/docs/api/java/sql/Array.html ). It's entirely likely that Phoenix unit tests assume the backing implementation is a PhoenixArray.
          Hide
          elserj Josh Elser added a comment -

          Yeah, agreed on the Array interface portion, James. I think I saw the code I was stepping through was in some PData type classes, but I'd have to pull it up again.

          My hunch here is that I've fallen into a typo in the docs, but maybe there's a reason this was documented as such . Granted, even if this was the case, we'd still be getting the ClassCastExceptions (trying to cast a Avatica's ArrayImpl into PhoenixArray, but that's for another day).

          Show
          elserj Josh Elser added a comment - Yeah, agreed on the Array interface portion, James. I think I saw the code I was stepping through was in some PData type classes, but I'd have to pull it up again. My hunch here is that I've fallen into a typo in the docs, but maybe there's a reason this was documented as such . Granted, even if this was the case, we'd still be getting the ClassCastExceptions (trying to cast a Avatica's ArrayImpl into PhoenixArray, but that's for another day).
          Hide
          julianhyde Julian Hyde added a comment -

          As far as I can tell, a JDBC driver is safe to assume that every Array it sees is one of its own. If a user creates class MyArrayImpl implements Array and passes it to PreparedStatement.setArray they will get an error.

          As a user, if I want to create an array, I must call Connection.createArrayOf.

          Show
          julianhyde Julian Hyde added a comment - As far as I can tell, a JDBC driver is safe to assume that every Array it sees is one of its own. If a user creates class MyArrayImpl implements Array and passes it to PreparedStatement.setArray they will get an error. As a user, if I want to create an array, I must call Connection.createArrayOf .
          Hide
          elserj Josh Elser added a comment -

          As far as I can tell, a JDBC driver is safe to assume that every Array it sees is one of its own. If a user creates class MyArrayImpl implements Array and passes it to PreparedStatement.setArray they will get an error.

          As a user, if I want to create an array, I must call Connection.createArrayOf.

          That makes sense to me. Something else to fix up down in Phoenix land.

          Should I make the leap to say that you agree with me that TypedValue is doing the wrong thing (by not creating an Array object for the JDBC form)?

          Show
          elserj Josh Elser added a comment - As far as I can tell, a JDBC driver is safe to assume that every Array it sees is one of its own. If a user creates class MyArrayImpl implements Array and passes it to PreparedStatement.setArray they will get an error. As a user, if I want to create an array, I must call Connection.createArrayOf. That makes sense to me. Something else to fix up down in Phoenix land. Should I make the leap to say that you agree with me that TypedValue is doing the wrong thing (by not creating an Array object for the JDBC form)?
          Hide
          julianhyde Julian Hyde added a comment -

          I don't think TypedValue is wrong. Its job is to send values over the wire, right? Array values aren't serializable, because they contain a connection. So I think you should serialize the values in the array (or map or struct or multiset) in something close to their native values then wrap them in your implementation of Array at the other end.

          I see you've added logic to AvaticaConnection.createArray to map type names to AvaticaType instances. At some point (not yet) we will want to convert that to a parser and caching map that can handle complex types and parameterized types such as "DECIMAL(10, 4)".

          Show
          julianhyde Julian Hyde added a comment - I don't think TypedValue is wrong. Its job is to send values over the wire, right? Array values aren't serializable, because they contain a connection. So I think you should serialize the values in the array (or map or struct or multiset) in something close to their native values then wrap them in your implementation of Array at the other end. I see you've added logic to AvaticaConnection.createArray to map type names to AvaticaType instances. At some point (not yet) we will want to convert that to a parser and caching map that can handle complex types and parameterized types such as "DECIMAL(10, 4)".
          Hide
          elserj Josh Elser added a comment -

          I don't think TypedValue is wrong. Its job is to send values over the wire, right? Array values aren't serializable, because they contain a connection. So I think you should serialize the values in the array (or map or struct or multiset) in something close to their native values then wrap them in your implementation of Array at the other end.

          Yes, I agree with you on the serial/local forms. I'm more trying to reconcile what the docs say the JDBC form is used for. Here's a snippet of code which I'm trying to use to reason about this:

            @Test public void testSetJavaArray() throws Exception {
              try (Connection conn = DriverManager.getConnection(url)) {
                Integer[] data = new Integer[] {1, 2, 3, 4, 5};
                final Array array = conn.createArrayOf("INTEGER", data);
                final TypedValue arrayTypedValue =
                    TypedValue.ofJdbc(Rep.ARRAY, array, Unsafe.localCalendar());
                final String tableName = "SET_JAVA_ARRAY";
                try (Statement stmt = conn.createStatement()) {
                  assertFalse(stmt.execute("DROP TABLE IF EXISTS "  + tableName));
                  assertFalse(stmt.execute("CREATE TABLE " + tableName + "(pk integer not null primary key,"
                      + " numbers integer array)"));
                }
                try (PreparedStatement pstmt = conn.prepareStatement("INSERT INTO " + tableName
                    + " values(?,?)")) {
                  pstmt.setInt(1, 1);
                  pstmt.setArray(2, (Array) arrayTypedValue.toJdbc(Unsafe.localCalendar()));
                  assertEquals(1, pstmt.executeUpdate());
                }
            }
          

          Given the javadoc, I'd expect the above to work. For a TypedValue, I should get an Object which is suitable to be set as an Array on the PreparedStatement. However, the above throws a ClassCastException (trying to cast Object[] to Array).

          Show
          elserj Josh Elser added a comment - I don't think TypedValue is wrong. Its job is to send values over the wire, right? Array values aren't serializable, because they contain a connection. So I think you should serialize the values in the array (or map or struct or multiset) in something close to their native values then wrap them in your implementation of Array at the other end. Yes, I agree with you on the serial/local forms. I'm more trying to reconcile what the docs say the JDBC form is used for. Here's a snippet of code which I'm trying to use to reason about this: @Test public void testSetJavaArray() throws Exception { try (Connection conn = DriverManager.getConnection(url)) { Integer [] data = new Integer [] {1, 2, 3, 4, 5}; final Array array = conn.createArrayOf( "INTEGER" , data); final TypedValue arrayTypedValue = TypedValue.ofJdbc(Rep.ARRAY, array, Unsafe.localCalendar()); final String tableName = "SET_JAVA_ARRAY" ; try (Statement stmt = conn.createStatement()) { assertFalse(stmt.execute( "DROP TABLE IF EXISTS " + tableName)); assertFalse(stmt.execute( "CREATE TABLE " + tableName + "(pk integer not null primary key," + " numbers integer array)" )); } try (PreparedStatement pstmt = conn.prepareStatement( "INSERT INTO " + tableName + " values(?,?)" )) { pstmt.setInt(1, 1); pstmt.setArray(2, (Array) arrayTypedValue.toJdbc(Unsafe.localCalendar())); assertEquals(1, pstmt.executeUpdate()); } } Given the javadoc, I'd expect the above to work. For a TypedValue, I should get an Object which is suitable to be set as an Array on the PreparedStatement. However, the above throws a ClassCastException (trying to cast Object[] to Array).
          Hide
          elserj Josh Elser added a comment -

          Given the javadoc, I'd expect the above to work. For a TypedValue, I should get an Object which is suitable to be set as an Array on the PreparedStatement. However, the above throws a ClassCastException (trying to cast Object[] to Array).

          Alright, I have convinced myself that the current javadoc on TypedValue is wrong. The reason we have not noticed this in the past is that HSQLDB will "coerce" Object[] (non-primitive type arrays) and List<Object> into columns of type ARRAY. For correctness: TypedValue.toJdbc(Calendar) should return an Array, not a List. Databases that happened to still work are just playing "nice".

          Show
          elserj Josh Elser added a comment - Given the javadoc, I'd expect the above to work. For a TypedValue, I should get an Object which is suitable to be set as an Array on the PreparedStatement. However, the above throws a ClassCastException (trying to cast Object[] to Array). Alright, I have convinced myself that the current javadoc on TypedValue is wrong. The reason we have not noticed this in the past is that HSQLDB will "coerce" Object[] (non-primitive type arrays) and List<Object> into columns of type ARRAY . For correctness: TypedValue.toJdbc(Calendar) should return an Array , not a List . Databases that happened to still work are just playing "nice".
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/calcite-avatica/pull/2

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/calcite-avatica/pull/2
          Hide
          elserj Josh Elser added a comment -
          Show
          elserj Josh Elser added a comment - Thanks for all of the reviews/feedback, folks. Fixed in https://git-wip-us.apache.org/repos/asf?p=calcite-avatica.git;a=commit;h=dd65a2b18b8c35cfccf1c47b6be87ea7db3ad658
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release avatica-1.10.0 (2017-05-30).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release avatica-1.10.0 (2017-05-30).

            People

            • Assignee:
              elserj Josh Elser
              Reporter:
              lukaslalinsky Lukas Lalinsky
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development