Allow tables with same name in different schemas in Blackhole #550

kokosing · 2019-03-27T08:52:31Z

Minor bug fix in BlackHoleConnector

BlackholeConnector cannot have tables with same table names in different
schemas.

This is because originally BlackHoleConnector only supports default
schema so it use the table name string to identify table. When creating
schema was supported in 4e522f355ed7f0711d877e2259e489a8ebf626b9, the
table identity should be updated to use SchemaTableName.

Extracted from: prestodb/presto#12457

cla-bot · 2019-03-27T08:52:33Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

sopel39 · 2019-03-27T11:17:17Z

presto-blackhole/src/main/java/io/prestosql/plugin/blackhole/BlackHoleMetadata.java

@@ -62,7 +62,7 @@
    public static final String SCHEMA_NAME = "default";

    private final List<String> schemas = new ArrayList<>();
-    private final Map<String, BlackHoleTableHandle> tables = new ConcurrentHashMap<>();
+    private final Map<SchemaTableName, BlackHoleTableHandle> tables = new ConcurrentHashMap<>();


Is there a check now that disallows table with same name in different schemas?
I don't see it.

Why would this be needed? SchemaTableName is a pair of schema and table name.

findepi · 2019-03-27T11:26:31Z

@kokosing please fix the cmt msg so that it says what the commit does. I edited the PR title with my suggestion for the cmt title.

findepi · 2019-03-27T11:27:32Z

presto-blackhole/src/test/java/io/prestosql/plugin/blackhole/TestBlackHoleSmoke.java

@@ -69,8 +69,14 @@ public void tearDown()
    public void testCreateSchema()
    {
        assertEquals(queryRunner.execute("SHOW SCHEMAS FROM blackhole").getRowCount(), 2);
+        assertThatQueryReturnsValue("CREATE TABLE nation as SELECT * FROM tpch.tiny.nation", 25L);


This doesn't really belong to testCreateSchema, does it?

hmm... I remember ending up doing this for some reason but didn't remember exactly ... it relates to the fact that we cannot cleanup new created schema in BlackholeConnector.

findepi · 2019-03-27T11:28:40Z

presto-blackhole/src/test/java/io/prestosql/plugin/blackhole/TestBlackHoleSmoke.java

        queryRunner.execute("CREATE SCHEMA blackhole.test");
        assertEquals(queryRunner.execute("SHOW SCHEMAS FROM blackhole").getRowCount(), 3);
+        assertThatQueryReturnsValue("CREATE TABLE test.nation as SELECT * FROM tpch.tiny.nation", 25L);


Since default.nation and test.nation are identical, we cannot test whether we have 2 tables or just one.
Create tables with same name, but with different "type" (column list).

findepi · 2019-03-27T11:29:07Z

presto-jdbc/src/test/java/io/prestosql/jdbc/TestPrestoDriver.java

@@ -1435,7 +1435,7 @@ public void testQueryCancelByInterrupt()
        AtomicReference<Throwable> queryFailure = new AtomicReference<>();

        Future<?> queryFuture = executorService.submit(() -> {
-            try (Connection connection = createConnection("blackhole", "default");
+            try (Connection connection = createConnection("blackhole", "blackhole");


Are the changes in this file necessary?

@findepi They are :) . Since before the schema name is not used to lookup the table, so it doesn't matter which schema you used... you still end up find the table in other schemas :)

@wenleix thanks for the clarification!

wenleix · 2019-03-27T18:29:26Z

@findepi

please fix the cmt msg so that it says what the commit does. I edited the PR title with my suggestion for the cmt title.

I assume "cmt msg" actually means "cmt title" ;) ? Since the whole commit message explained what this commit tries to fix.

For the commit title, there is always the tradeoff of making commit title too long (50 characters) vs. make things clear in commit title :/

cla-bot · 2019-04-09T07:50:01Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

kokosing

Rebased and responded to comments.

kokosing · 2019-04-09T06:24:04Z

presto-blackhole/src/main/java/io/prestosql/plugin/blackhole/BlackHoleMetadata.java

@@ -62,7 +62,7 @@
    public static final String SCHEMA_NAME = "default";

    private final List<String> schemas = new ArrayList<>();
-    private final Map<String, BlackHoleTableHandle> tables = new ConcurrentHashMap<>();
+    private final Map<SchemaTableName, BlackHoleTableHandle> tables = new ConcurrentHashMap<>();


Why would this be needed? SchemaTableName is a pair of schema and table name.

BlackholeConnector cannot have tables with same table names in different schemas. This is because originally BlackHoleConnector only supports `default` schema so it use the table name string to identify table. When creating schema was supported in 4e522f355ed7f0711d877e2259e489a8ebf626b9, the table identity should be updated to use SchemaTableName. Extracted from: prestodb/presto#12457

cla-bot · 2019-05-01T08:11:48Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

sopel39 reviewed Mar 27, 2019

View reviewed changes

findepi changed the title ~~Minor bug fix in BlackHoleConnector~~ Allow tables with same name in different schemas in Blackhole Mar 27, 2019

findepi requested changes Mar 27, 2019

View reviewed changes

kokosing force-pushed the origin/master/113_port_12457_from_legacy_presto branch from 9e74199 to c855e65 Compare April 9, 2019 07:49

kokosing commented Apr 9, 2019

View reviewed changes

findepi approved these changes Apr 9, 2019

View reviewed changes

kokosing force-pushed the origin/master/113_port_12457_from_legacy_presto branch from c855e65 to 8ed4664 Compare May 1, 2019 08:11

kokosing merged commit 207665e into trinodb:master May 1, 2019

kokosing deleted the origin/master/113_port_12457_from_legacy_presto branch May 1, 2019 13:15

kokosing added this to the 310 milestone May 1, 2019

kokosing mentioned this pull request May 1, 2019

Release notes for 310 #678

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow tables with same name in different schemas in Blackhole #550

Allow tables with same name in different schemas in Blackhole #550

kokosing commented Mar 27, 2019

cla-bot bot commented Mar 27, 2019

sopel39 Mar 27, 2019

kokosing Apr 9, 2019

findepi commented Mar 27, 2019

findepi Mar 27, 2019

wenleix Mar 27, 2019

findepi Mar 27, 2019

findepi Mar 27, 2019

wenleix Mar 27, 2019

findepi Mar 27, 2019

wenleix commented Mar 27, 2019 •

edited

Loading

cla-bot bot commented Apr 9, 2019

kokosing left a comment

kokosing Apr 9, 2019

cla-bot bot commented May 1, 2019

Allow tables with same name in different schemas in Blackhole #550

Allow tables with same name in different schemas in Blackhole #550

Conversation

kokosing commented Mar 27, 2019

cla-bot bot commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Mar 27, 2019

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

wenleix commented Mar 27, 2019 • edited Loading

cla-bot bot commented Apr 9, 2019

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented May 1, 2019

wenleix commented Mar 27, 2019 •

edited

Loading