-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add connector test for materialized view #8117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.trino.plugin.hive; | ||
|
|
||
| import io.trino.testing.BaseConnectorSmokeTest; | ||
| import io.trino.testing.QueryRunner; | ||
| import io.trino.testing.TestingConnectorBehavior; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| // Redundant over TestHiveConnectorTest, but exists to exercise BaseConnectorSmokeTest | ||
| // Some features like views may be supported by Hive only. | ||
| public class TestHiveConnectorSmokeTest | ||
| extends BaseConnectorSmokeTest | ||
| { | ||
| @Override | ||
| protected QueryRunner createQueryRunner() | ||
| throws Exception | ||
| { | ||
| return HiveQueryRunner.builder() | ||
| .setInitialTables(REQUIRED_TPCH_TABLES) | ||
| .build(); | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | ||
| { | ||
| switch (connectorBehavior) { | ||
| case SUPPORTS_TOPN_PUSHDOWN: | ||
| return false; | ||
|
|
||
| case SUPPORTS_CREATE_VIEW: | ||
| return true; | ||
|
|
||
| case SUPPORTS_DELETE: | ||
| return true; | ||
|
|
||
| default: | ||
| return super.hasBehavior(connectorBehavior); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void testDelete() | ||
| { | ||
| assertThatThrownBy(super::testDelete) | ||
| .hasMessage("Deletes must match whole partitions for non-transactional tables"); | ||
| } | ||
|
|
||
| @Test | ||
| @Override | ||
| public void testShowCreateTable() | ||
| { | ||
| assertThat((String) computeScalar("SHOW CREATE TABLE region")) | ||
| .isEqualTo("" + | ||
| "CREATE TABLE hive.tpch.region (\n" + | ||
| " regionkey bigint,\n" + | ||
| " name varchar(25),\n" + | ||
| " comment varchar(152)\n" + | ||
| ")\n" + | ||
| "WITH (\n" + | ||
| " format = 'ORC'\n" + | ||
| ")"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,6 +203,13 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | |
| switch (connectorBehavior) { | ||
| case SUPPORTS_TOPN_PUSHDOWN: | ||
| return false; | ||
|
|
||
| case SUPPORTS_CREATE_VIEW: | ||
| return true; | ||
|
|
||
| case SUPPORTS_DELETE: | ||
| return true; | ||
|
|
||
| default: | ||
| return super.hasBehavior(connectorBehavior); | ||
| } | ||
|
|
@@ -211,7 +218,8 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | |
| @Override | ||
| public void testDelete() | ||
| { | ||
| throw new SkipException("Hive connector supports row-by-row delete only for ACID tables but these currently cannot be used with file metastore."); | ||
| assertThatThrownBy(super::testDelete) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: separete commit |
||
| .hasStackTraceContaining("Deletes must match whole partitions for non-transactional tables"); | ||
| } | ||
|
|
||
| @Test | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,6 +121,10 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | |
| case SUPPORTS_RENAME_TABLE: | ||
| case SUPPORTS_TOPN_PUSHDOWN: | ||
| return false; | ||
|
|
||
| case SUPPORTS_CREATE_MATERIALIZED_VIEW: | ||
| return true; | ||
|
|
||
| case SUPPORTS_DELETE: | ||
| return true; | ||
| default: | ||
|
|
@@ -142,6 +146,7 @@ public void testDelete() | |
| public void testRenameTable() | ||
| { | ||
| // Iceberg table rename is not supported in FileHiveMetastore | ||
| // TODO add a test with a different metastore, or block rename in IcebergMetadata | ||
| assertThatThrownBy(super::testRenameTable) | ||
| .hasStackTraceContaining("Rename not supported for Iceberg tables"); | ||
| } | ||
|
|
@@ -203,6 +208,46 @@ public void testShowCreateTable() | |
| ")"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void checkInformationSchemaTablesForPointedQueryForMaterializedView(String schemaName, String viewName) | ||
| { | ||
| // TODO The query should not fail, obviously. It should return the viewName, as information_schema.tables returns it when invoked without filters | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue link
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raunaqmorarka is there an issue for fix Iceberg MV reporting?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW Do we have any roadmap issue for materialized views?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already have a fix for iceberg information_schema.tables listing in 5fff606 |
||
| assertThatThrownBy(() -> super.checkInformationSchemaTablesForPointedQueryForMaterializedView(schemaName, viewName)) | ||
| .hasMessageContaining("Not an Iceberg table"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void checkShowColumnsForMaterializedView(String viewName) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where are you overriding those?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @losipiuk in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @losipiuk sorry, i think i misunderstood the question. |
||
| { | ||
| // TODO The query should not fail, obviously. It should return all the columns in the view | ||
| assertThatThrownBy(() -> super.checkShowColumnsForMaterializedView(viewName)) | ||
| .hasMessageContaining("Not an Iceberg table"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void checkInformationSchemaColumnsForMaterializedView(String schemaName, String viewName) | ||
| { | ||
| // TODO The query should not fail, obviously. It should return columns for all tables, views, and materialized views | ||
| assertThatThrownBy(() -> super.checkInformationSchemaColumnsForMaterializedView(schemaName, viewName)) | ||
| .hasMessageFindingMatch("(?s)Expecting.*to contain:.*, nationkey\\).*, name\\).*, regionkey\\).*, comment\\)"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void checkInformationSchemaColumnsForPointedQueryForMaterializedView(String schemaName, String viewName) | ||
| { | ||
| // TODO The query should not fail, obviously. It should return columns for the materialized view | ||
| assertThatThrownBy(() -> super.checkInformationSchemaColumnsForPointedQueryForMaterializedView(schemaName, viewName)) | ||
| .hasMessageContaining("Not an Iceberg table"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void checkInformationSchemaViewsForMaterializedView(String schemaName, String viewName) | ||
| { | ||
| // TODO should probably return materialized view, as it's also a view -- to be double checked | ||
| assertThatThrownBy(() -> super.checkInformationSchemaViewsForMaterializedView(schemaName, viewName)) | ||
| .hasMessageFindingMatch("(?s)Expecting.*to contain:.*\\Q[(" + viewName + ")]"); | ||
| } | ||
|
|
||
| @Test | ||
| // This particular method may or may not be @Flaky. It is annotated since the problem is generic. | ||
| @Flaky(issue = "https://github.com/trinodb/trino/issues/5201", match = "Failed to read footer of file: HdfsInputFile") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.trino.plugin.iceberg; | ||
|
|
||
| import com.google.common.collect.ImmutableMap; | ||
| import io.trino.testing.BaseConnectorSmokeTest; | ||
| import io.trino.testing.QueryRunner; | ||
| import io.trino.testing.TestingConnectorBehavior; | ||
| import org.apache.iceberg.FileFormat; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| import static io.trino.plugin.iceberg.IcebergQueryRunner.createIcebergQueryRunner; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| // Redundant over TestIcebergOrcConnectorTest, but exists to exercise BaseConnectorSmokeTest | ||
| // Some features like materialized views may be supported by Iceberg only. | ||
| public class TestIcebergConnectorSmokeTest | ||
| extends BaseConnectorSmokeTest | ||
| { | ||
| @Override | ||
| protected QueryRunner createQueryRunner() | ||
| throws Exception | ||
| { | ||
| return createIcebergQueryRunner(ImmutableMap.of(), FileFormat.ORC, REQUIRED_TPCH_TABLES); | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | ||
| { | ||
| switch (connectorBehavior) { | ||
| case SUPPORTS_COMMENT_ON_COLUMN: | ||
| case SUPPORTS_RENAME_TABLE: | ||
| case SUPPORTS_TOPN_PUSHDOWN: | ||
| return false; | ||
|
|
||
| case SUPPORTS_CREATE_MATERIALIZED_VIEW: | ||
| return true; | ||
|
|
||
| case SUPPORTS_DELETE: | ||
| return true; | ||
| default: | ||
| return super.hasBehavior(connectorBehavior); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| @Override | ||
| public void testDelete() | ||
| { | ||
| // Deletes are covered AbstractTestIcebergConnectorTest | ||
| assertThatThrownBy(super::testDelete) | ||
| .hasStackTraceContaining("This connector only supports delete where one or more partitions are deleted entirely"); | ||
| } | ||
|
|
||
| @Test | ||
| @Override | ||
| public void testRenameTable() | ||
| { | ||
| // Iceberg table rename is not supported in FileHiveMetastore | ||
| // TODO add a test with a different metastore, or block rename in IcebergMetadata | ||
| assertThatThrownBy(super::testRenameTable) | ||
| .hasStackTraceContaining("Rename not supported for Iceberg tables"); | ||
| } | ||
|
|
||
| @Test | ||
| @Override | ||
| public void testShowCreateTable() | ||
| { | ||
| assertThat((String) computeScalar("SHOW CREATE TABLE region")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: cast to string is not needed |
||
| .isEqualTo("" + | ||
| "CREATE TABLE iceberg.tpch.region (\n" + | ||
| " regionkey bigint,\n" + | ||
| " name varchar,\n" + | ||
| " comment varchar\n" + | ||
| ")\n" + | ||
| "WITH (\n" + | ||
| " format = 'ORC'\n" + | ||
| ")"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: cast to string is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, but may help completion. i will keep for now, but we can remove as followup