[tarantool-patches] Re: [PATCH] jdbc: fix primary keys meta retrieval

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi!

Sorry for the late response.

From [1]:

> Retrieves a description of the given table's primary key columns. They are ordered by COLUMN_NAME.

That is strange, but that is. Maybe we should also check the standard.

[1]: https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html#getPrimaryKeys(java.lang.String,%20java.lang.String,%20java.lang.String)

Tests are ok for me.

Maybe we also should set some defined behaviour for the case when
'catalog' and 'schema' are given for getPrimaryKeys. I think we should
ignore them and return nulls in TABLE_CAT and TABLE_SCHEM. I think we
should have corresponding test case.

More comment are inline.

WBR, Alexander Turenko.

On Wed, Sep 26, 2018 at 12:33:28PM +0300, Sergei Kalashnikov wrote:
> Fixed parsing primary keys metadata response.
> Updated integration tests.
> 
> Closes #41
> ---
> https://github.com/tarantool/tarantool-java/issues/41
> https://github.com/ztarvos/tarantool-java/commits/gh-41-fix-jdbc-pk-meta
> 
>  .../org/tarantool/jdbc/SQLDatabaseMetadata.java    |  8 +++---
>  .../java/org/tarantool/jdbc/AbstractJdbcIT.java    | 11 ++++----
>  .../org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java | 32 ++++++++++++++++++++--
>  3 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/src/main/java/org/tarantool/jdbc/SQLDatabaseMetadata.java b/src/main/java/org/tarantool/jdbc/SQLDatabaseMetadata.java
> index e43bcdc..790bbe7 100644
> --- a/src/main/java/org/tarantool/jdbc/SQLDatabaseMetadata.java
> +++ b/src/main/java/org/tarantool/jdbc/SQLDatabaseMetadata.java
> @@ -774,12 +774,12 @@ public class SQLDatabaseMetadata implements DatabaseMetaData {
>              List<Map<String, Object>> fields = (List<Map<String, Object>>) space.get(FORMAT_IDX);
>              List<List<Object>> indexes = (List<List<Object>>) connection.connection.select(_VINDEX, 0, Arrays.asList(space.get(SPACE_ID_IDX), 0), 0, 1, 0);
>              List<Object> primaryKey = indexes.get(0);
> -            List<List<Object>> parts = (List<List<Object>>) primaryKey.get(INDEX_FORMAT_IDX);
> +            List<Map<String, Object>> parts = (List<Map<String, Object>>) primaryKey.get(INDEX_FORMAT_IDX);

I think we should check the type to give a proper error message in the
case when a user give 'native' tarantool space in parameters.
Corresponding test case should be added.

Don't sure how it will be wrapped in Java, I just about the following:

6th field of _vspace tuple for a 'native' space:

[[0, 'unsigned'], [1, 'unsigned']]

(Don't sure whether it is list or map in the binary protocol; likely
map.)

6th field of _vspace tuple for a sql space:

[{'sort_order': 'asc', 'type': 'scalar', 'field': 0,
    'nullable_action': 'abort', 'is_nullable': false}]

What would going on if a user give us 'native' space name?

>              for (int i = 0; i < parts.size(); i++) {
> -                List<Object> part = parts.get(i);
> -                int ordinal = ((Number) part.get(0)).intValue();
> +                Map<String, Object> part = parts.get(i);
> +                int ordinal = ((Number) part.get("field")).intValue();
>                  String column = (String) fields.get(ordinal).get("name");

Just note: it is not mandatory to space format fields to have a name,
but SQL spaces have them. Maybe a comment needed here.

> -                rows.add(Arrays.asList(table, column, i + 1, "primary", primaryKey.get(NAME_IDX)));
> +                rows.add(Arrays.asList(table, column, i + 1, primaryKey.get(NAME_IDX)));
>              }
>          }
>          return new SQLNullResultSet((JDBCBridge.mock(