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.
next prev parent 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