Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergei Kalashnikov <ztarvos@gmail.com>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches <tarantool-patches@freelists.org>
Subject: [tarantool-patches] Re: [PATCH] jdbc: fix primary keys meta retrieval
Date: Wed, 10 Oct 2018 11:14:05 +0300	[thread overview]
Message-ID: <20181010081401.GA22345@daedra.localdomain> (raw)
In-Reply-To: <20181009154005.uj5cwb6hcmhsmbju@tkn_work_nb>

Alexander, please find my answers inline below.

On Tue, Oct 09, 2018 at 06:40:06PM +0300, Alexander Turenko wrote:
> Hi, Sergei!
> 
> The patch is okay for me. I have a couple of minor questions below, can you
> please clarify them to me before we'll going to merge this patch?
> 
> WBR, Alexander Turenko.
> 
> > I have noticed another deviation from specification. The order in which columns are
> > returned in the resultset is not correct. The TABLE_CAT and TABLE_SCHEM must go first.
> > I have fixed this as well. Plz see the new test 'testGetPrimaryKeysOrderOfColumns'.
>  
> Good catch, thanks!
> 
> > > 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?
> > 
> > I've made some tests. For the 6th field of '_vspace' tuple, I've got the same 'List<Map>'
> > in both native and sql space cases, the difference was only in map contents.
> > However for the 5th field of '_vindex' tuple, it is 'List<List>' for the native space
> > and 'List<Map>' for the sql space.
> > 
> > So, I added extensive type checking and throw an exception in case unexpected type is encountered.
> > See tests 'testGetPrimaryKeyNonSQLSpace' and 'testDatabaseMetaDataGetPrimaryKeysFormatError'.
> > 
> > Are you certain that we should return the error in case we have been given a native space?
> > I went for empty resultset instead. Hope you are OK with that.
> 
> A 'native' space is not visible in Tarantool SQL as a table, so we
> likely should behave in the similar way as in no such table situation.
> 
> Don't found any information whether we should raise an error in case of
> non-exists table or return empty result set from the API reference. What
> other vendors do?  Postgresql seems ([1]) to return empty result set.
> 
> [1]: https://github.com/pgjdbc/pgjdbc/blob/08631ccdabdb8ba6d52f398e2b0b46a9cf0cafbf/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java#L2060
> 
> So it is ok if it is really common way to return metainformation (across API
> methods as well as across vendors). Are you have any extra info / experience on
> that?
> 

I just did a quick test with live Oracle instance and it returns empty
resultset for unknown table. So I guess, it is correct.


> > +    private void checkGetPrimaryKeysRow(ResultSet rs, String table, String colName, String pkName, int seq)
> > +        throws Exception {
> 
> Why `throws Exception`, but not `SQLException`? The following patch
> runs smoothly for me:
> 
> ```
> diff --git a/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java b/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java
> index 78ff89b..8577b23 100644
> --- a/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java
> +++ b/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java
> @@ -88,7 +88,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
>      }
>  
>      @Test
> -    public void testGetPrimaryKeys() throws Exception {
> +    public void testGetPrimaryKeys() throws SQLException {
>          ResultSet rs = meta.getPrimaryKeys(null, null, "TEST");
>  
>          assertNotNull(rs);
> @@ -102,7 +102,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
>      }
>  
>      @Test
> -    public void testGetPrimaryKeysCompound() throws Exception {
> +    public void testGetPrimaryKeysCompound() throws SQLException {
>          ResultSet rs = meta.getPrimaryKeys(null, null, "TEST_COMPOUND");
>  
>          assertNotNull(rs);
> @@ -118,7 +118,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
>      }
>  
>      @Test
> -    public void testGetPrimaryKeysIgnoresCatalogSchema() throws Exception {
> +    public void testGetPrimaryKeysIgnoresCatalogSchema() throws SQLException {
>          String[] vals = new String[] {null, "", "IGNORE"};
>          for (String cat : vals) {
>              for (String schema : vals) {
> @@ -168,7 +168,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
>      }
>  
>      private void checkGetPrimaryKeysRow(ResultSet rs, String table, String colName, String pkName, int seq)
> -        throws Exception {
> +        throws SQLException {
>          assertNull(rs.getString("TABLE_CAT"));
>          assertNull(rs.getString("TABLE_SCHEM"));
>          assertEquals(table, rs.getString("TABLE_NAME"));
> ```

Yes, it should be SQLException. Probably I was doing some experiments here and then forgot to clean it up.
Let me know if you want me to correct this with another revision of the patch.

--
Best Regards,
Sergei K.

  reply	other threads:[~2018-10-10  8:14 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-26  9:33 [tarantool-patches] " Sergei Kalashnikov
2018-10-01 13:04 ` [tarantool-patches] " Alexander Turenko
2018-10-02 17:09   ` Sergei Kalashnikov
2018-10-09  9:21   ` Sergei Kalashnikov
2018-10-09 15:40     ` Alexander Turenko
2018-10-10  8:14       ` Sergei Kalashnikov [this message]
2018-10-10 10:19         ` Alexander Turenko
2018-10-10 11:09           ` Sergei Kalashnikov
2018-10-10 12:50             ` Alexander Turenko
2018-10-10 13:19               ` Sergei Kalashnikov
2018-10-10 13:40                 ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181010081401.GA22345@daedra.localdomain \
    --to=ztarvos@gmail.com \
    --cc=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] jdbc: fix primary keys meta retrieval' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox