Add connector test for materialized view by findepi · Pull Request #8117 · trinodb/trino

findepi · 2021-05-28T10:05:59Z

This also improves connector tests for view.

cc @martint @raunaqmorarka @sopel39 @hashhar @wendigo

relates to https://trinodb.slack.com/archives/C01UM2VGMC3/p1622015285003600, https://trinodb.slack.com/archives/C01UM2VGMC3/p1619625097028600

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorSmokeTest.java

raunaqmorarka · 2021-05-28T10:23:28Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

should return materialised view, as it's also a view
I don't think we have established what is the right behaviour here
We might want to list MVs in another metadata table instead of listing in information_schema.views

will add a comment

// TODO should probably return materialized view, as it's also a view -- to be double checked

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java

raunaqmorarka · 2021-05-28T10:29:40Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

We can also add system.jdbc tests similar to https://github.com/trinodb/trino/pull/8113/files#diff-6f73e014ee92e4e98ff14049eaa094585b893d44d7ee942a4c5a32deee15d836R129

awesome, however, i would prefer to have this tested end-to-end via java.sql.DatabaseMetadata.
it then is kind of cumbersome as a connector test

@raunaqmorarka can you please file an issue to have DatabaseMetadata tested with MVs?

created #8128

losipiuk · 2021-05-28T14:38:20Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java

nit: separete commit

losipiuk · 2021-05-28T14:39:32Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

side note: do we want that check actually?

@losipiuk removed for now. I think it's default IDE's behavior, so if we want to ignore it, we should document that in DEVELOP*.md

losipiuk · 2021-05-28T14:39:50Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

@raunaqmorarka is there an issue for fix Iceberg MV reporting?

BTW Do we have any roadmap issue for materialized views?

I already have a fix for iceberg information_schema.tables listing in 5fff606

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

losipiuk · 2021-05-28T14:46:01Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

also add a test which filters byt type expecting VIEW or MATERIALIZED VIEW depending on what we settle on.

Or assert on type.

@losipiuk i think we didn't settle on this yet, but that's a very good idea.

for now will have assertion like this:

.containsAll("VALUES ('" + viewName + "', 'BASE TABLE')"); // TODO table_type should probably be "* VIEW"

losipiuk · 2021-05-28T14:47:17Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/AbstractTestIcebergConnectorTest.java

where are you overriding those?

@losipiuk in AbstractTestIcebergConnectorTest

@losipiuk sorry, i think i misunderstood the question.

losipiuk

Thanks.
Nice to have as a base for further work on MVs.

findepi · 2021-05-28T15:16:27Z

ac

findepi · 2021-05-28T15:22:51Z

https://github.com/trinodb/trino/runs/2693225647

2021-05-28T10:19:18.4025897Z [ERROR] testCapturingSuccessfulStatement(io.trino.plugin.postgresql.TestTestingPostgreSqlServer)  Time elapsed: 0.051 s  <<< FAILURE!
2021-05-28T10:19:18.4067860Z java.lang.AssertionError: 
2021-05-28T10:19:18.4069489Z 
2021-05-28T10:19:18.4072161Z Expecting ArrayList:
2021-05-28T10:19:18.4074392Z  <[]>
2021-05-28T10:19:18.4076456Z to contain:
2021-05-28T10:19:18.4081401Z  <[RemoteDatabaseEvent{query=SELECT 1, status=RUNNING}]>
2021-05-28T10:19:18.4085770Z but could not find the following element(s):
2021-05-28T10:19:18.4090833Z  <[RemoteDatabaseEvent{query=SELECT 1, status=RUNNING}]>

cc @kokosing

findepi · 2021-05-28T15:23:54Z

It turns out Accumulo supports views, but its tests were lying it does not.
Fixing.

This also improves connector tests for view.

raunaqmorarka · 2021-05-28T16:28:32Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorSmokeTest.java

+    @Override
+    public void testShowCreateTable()
+    {
+        assertThat((String) computeScalar("SHOW CREATE TABLE region"))


nit: cast to string is not needed

indeed, but may help completion. i will keep for now, but we can remove as followup

raunaqmorarka · 2021-05-28T16:28:51Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java

+    @Override
+    public void testShowCreateTable()
+    {
+        assertThat((String) computeScalar("SHOW CREATE TABLE region"))


nit: cast to string is not needed

findepi requested a review from losipiuk May 28, 2021 10:05

cla-bot bot added the cla-signed label May 28, 2021

sopel39 requested a review from raunaqmorarka May 28, 2021 10:09

findepi mentioned this pull request May 28, 2021

Add listMaterializedViews and getMaterializedViews SPI #8113

Merged

raunaqmorarka reviewed May 28, 2021

View reviewed changes

losipiuk reviewed May 28, 2021

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java Outdated

Copy link
Copy Markdown

Member

losipiuk May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: separete commit

losipiuk reviewed May 28, 2021

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java Outdated Show resolved Hide resolved

losipiuk reviewed May 28, 2021

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved

losipiuk reviewed May 28, 2021

View reviewed changes

losipiuk approved these changes May 28, 2021

View reviewed changes

findepi force-pushed the findepi/mvct branch from d2457d6 to 5656a25 Compare May 28, 2021 15:16

Add connector test for materialized view

cf933f4

This also improves connector tests for view.

findepi force-pushed the findepi/mvct branch from 5656a25 to cf933f4 Compare May 28, 2021 15:24

raunaqmorarka approved these changes May 28, 2021

View reviewed changes

findepi merged commit 36cada0 into trinodb:master May 28, 2021

findepi deleted the findepi/mvct branch May 28, 2021 18:15

findepi added this to the 358 milestone May 28, 2021

Conversation

findepi commented May 28, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented May 28, 2021

Uh oh!

findepi commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented May 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

findepi commented May 28, 2021 •

edited

Loading