feat: Add an ability to read metadata for attached databases #829

Destrolaric · 2023-01-16T14:16:06Z

Hi SQLite team!
Here are my proposed changes to the reading of metadata. To allow to read of metadata from attached databases
https://www.sqlite.org/lang_attach.html
~~This functionality will be optional and disabled by default because it adds more layers [schema].[table] for metadata reading.~~
The reason why this functionality will be useful:
It isn't easy to work with attached databases using JDBC API because all metadata reading assumes that we are in the main schema. This is fixable by adding schema information to the existing queries.

gotson · 2023-01-17T02:29:49Z

Hi, i don't think we should add a pragma for that. The JDBC methods accept a schema, and if it is null then it should not be used for filtering.

Destrolaric · 2023-01-17T07:03:15Z

Hi, pragma is used only for getSchemas to mark if we read them at all. Other parts just check for null.

gotson · 2023-01-17T07:16:41Z

Hi, pragma is used only for getSchemas to mark if we read them at all. Other parts just check for null.

i don't think the switch is necessary, we can always retrieve them all

Destrolaric · 2023-01-17T10:01:57Z

@gotson I have removed pragma now the schema read is by default

gotson

i did a very quick first review

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

gotson · 2023-01-18T02:59:25Z

Code formatting is failing, can you run mvn spotless:apply ?

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

src/test/java/org/sqlite/DBMetaDataTest.java

Destrolaric · 2023-01-18T11:05:16Z

Hi, I Added 2 test cases for the default DB and for attaching an additional one after getschemas() call. Also fixed formatting issues and now null schema replaced with main

gotson · 2023-01-19T02:45:10Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

+                .append(quote(s == null ? "main" : s))
+                .append(" as TABLE_SCHEM, tblname as TABLE_NAME, ")


I'm not sure about that.

The function should return the actual schema of the table, no? But it seems we just feed back the parameter that was passed.

If we have 2 tables with the same name in 2 different schemas, and we call this function without specifying the schema s, then that function should return columns from both tables, but we would need to have TABLE_SCHEM to differentiate them.

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

gotson · 2023-01-19T02:50:21Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

@@ -1198,7 +1206,9 @@ public ResultSet getPrimaryKeys(String c, String s, String table) throws SQLExce

        Statement stat = conn.createStatement();
        StringBuilder sql = new StringBuilder(512);
-        sql.append("select null as TABLE_CAT, ").append(quote(s)).append(" as TABLE_SCHEM, '")
+        sql.append("select null as TABLE_CAT, ")
+                .append(quote(s == null ? "main" : s))


similar as above, we should return the actual schema of the retrieved PK's table, not the parameter that was passed to the function

gotson · 2023-01-19T02:50:32Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

@@ -1249,7 +1259,7 @@ public ResultSet getExportedKeys(String catalog, String schema, String table)

        catalog = (catalog != null) ? quote(catalog) : null;

-        String quotedSchema = (schema != null) ? quote(schema) : null;
+        String quotedSchema = (schema != null) ? quote(schema) : quote("main");


similar as above, we should return the actual schema of the retrieved PK's table, not the parameter that was passed to the function

gotson · 2023-01-19T02:50:46Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

@@ -1406,12 +1416,12 @@ public ResultSet getImportedKeys(String catalog, String schema, String table)
        sql.append("select ")
                .append(quote(catalog))
                .append(" as PKTABLE_CAT, ")
-                .append(quote(schema))
+                .append(quote(schema == null ? "main" : schema))


similar as above, we should return the actual schema of the retrieved PK's table, not the parameter that was passed to the function

gotson · 2023-01-19T02:50:51Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

                .append(" as PKTABLE_SCHEM, ")
                .append("ptn as PKTABLE_NAME, pcn as PKCOLUMN_NAME, ")
                .append(quote(catalog))
                .append(" as FKTABLE_CAT, ")
-                .append(quote(schema))
+                .append(quote(schema == null ? "main" : schema))


similar as above, we should return the actual schema of the retrieved PK's table, not the parameter that was passed to the function

gotson · 2023-01-19T02:50:58Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

@@ -1526,7 +1536,7 @@ public ResultSet getIndexInfo(String c, String s, String table, boolean u, boole
        // define the column header
        // this is from the JDBC spec, it is part of the driver protocol
        sql.append("select null as TABLE_CAT,")
-                .append(quote(s))
+                .append(quote(s == null ? "main" : s))


similar as above, we should return the actual schema of the retrieved PK's table, not the parameter that was passed to the function

gotson · 2023-01-19T02:51:08Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

@@ -1689,7 +1705,10 @@ public synchronized ResultSet getTables(
        StringBuilder sql = new StringBuilder();
        sql.append("SELECT").append("\n");
        sql.append("  NULL AS TABLE_CAT,").append("\n");
-        sql.append("  NULL AS TABLE_SCHEM,").append("\n");
+        sql.append("  ")
+                .append(quote(s == null ? "main" : s))


similar as above, we should return the actual schema of the retrieved PK's table, not the parameter that was passed to the function

gotson · 2023-01-19T03:10:41Z

I think the logic should be enhance to retrieve various entities.

The way the schema parameter work in most of the DB Metadata function is as follows:

schemaPattern - a schema name pattern; must match the schema name as it is stored in the database; "" retrieves those without a schema; null means that the schema name should not be used to narrow the search

For all those functions, we probably need to apply that logic:

if schemaPattern is "", then we retrieve objects from the main database. That's a subset of 2.
if schemaPattern is null, then we should retrieve objects from all schemas. That means we need to loop through all schemas (getSchemas), and run the statements like table_info, table_xinfo and select from sqlite_master for all those schemas, and join the results. The returned TABLE_SCHEM should be the correct one, not the parameter that was passed to the function.
if schemaPattern is a String, we should match it against getSchemas. If found, we get items for that schema only. That's a subset of 2.

That means we need to change the logic for all those functions to loop through all known schemas, optionnaly filtered by the schemaPattern parameter, or using main, in order to call the sqlite functions or special tables with the schema, then join everything.

Destrolaric · 2023-01-20T08:39:21Z

@gotson Ok, this one looks doable and will probably, be ready sometime next week.

Destrolaric · 2023-02-13T17:07:28Z

@gotson Hi, sorry for waiting; unfortunately, I had no time to finish the pr before. I have a question. JDBC has some functions like getExportedKeys that require using full schema name and returns everything if null. Do we want to read the data from all existing schemas or to use old behaviour?

Destrolaric · 2023-02-13T17:41:45Z

Added schema(String, String) for pattern matching, which still requires some unit tests and discussion.

Destrolaric · 2023-02-13T19:12:33Z

        sql.append("    SELECT").append("\n");
        sql.append("      NAME,").append("\n");
        sql.append("      'GLOBAL TEMPORARY' AS TYPE").append("\n");
        sql.append("    FROM").append("\n");
        sql.append("      sqlite_temp_master").append("\n");

Also, what should I do with GLOBAL TEMPORARY tables? They kind of have their scheme now.
Converting pr to draft until all this is covered with tests.

Destrolaric · 2023-02-21T12:36:13Z

@gotson, Hi, again, can we discuss acceptance criteria regarding this pr? Currently, it needs to add some unit tests, and we can probably discuss if anything else is required. Also it looks like this one leads to the significant change of default behaviour, so it probably may require more accurate handling.

gotson · 2023-02-22T02:40:43Z

@gotson, Hi, again, can we discuss acceptance criteria regarding this pr? Currently, it needs to add some unit tests, and we can probably discuss if anything else is required. Also it looks like this one leads to the significant change of default behaviour, so it probably may require more accurate handling.

Hi, unit tests would be needed yes. What kind of significant changes of default behavior would you foresee though ?

Destrolaric · 2023-02-22T10:17:51Z

@gotson Well, there are some. Most importantly, if the value of the schema pattern is null, it will show all DBs instead of only 'main', that may look like a significant API change.

gotson · 2023-02-22T14:50:24Z

@gotson Well, there are some. Most importantly, if the value of the schema pattern is null, it will show all DBs instead of only 'main', that may look like a significant API change.

It's fine, it is more correct

…nto add_metadata_read_for_attached_db � Conflicts: � src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java � src/test/java/org/sqlite/DBMetaDataTest.java

Destrolaric · 2023-04-18T14:58:30Z

@gotson Sorry, could you look at the current state of pr and see if anything else is required?

gotson · 2023-04-19T12:29:05Z

@gotson Sorry, could you look at the current state of pr and see if anything else is required?

The PR has conflict, you would need to fix those before I can have a look.

Destrolaric · 2023-04-19T13:22:38Z

@gotson I resolved the conflict with the recent two commits

gotson · 2023-04-19T13:33:17Z

@gotson I resolved the conflict with the recent two commits

Thank you. I don't have much time these days, but I will look at this when I have time.

gotson · 2023-04-19T13:34:05Z

I still see conflicts though. Are you sure you pushed your latest changes?

Destrolaric · 2023-04-19T13:59:34Z

Weird, for me branch have no conflicts

gotson · 2023-04-19T14:06:27Z

Weird indeed

gotson · 2023-04-19T14:08:30Z

My bad, it's only the rebase (which was selected by default) that can't be done.

Squash and merge is ok (and it's what we want).

gotson · 2023-04-27T11:15:38Z

code formatting is failing, can you fix it with mvn spotless:apply ?

Destrolaric · 2023-04-27T11:32:28Z

@gotson Fixed

Destrolaric · 2023-05-15T11:44:41Z

@gotson Hi there! I hope you're doing well. I wanted to reach out and kindly ask if you had a chance to review the pull request.

gotson · 2023-05-15T13:03:07Z

@gotson Hi there! I hope you're doing well. I wanted to reach out and kindly ask if you had a chance to review the pull request.

It's on my list, unfortunately time is scarce these days. Hopefully i have some time this week to look at it!

gotson

For this review i mostly focused on the test class. I think we should have more coverage on dynamic attachment, ie for all functions:

test the behaviour
attach 1 db
test the behaviour
detach the db
test the behaviour

Is that something you could add ?

gotson · 2023-05-16T03:20:16Z

src/test/java/org/sqlite/DBMetaDataTest.java

+        //        assertThat(rs.next()).isTrue();
+        //        assertThat(rs.getString("TABLE_NAME")).isEqualTo("sqlite_schema");
+        //        assertThat(rs.getString("TABLE_SCHEM")).isEqualTo("temp");


What's with those commented lines?

gotson · 2023-05-16T03:23:42Z

src/test/java/org/sqlite/DBMetaDataTest.java

+        }
+    }
+
+    private void assertSystemSchema(ResultSet rs) throws SQLException {


shouldn't that also check for TABLE_SCHEM and compare it to a parameter passed in the function?

gotson · 2023-05-16T03:24:59Z

src/test/java/org/sqlite/DBMetaDataTest.java

+        // SYSTEM TABLE "sqlite_schema" for main
+        assertSystemSchema(rs);
+
+        // SYSTEM TABLE "sqlite_schema" for temp


where is that temp database attached? i can't find any mention to it

temp is for the sqlite temporary files, I think are returned as system external schema if any exist.

gotson · 2023-05-16T03:26:43Z

src/test/java/org/sqlite/DBMetaDataTest.java

@@ -1001,7 +999,7 @@ public void columnOrderOfgetTables() throws SQLException {
        ResultSet rsTables =
                meta.getTables(
                        null,
-                        null,
+                        "",


I'm not sure about that, i don't think passing "" should match any schema, that's what passing null is for.

gotson · 2023-05-16T03:27:41Z

src/test/java/org/sqlite/DBMetaDataTest.java

@@ -1158,7 +1156,7 @@ public void columnOrderOfgetProcedurColumns() throws SQLException {
    @Test
    public void columnOrderOfgetSchemas() throws SQLException {
        ResultSet rs = meta.getSchemas();
-        assertThat(rs.next()).isFalse();
+        assertThat(rs.next()).isTrue();


shouldn't we have more assertions to verify the content of the result set?

gotson · 2023-05-16T03:36:09Z

src/test/java/org/sqlite/DBMetaDataTest.java

+            rs = meta.getTables(null, "main", null, new String[] {"table"});
+            assertThat(rs.next()).isTrue();
+            assertThat(rs.getString("TABLE_NAME")).isEqualTo("test");
+            assertThat(rs.next()).isFalse();
+            rs.close();
+
+            rs = meta.getTables(null, "main", null, new String[] {"view"});
+            assertThat(rs.next()).isTrue();
+            assertThat(rs.getString("TABLE_NAME")).isEqualTo("testView");
+            assertThat(rs.next()).isFalse();
+            rs.close();
+
+            rs = meta.getTables(null, "main", null, new String[] {"system table"});
+            assertThat(rs.next()).isTrue();
+            assertThat(rs.getString("TABLE_NAME")).isEqualTo("sqlite_schema");
+            assertThat(rs.next()).isFalse();
+            rs.close();


shouldn't we have the same suite of tests for db2 for consistency?

gotson · 2023-05-16T03:37:24Z

src/test/java/org/sqlite/DBMetaDataTest.java

+                boolean schemaFound = false;
+                while (schemas.next()) {
+                    schemaFound = "db3".equals(schemas.getString("TABLE_SCHEM"));
+                    if (schemaFound) {
+                        break;
+                    }
+                }
+                assertThat(schemaFound).isTrue();


maybe put all schemas in a list and verify that it contains all the values we expect, ie db2,db3 and main.
If we expect some ordering, we should also test for that here.

gotson · 2023-05-16T03:38:06Z

src/test/java/org/sqlite/DBMetaDataTest.java

+        }
+
+        @Test
+        public void testGetSchemasForAttachedDatabases() throws SQLException, IOException {


the above function name implies it's about dynamic attachment, but this one doesn't

gotson · 2023-05-16T03:38:29Z

src/test/java/org/sqlite/DBMetaDataTest.java

+                assertThat(schemaFound).isTrue();
+
+            } finally {
+                stat.executeUpdate("detach database db3;");


should we also test after the detach that the schemas are properly returned?

gotson · 2023-05-16T03:45:22Z

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java

+        if ("".equals(schemaPattern)) {
+            schemaPattern = "main";
+        }


I don't think we should have that behaviour. The documentation says:

schemaPattern – a schema name; must match the schema name as it is stored in the database; null means schema name should not be used to narrow down the search.

There is no mention of a special handling when passing ""

feat: Add an ability to read metadata for attached databases

e7fe0a9

Destrolaric mentioned this pull request Jan 16, 2023

Support for attached databases (SQLite) dbeaver/dbeaver#7208

Open

remove read attached databases pragma

8ccbf02

gotson reviewed Jan 17, 2023

View reviewed changes

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java Outdated Show resolved Hide resolved

src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java Outdated Show resolved Hide resolved

suggestions from code review

24fcc05

gotson reviewed Jan 18, 2023

View reviewed changes

Destrolaric force-pushed the add_metadata_read_for_attached_db branch from 5e41cc5 to 4820bfd Compare January 18, 2023 11:03

additional test cases & improve metadata reading & formatting

6e59227

Destrolaric force-pushed the add_metadata_read_for_attached_db branch from 4820bfd to 6e59227 Compare January 18, 2023 11:13

gotson reviewed Jan 19, 2023

View reviewed changes

getSchemas(String, String) support & schema pattern matching

e5e4f1d

Destrolaric force-pushed the add_metadata_read_for_attached_db branch from 5b4b2ad to e5e4f1d Compare February 13, 2023 17:42

update getColumns to support patterns

6e6eea8

Destrolaric marked this pull request as draft February 13, 2023 19:27

Destrolaric added 2 commits February 14, 2023 15:38

add handling when schema is not found

b0eab66

update tests

1313587

Merge branch 'master' of https://github.com/Destrolaric/sqlite-jdbc i…

79fa812

…nto add_metadata_read_for_attached_db � Conflicts: � src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java � src/test/java/org/sqlite/DBMetaDataTest.java

Fix error in the tests after the merge conflict resolution

61f5344

Destrolaric marked this pull request as ready for review April 18, 2023 14:59

Destrolaric force-pushed the add_metadata_read_for_attached_db branch from 469887c to 61f5344 Compare April 18, 2023 15:08

gotson self-assigned this Apr 19, 2023

Apply code style

cd76f57

Destrolaric requested a review from gotson May 4, 2023 15:28

gotson requested changes May 16, 2023

View reviewed changes

gotson added the enhancement:JDBC Enhancement specific to the JDBC standard label May 23, 2023

gotson removed their assignment Jul 17, 2023

		.append(quote(s == null ? "main" : s))
		.append(" as TABLE_SCHEM, tblname as TABLE_NAME, ")

feat: Add an ability to read metadata for attached databases #829

Are you sure you want to change the base?

feat: Add an ability to read metadata for attached databases #829

Conversation

Destrolaric commented Jan 16, 2023 • edited

gotson commented Jan 17, 2023

Destrolaric commented Jan 17, 2023

gotson commented Jan 17, 2023

Destrolaric commented Jan 17, 2023

gotson left a comment

Choose a reason for hiding this comment

gotson commented Jan 18, 2023

Destrolaric commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotson commented Jan 19, 2023

Destrolaric commented Jan 20, 2023

Destrolaric commented Feb 13, 2023 • edited

Destrolaric commented Feb 13, 2023

Destrolaric commented Feb 13, 2023 • edited

Destrolaric commented Feb 21, 2023

gotson commented Feb 22, 2023

Destrolaric commented Feb 22, 2023

gotson commented Feb 22, 2023

Destrolaric commented Apr 18, 2023

gotson commented Apr 19, 2023

Destrolaric commented Apr 19, 2023

gotson commented Apr 19, 2023

gotson commented Apr 19, 2023

Destrolaric commented Apr 19, 2023

gotson commented Apr 19, 2023

gotson commented Apr 19, 2023

gotson commented Apr 27, 2023

Destrolaric commented Apr 27, 2023

Destrolaric commented May 15, 2023

gotson commented May 15, 2023

gotson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Destrolaric May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotson May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotson May 16, 2023 • edited

Choose a reason for hiding this comment

Destrolaric commented Jan 16, 2023 •

edited

Destrolaric commented Feb 13, 2023 •

edited

Destrolaric commented Feb 13, 2023 •

edited

Destrolaric May 16, 2023 •

edited

gotson May 16, 2023 •

edited

gotson May 16, 2023 •

edited