Add support for prepared statements in JDBC #3116
Conversation
ohadzeliger
changed the title
# Conflicts: # yaml-tests/src/test/java/JDBCYamlIntegrationTests.java
…t type mismatch in array.
| case Types.DOUBLE: | ||
| return column.getDouble(); | ||
| default: | ||
| // TODO: NULL? |
There was a problem hiding this comment.
"enum" is also missing from this, but enum support in general is awaiting: #3074
There was a problem hiding this comment.
Is #3074 going to be merged in soon? If so, I can add support here after it was merged.
There was a problem hiding this comment.
#3074 merged, but I don't know that we need to add support here, now.
fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/TypeConversion.java
Show resolved
Hide resolved
fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/TypeConversion.java
Show resolved
Hide resolved
fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/TypeConversion.java
Outdated
Show resolved
Hide resolved
fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/TypeConversion.java
Outdated
Show resolved
Hide resolved
| * TODO: We can't use the other implementation of array in {@link RelationalArrayFacade} as it makes several assumptions | ||
| * that would be incompatible with the need to transfer an array across the wire (e.g. the type of element is missing, | ||
| * {@link RelationalArrayFacade#getArray()} returns a type other than a Java array). |
There was a problem hiding this comment.
Why create a new implementation rather than changing RelationalArrayFacade? It seems like the two are trying to accomplish the same thing.
There was a problem hiding this comment.
I wanted to reduce risk: RelationalArrayFacade is used in many places and reimplementing it as part of this PR would require many changes (potentially).
There was a problem hiding this comment.
I'm still not sure I follow, I'll reach out offline.
fdb-relational-jdbc/src/main/java/com/apple/foundationdb/relational/jdbc/JDBCArrayImpl.java
Show resolved
Hide resolved
fdb-relational-jdbc/src/main/java/com/apple/foundationdb/relational/jdbc/ParameterHelper.java
Outdated
Show resolved
Hide resolved
fdb-relational-jdbc/src/main/java/com/apple/foundationdb/relational/jdbc/ParameterHelper.java
Show resolved
Hide resolved
| @@ -49,7 +49,10 @@ message Struct { | |||
|
|
|||
| // Relational Array. | |||
| message Array { | |||
There was a problem hiding this comment.
I think by making this a backward compatible change, and if the elementType is set (non-zero), the client will just get the type from the columns.
I think this shouldn't be much work, and would allow us to keep the mixed-mode testing for returning arrays.
There was a problem hiding this comment.
Making the changes allows arrays.yamsql to run, however prepared.yamsql still fails: The server-side code to deserialize (FRL.java:254) cannot handle an ARRAY type required by the tests.
We may cherry pick this PR to the earlier branch if we want to allow these tests to run against older seervers
# Conflicts: # yaml-tests/src/test/java/YamlIntegrationTests.java
…here needed for old server support
| return fromArray(column.getArray()); | ||
| case Types.BIGINT: | ||
| checkColumnType(columnType, column.hasLong()); |
There was a problem hiding this comment.
Does this work correctly if the column has the default value for the desired item, i.e. 0 for a long, or an int, or an empty string?
| case Types.DOUBLE: | ||
| return column.getDouble(); | ||
| default: | ||
| // TODO: NULL? |
There was a problem hiding this comment.
#3074 merged, but I don't know that we need to add support here, now.
| @@ -58,6 +58,7 @@ test_block: | |||
| - | |||
| # correlation are allowed in from-subquery. | |||
| - query: select x, sq.idr, sq.nr from a, (select * from r where r.idr = a.x) sq; | |||
| - supported_version: !current_version # changes in Proto definition for arrays | |||
| * TODO: We can't use the other implementation of array in {@link RelationalArrayFacade} as it makes several assumptions | ||
| * that would be incompatible with the need to transfer an array across the wire (e.g. the type of element is missing, | ||
| * {@link RelationalArrayFacade#getArray()} returns a type other than a Java array). |
There was a problem hiding this comment.
I'm still not sure I follow, I'll reach out offline.
This PR enables support for prepared statements on the JDBC server.
Specifically, support for PreparedStatement
setObject()andsetArray().Also,
JDBCRelationalConnectionneeded support for array creation.A slew of type conversion utilities were added in support of these features.
In general, we have 3 models that we are dealing with:
And because of the way the API is structured, we may need to perform conversions between all 3. Specifically, we support the following:
Connection.createArrayOf, where the type of array elements is also providedArrayandStructare assumed in all cases of nesting. That is, an array of elements would be assumed to have SQLArrayif there is any nested array. The reasoning is that we would need the additional type information in these casesSome cleanup and restructuring of that code may be warranted, in addition to null, error and edge case testing.
The protobuf for the
Arraydefinition was changed in order to support the extra type information.The change was made to be backwards compatible at the protobuf level but the old server (before this change) cannot handle arrays in parameters. In order to facilitate multi-version testing against the older servers we may be able to cherry-pick these commits to the server's branch.