Skip to content

Commit

Permalink
Avoid positively depending on query assert constructor side effect
Browse files Browse the repository at this point in the history
Update places that depended on `query(...)` query assert constructor
executing the query. These should use `execute`, `assertUpdate`, etc.
  • Loading branch information
findepi committed Jan 31, 2024
1 parent baaef6d commit 1924263
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.errorprone.annotations.CheckReturnValue;
import io.trino.Session;
import io.trino.execution.warnings.WarningCollector;
import io.trino.metadata.FunctionBundle;
Expand Down Expand Up @@ -119,11 +120,13 @@ public void addPlugin(Plugin plugin)
runner.installPlugin(plugin);
}

@CheckReturnValue
public AssertProvider<QueryAssert> query(@Language("SQL") String query)
{
return query(runner.getDefaultSession(), query);
}

@CheckReturnValue
public AssertProvider<QueryAssert> query(Session session, @Language("SQL") String query)
{
return newQueryAssert(query, runner, session);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ public void testUnnestArrays()
public void testNullRows()
{
// This query tries to simulate testArrayOfRowsUnnesterWithNulls e2e
assertions.query("SELECT "
assertions.execute("SELECT "
+ " x, y "
+ "FROM "
+ " (VALUES "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,25 +257,25 @@ public void testDropSchemaExternalFiles()
hiveHadoop.executeInContainerFailOnError("hdfs", "dfs", "-mkdir", "-p", subDir);
hiveHadoop.executeInContainerFailOnError("hdfs", "dfs", "-touchz", externalFile);

query(format("CREATE SCHEMA %s WITH (location = '%s')", schemaName, schemaDir));
assertUpdate(format("CREATE SCHEMA %s WITH (location = '%s')", schemaName, schemaDir));
assertThat(hiveHadoop.executeInContainer("hdfs", "dfs", "-test", "-e", externalFile).getExitCode())
.as("external file exists after creating schema")
.isEqualTo(0);

query("DROP SCHEMA " + schemaName);
assertUpdate("DROP SCHEMA " + schemaName);
assertThat(hiveHadoop.executeInContainer("hdfs", "dfs", "-test", "-e", externalFile).getExitCode())
.as("external file exists after dropping schema")
.isEqualTo(0);

// Test behavior without external file
hiveHadoop.executeInContainerFailOnError("hdfs", "dfs", "-rm", "-r", subDir);

query(format("CREATE SCHEMA %s WITH (location = '%s')", schemaName, schemaDir));
assertUpdate(format("CREATE SCHEMA %s WITH (location = '%s')", schemaName, schemaDir));
assertThat(hiveHadoop.executeInContainer("hdfs", "dfs", "-test", "-d", schemaDir).getExitCode())
.as("schema directory exists after creating schema")
.isEqualTo(0);

query("DROP SCHEMA " + schemaName);
assertUpdate("DROP SCHEMA " + schemaName);
assertThat(hiveHadoop.executeInContainer("hdfs", "dfs", "-test", "-e", externalFile).getExitCode())
.as("schema directory deleted after dropping schema without external file")
.isEqualTo(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ public void testTimeTravelWithRedirection()
"\\QThis connector does not support versioned tables");
}
finally {
query("DROP TABLE IF EXISTS iceberg." + testLocalSchema + ".nation_test");
query("DROP SCHEMA IF EXISTS iceberg." + testLocalSchema);
assertUpdate("DROP TABLE IF EXISTS iceberg." + testLocalSchema + ".nation_test");
assertUpdate("DROP SCHEMA IF EXISTS iceberg." + testLocalSchema);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ public void testOptimizingV2TableRemovesEqualityDeletesWhenWholeTableIsScanned()
assertThat(icebergTable.currentSnapshot().summary()).containsEntry("total-equality-deletes", "0");
writeEqualityDeleteToNationTable(icebergTable, Optional.of(icebergTable.spec()), Optional.of(new PartitionData(new Long[] {1L})));
List<String> initialActiveFiles = getActiveFiles(tableName);
query("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
assertUpdate("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1");
// nationkey is before the equality delete column in the table schema, comment is after
assertQuery("SELECT nationkey, comment FROM " + tableName, "SELECT nationkey, comment FROM nation WHERE regionkey != 1");
Expand All @@ -268,7 +268,7 @@ public void testOptimizingV2TableDoesntRemoveEqualityDeletesWhenOnlyPartOfTheTab
assertThat(icebergTable.currentSnapshot().summary()).containsEntry("total-equality-deletes", "0");
List<String> initialActiveFiles = getActiveFiles(tableName);
writeEqualityDeleteToNationTable(icebergTable, Optional.of(icebergTable.spec()), Optional.of(new PartitionData(new Long[] {1L})));
query("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE WHERE regionkey != 1");
assertUpdate("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE WHERE regionkey != 1");
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1");
// nationkey is before the equality delete column in the table schema, comment is after
assertQuery("SELECT nationkey, comment FROM " + tableName, "SELECT nationkey, comment FROM nation WHERE regionkey != 1");
Expand All @@ -285,7 +285,7 @@ public void testSelectivelyOptimizingLeavesEqualityDeletes()
assertUpdate("CREATE TABLE " + tableName + " WITH (partitioning = ARRAY['nationkey']) AS SELECT * FROM tpch.tiny.nation", 25);
Table icebergTable = loadTable(tableName);
writeEqualityDeleteToNationTable(icebergTable, Optional.of(icebergTable.spec()), Optional.of(new PartitionData(new Long[] {1L})));
query("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE WHERE nationkey < 5");
assertUpdate("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE WHERE nationkey < 5");
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1 OR nationkey != 1");
assertThat(loadTable(tableName).currentSnapshot().summary()).containsEntry("total-equality-deletes", "1");
}
Expand Down Expand Up @@ -421,7 +421,7 @@ public void testOptimizingWholeTableRemovesEqualityDeletes()
assertUpdate("CREATE TABLE " + tableName + " WITH (partitioning = ARRAY['nationkey']) AS SELECT * FROM tpch.tiny.nation", 25);
Table icebergTable = loadTable(tableName);
writeEqualityDeleteToNationTable(icebergTable, Optional.of(icebergTable.spec()), Optional.of(new PartitionData(new Long[] {1L})));
query("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
assertUpdate("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1 OR nationkey != 1");
assertThat(loadTable(tableName).currentSnapshot().summary()).containsEntry("total-equality-deletes", "0");
}
Expand All @@ -436,7 +436,7 @@ public void testOptimizingV2TableWithEmptyPartitionSpec()
assertThat(icebergTable.currentSnapshot().summary()).containsEntry("total-equality-deletes", "0");
writeEqualityDeleteToNationTable(icebergTable);
List<String> initialActiveFiles = getActiveFiles(tableName);
query("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
assertUpdate("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1");
// nationkey is before the equality delete column in the table schema, comment is after
assertQuery("SELECT nationkey, comment FROM " + tableName, "SELECT nationkey, comment FROM nation WHERE regionkey != 1");
Expand All @@ -456,7 +456,7 @@ public void testOptimizingPartitionsOfV2TableWithGlobalEqualityDeleteFile()
writeEqualityDeleteToNationTable(icebergTable, Optional.of(icebergTable.spec()), Optional.of(new PartitionData(new Long[] {1L})));
List<String> initialActiveFiles = getActiveFiles(tableName);
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1");
query("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE WHERE regionkey != 1");
assertUpdate("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE WHERE regionkey != 1");
assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation WHERE regionkey != 1");
// nationkey is before the equality delete column in the table schema, comment is after
assertQuery("SELECT nationkey, comment FROM " + tableName, "SELECT nationkey, comment FROM nation WHERE regionkey != 1");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public void testMetadataOnlyQueries()
ExecutorService backgroundExecutor = newCachedThreadPool();
try {
backgroundExecutor.submit(() -> {
query(highTaskMemorySession, slowQuery);
assertUpdate(highTaskMemorySession, slowQuery);
});
assertEventually(() -> queryIsInState(slowQuery, QueryState.RUNNING));

Expand All @@ -158,7 +158,7 @@ public void testMetadataOnlyQueries()

// check non-metadata queries still wait for resources
backgroundExecutor.submit(() -> {
query(nonMetadataQuery);
assertUpdate(nonMetadataQuery);
});
assertEventually(() -> queryIsInState(nonMetadataQuery, QueryState.STARTING));
Thread.sleep(1000); // wait a bit longer and query should be still STARTING
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.MoreCollectors;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.CheckReturnValue;
import io.airlift.log.Level;
import io.airlift.log.Logging;
import io.airlift.units.Duration;
Expand Down Expand Up @@ -323,11 +324,13 @@ protected Object computeScalar(Session session, @Language("SQL") String sql)
return computeActual(session, sql).getOnlyValue();
}

@CheckReturnValue
protected AssertProvider<QueryAssert> query(@Language("SQL") String sql)
{
return query(getSession(), sql);
}

@CheckReturnValue
protected AssertProvider<QueryAssert> query(Session session, @Language("SQL") String sql)
{
return queryAssertions.query(session, sql);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ private void testValidGrantWithGrantOption(String privilege)
queryRunner.execute(admin, format("GRANT %s ON SCHEMA default TO %s WITH GRANT OPTION", privilege, username));

assertThat(assertions.query(user, "SHOW SCHEMAS FROM local")).matches("VALUES (VARCHAR 'information_schema'), (VARCHAR 'default')");
assertions.query(user, format("GRANT %s ON SCHEMA default TO %s", privilege, randomUsername()));
assertions.query(user, format("GRANT %s ON SCHEMA default TO %s WITH GRANT OPTION", privilege, randomUsername()));
assertions.execute(user, format("GRANT %s ON SCHEMA default TO %s", privilege, randomUsername()));
assertions.execute(user, format("GRANT %s ON SCHEMA default TO %s WITH GRANT OPTION", privilege, randomUsername()));
}

@Test
Expand Down

0 comments on commit 1924263

Please sign in to comment.