From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 47A282BB52 for ; Tue, 9 Oct 2018 11:40:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6Gz3afEflliY for ; Tue, 9 Oct 2018 11:40:10 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 902852BB49 for ; Tue, 9 Oct 2018 11:40:09 -0400 (EDT) Date: Tue, 9 Oct 2018 18:40:06 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] jdbc: fix primary keys meta retrieval Message-ID: <20181009154005.uj5cwb6hcmhsmbju@tkn_work_nb> References: <1537954408-3275-1-git-send-email-ztarvos@gmail.com> <20181001130427.zlugbqlg3madlo7r@tkn_work_nb> <20181009092158.GA17512@daedra.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20181009092158.GA17512@daedra.localdomain> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Sergei Kalashnikov Cc: tarantool-patches 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' > 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' for the native space > and 'List' 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? > + 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")); ```