Filter out Hive information_schema and sys #3008

findepi · 2020-03-05T12:51:37Z

information_schema will be inaccessible anyway.

sys could be accessible, but

it doesn't work (contains JdbcStorageHandler tables and Hive views)
exposing it may require proper handling in access control.

findepi · 2020-03-05T12:52:53Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveSchema.java

+import static io.prestosql.tests.utils.QueryExecutors.onPresto;
+import static java.util.Objects.requireNonNull;
+
+public class TestHiveSchema


Dear Reviewer,
I know that the TestHiveSchema is awfully long and looks like 90% redundant, but it is actually not.

aalbu · 2020-03-05T13:48:12Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

            return ImmutableList.of(schemaName.get());
        }
        return listSchemaNames(session);
    }

+    private static boolean filterSchema(String schemaName)


This is a subjective comment, but to me if (!filterSchema(tableName.getSchemaName())) isn't very intuitive. Perhaps if the we'd call the method something like isSystemSchema() (of course, the return values would be reversed), those tests would read better.

it seems filter is overloaded, and doesn't really mean whether it's positive (filter) or negative (filter out, exclude).
the reason i picked this name is -- java.util.stream.Stream#filter, io.prestosql.security.AccessControl#filterSchemas ...

hmm maybe isExcludedSchema ?

i am keeping filterSchema

isHiveMetadataSchema?

filterSchema sounds like *AccessControl#filterSchemas. I don't like this name here.

the problem is with semntics of the method

While it's true today, in general: metadata schema != we should hide it

so isHiveMetadataSchema isn't IMO btter

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveSchema.java

`information_schema` will be inaccessible anyway. `sys` could be accessible, but - it doesn't work (contains JdbcStorageHandler tables and Hive views) - exposing it may require proper handling in access control.

kokosing · 2020-03-06T10:40:04Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

            return ImmutableList.of(schemaName.get());
        }
        return listSchemaNames(session);
    }

+    private static boolean filterSchema(String schemaName)


isHiveMetadataSchema?

filterSchema sounds like *AccessControl#filterSchemas. I don't like this name here.

kokosing · 2020-03-06T10:40:42Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

+        }
+        if ("sys".equals(schemaName)) {
+            // Hive 3's `sys` schema contains no objects we can handle, so there is no point in exposing it.
+            // Also, exposing it may require proper handling in access control.


What do you mean by: exposing it may require proper handling in access control.? What is sys schema?

sys contains table paramters, partition paramters, some stats, etc.

kokosing · 2020-03-06T10:41:25Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

            return ImmutableList.of(schemaName.get());
        }
        return listSchemaNames(session);
    }

+    private static boolean filterSchema(String schemaName)


I would put this method under the last usage.

kokosing · 2020-03-06T10:43:29Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveSchema.java

+    public void setUp()
+    {
+        // make sure hive.default schema is not empty
+        onPresto().executeQuery("DROP TABLE IF EXISTS hive.default.test_sys_schema_disabled_table_in_default");


this is going to be called before each test. Maybe you could have a variable that would not create this table if it is already created, then tearDown could be removed. WDYT?

in general -- i agree

it's tiny overhead compared to the number of test queries. i choose to ignore this

kokosing · 2020-03-06T10:45:42Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveSchema.java

+import static io.prestosql.tests.utils.QueryExecutors.onPresto;
+import static java.util.Objects.requireNonNull;
+
+public class TestHiveSchema


It is a pity that this is a product test. After #3009 we could write "unit" test, but still it is not going to be much faster, but it would be easier to run tests from IDE.

it's a necessity that there is a product test. i want to run with HMS whether information_schema/sys exists and where it doesn't

it would be good to have a unit test too though. out of scope

kokosing · 2020-03-06T10:46:28Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveSchema.java

+                .satisfies(containsFirstColumnValue("information_schema"));
+        assertThat(onPresto().executeQuery("SELECT table_name FROM information_schema.tables WHERE table_schema = 'information_schema'"))
+                .containsOnly(allInformationSchemaTablesAsRows);
+        Assertions.assertThat(onPresto().executeQuery("SELECT table_schema, table_name FROM information_schema.tables").rows().stream()


static import

static import

assertThat?
I use tempto QueryAssert.assertThat and assertj Assertions.assertThat -- it happens; not pretty; we already do this in other tests

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveSchema.java

findepi · 2020-03-06T14:24:54Z

AC

findepi requested review from aalbu and kokosing March 5, 2020 12:51

cla-bot bot added the cla-signed label Mar 5, 2020

findepi commented Mar 5, 2020

View reviewed changes

aalbu reviewed Mar 5, 2020

View reviewed changes

findepi mentioned this pull request Mar 5, 2020

Implement QueryResult#toString trinodb/tempto#44

Merged

Filter out Hive information_schema and sys

c0d69ec

`information_schema` will be inaccessible anyway. `sys` could be accessible, but - it doesn't work (contains JdbcStorageHandler tables and Hive views) - exposing it may require proper handling in access control.

findepi force-pushed the hdp4 branch from e9e6893 to c0d69ec Compare March 5, 2020 15:37

findepi mentioned this pull request Mar 5, 2020

Hive 3 #1218

Open

29 tasks

kokosing approved these changes Mar 6, 2020

View reviewed changes

findepi force-pushed the hdp4 branch from eb212ca to 4619ca0 Compare March 6, 2020 14:25

Run storage_formats for kerberized HDFS without impersonation

ac1b602

findepi force-pushed the hdp4 branch from 4619ca0 to ac1b602 Compare March 9, 2020 11:58

findepi merged commit f01b3b4 into trinodb:master Mar 9, 2020

findepi deleted the hdp4 branch March 9, 2020 13:19

findepi mentioned this pull request Mar 9, 2020

Release notes for 331 #2880

Closed

6 tasks

findepi added this to the 331 milestone Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out Hive information_schema and sys #3008

Filter out Hive information_schema and sys #3008

findepi commented Mar 5, 2020

findepi Mar 5, 2020

aalbu Mar 5, 2020

findepi Mar 5, 2020

findepi Mar 6, 2020

kokosing Mar 6, 2020

findepi Mar 6, 2020

kokosing Mar 6, 2020

kokosing Mar 6, 2020

findepi Mar 6, 2020

kokosing Mar 6, 2020

findepi Mar 6, 2020

kokosing Mar 6, 2020

findepi Mar 6, 2020

kokosing Mar 6, 2020

findepi Mar 6, 2020

kokosing Mar 6, 2020

findepi Mar 6, 2020

findepi commented Mar 6, 2020

Filter out Hive information_schema and sys #3008

Filter out Hive information_schema and sys #3008

Conversation

findepi commented Mar 5, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Mar 6, 2020