Skip to content

Add reusable smoke test coverage for UPDATE #10355

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.

Sign up for GitHub

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

Merged
findepi merged 1 commit into from
Jan 5, 2022
Merged

Add reusable smoke test coverage for UPDATE #10355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
case SUPPORTS_DELETE:
return true;

case SUPPORTS_UPDATE:
return true;

case SUPPORTS_MULTI_STATEMENT_WRITES:
return true;

Expand All @@ -63,6 +66,13 @@ public void testRowLevelDelete()
.hasMessage("Deletes must match whole partitions for non-transactional tables");
}

@Override
public void testUpdate()
{
assertThatThrownBy(super::testUpdate)
.hasMessage("Hive update is only supported for ACID transactional tables");
}

@Test
@Override
public void testShowCreateTable()
Expand Down
10 changes: 10 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
case SUPPORTS_DELETE:
return true;

case SUPPORTS_UPDATE:
return true;

case SUPPORTS_MULTI_STATEMENT_WRITES:
return true;

Expand Down Expand Up @@ -258,6 +261,13 @@ public void testDeleteWithSubquery()
.hasStackTraceContaining("Deletes must match whole partitions for non-transactional tables");
}

@Override
public void testUpdate()
{
assertThatThrownBy(super::testUpdate)
.hasMessage("Hive update is only supported for ACID transactional tables");
}

@Override
public void testExplainAnalyzeWithDeleteWithSubquery()
{
Expand Down
19 changes: 19 additions & 0 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_RENAME_TABLE;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_RENAME_TABLE_ACROSS_SCHEMAS;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ROW_LEVEL_DELETE;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_UPDATE;
import static io.trino.testing.sql.TestTable.randomTableSuffix;
import static io.trino.tpch.TpchTable.NATION;
import static io.trino.tpch.TpchTable.REGION;
Expand Down Expand Up @@ -221,6 +222,24 @@ public void testRowLevelDelete()
}
}

@Test
public void testUpdate()
{
if (!hasBehavior(SUPPORTS_UPDATE)) {
// Note this change is a no-op, if actually run
assertQueryFails("UPDATE nation SET nationkey = nationkey + regionkey WHERE regionkey < 1", "This connector does not support updates");
Copy link
Copy Markdown
Member

@hashhar hashhar Dec 28, 2021

Choose a reason for hiding this comment

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

Can we in future omit such no-op operations in some connectors (hence silently breaking the test)?

Copy link
Copy Markdown
Member

@losipiuk losipiuk Dec 28, 2021

Choose a reason for hiding this comment

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

I am not a big fan on testing that on a logically immutable table. If update has bug and actually mutates the table we will get weird test failures in a very unrelated place

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 4, 2022

Choose a reason for hiding this comment

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

Can we in future omit such no-op operations in some connectors (hence silently breaking the test)?

@hashhar That would require implementing some form of UPDATE support.

If update has bug and actually mutates the table

@losipiuk in the worst case, the test env needs a fix. I don't think it's a big deal.

return;
}

try (TestTable table = new TestTable(getQueryRunner()::execute, "test_update", "AS TABLE tpch.tiny.nation")) {
String tableName = table.getName();
assertUpdate("UPDATE " + tableName + " SET nationkey = 100 + nationkey WHERE regionkey = 2", 5);
assertThat(query("SELECT * FROM " + tableName))
.skippingTypesCheck()
.matches("SELECT IF(regionkey=2, nationkey + 100, nationkey) nationkey, name, regionkey, comment FROM tpch.tiny.nation");
}
}

@Test
public void testCreateSchema()
{
Expand Down
25 changes: 25 additions & 0 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ROW_LEVEL_DELETE;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_TOPN_PUSHDOWN;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_TRUNCATE;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_UPDATE;
import static io.trino.testing.assertions.Assert.assertEquals;
import static io.trino.testing.sql.TestTable.randomTableSuffix;
import static java.lang.String.format;
Expand Down Expand Up @@ -1255,6 +1256,30 @@ public void testRowLevelDelete()
}
}

@Test
public void testUpdate()
{
if (!hasBehavior(SUPPORTS_UPDATE)) {
// Note this change is a no-op, if actually run
assertQueryFails("UPDATE nation SET nationkey = nationkey + regionkey WHERE regionkey < 1", "This connector does not support updates");
return;
}

try (TestTable table = new TestTable(getQueryRunner()::execute, "test_update", "AS TABLE tpch.tiny.nation")) {
String tableName = table.getName();
assertUpdate("UPDATE " + tableName + " SET nationkey = 100 + nationkey WHERE regionkey = 2", 5);
assertThat(query("SELECT * FROM " + tableName))
.skippingTypesCheck()
.matches("SELECT IF(regionkey=2, nationkey + 100, nationkey) nationkey, name, regionkey, comment FROM tpch.tiny.nation");

// UPDATE after UPDATE
assertUpdate("UPDATE " + tableName + " SET nationkey = nationkey * 2 WHERE regionkey IN (2,3)", 10);
assertThat(query("SELECT * FROM " + tableName))
.skippingTypesCheck()
.matches("SELECT CASE regionkey WHEN 2 THEN 2*(nationkey+100) WHEN 3 THEN 2*nationkey ELSE nationkey END nationkey, name, regionkey, comment FROM tpch.tiny.nation");
}
}

@Test
public void testTruncateTable()
{
Expand Down
2 changes: 2 additions & 0 deletions testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ public enum TestingConnectorBehavior
SUPPORTS_DELETE(false),
SUPPORTS_ROW_LEVEL_DELETE(SUPPORTS_DELETE),

SUPPORTS_UPDATE(false),

SUPPORTS_TRUNCATE(false),

SUPPORTS_ARRAY,
Expand Down