Skip to content

Commit

Permalink
Remove redundant flexible assertions
Browse files Browse the repository at this point in the history
  • Loading branch information
@findepi
findepi committed Nov 23, 2021
1 parent c68bda7 commit 2e35fbe
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 149 deletions.
63 changes: 3 additions & 60 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import io.airlift.json.ObjectMapperProvider;
import io.airlift.units.DataSize;
import io.airlift.units.DataSize.Unit;
import io.airlift.units.Duration;
import io.trino.Session;
import io.trino.connector.CatalogName;
import io.trino.cost.StatsAndCosts;
Expand Down Expand Up @@ -159,7 +158,6 @@
import static java.util.Locale.ENGLISH;
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toSet;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -4617,18 +4615,13 @@ private void doTestParquetTimestampPredicatePushdown(Session baseSession, HiveTi
format("SELECT * FROM %s WHERE t > %s", tableName, formatTimestamp(value)));
assertEquals(getQueryInfo(queryRunner, queryResult).getQueryStats().getProcessedInputDataSize().toBytes(), 0);

// TODO: replace this with a simple query stats check once we find a way to wait until all pending updates to query stats have been applied
// (might be fixed by https://github.com/trinodb/trino/issues/5172)
ExponentialSleeper sleeper = new ExponentialSleeper();
assertQueryStats(
session,
format("SELECT * FROM %s WHERE t = %s", tableName, formatTimestamp(value)),
queryStats -> {
sleeper.sleep();
assertThat(queryStats.getProcessedInputDataSize().toBytes()).isGreaterThan(0);
},
results -> {},
new Duration(30, SECONDS));
results -> {});
}

@Test(dataProvider = "timestampPrecisionAndValues")
Expand All @@ -4655,18 +4648,13 @@ public void testOrcTimestampPredicatePushdown(HiveTimestampPrecision timestampPr

assertQuery(session, "SELECT * FROM test_orc_timestamp_predicate_pushdown WHERE t < " + formatTimestamp(value.plusNanos(1)), format("VALUES (%s)", formatTimestamp(value)));

// TODO: replace this with a simple query stats check once we find a way to wait until all pending updates to query stats have been applied
// (might be fixed by https://github.com/trinodb/trino/issues/5172)
ExponentialSleeper sleeper = new ExponentialSleeper();
assertQueryStats(
session,
format("SELECT * FROM test_orc_timestamp_predicate_pushdown WHERE t = %s", formatTimestamp(value)),
queryStats -> {
sleeper.sleep();
assertThat(queryStats.getProcessedInputDataSize().toBytes()).isGreaterThan(0);
},
results -> {},
new Duration(30, SECONDS));
results -> {});
}

private static String formatTimestamp(LocalDateTime timestamp)
Expand Down Expand Up @@ -4734,8 +4722,7 @@ private void assertNoDataRead(@Language("SQL") String sql)
getSession(),
sql,
queryStats -> assertThat(queryStats.getProcessedInputDataSize().toBytes()).isEqualTo(0),
results -> assertThat(results.getRowCount()).isEqualTo(0),
new Duration(5, SECONDS));
results -> assertThat(results.getRowCount()).isEqualTo(0));
}

private QueryInfo getQueryInfo(DistributedQueryRunner queryRunner, ResultWithQueryId<MaterializedResult> queryResult)
Expand Down Expand Up @@ -8469,50 +8456,6 @@ public TypeAndEstimate(Type type, EstimatedStatsAndCost estimate)
}
}

private static class ExponentialSleeper
{
private Duration nextSleepTime;
private final Duration maxSleepTime;
private final Duration minSleepIncrement;
private final double sleepIncrementFactor;

ExponentialSleeper(Duration minSleepTime, Duration maxSleepTime, Duration minSleepIncrement, double sleepIncrementFactor)
{
this.nextSleepTime = minSleepTime;
this.maxSleepTime = maxSleepTime;
this.minSleepIncrement = minSleepIncrement;
this.sleepIncrementFactor = sleepIncrementFactor;
}

ExponentialSleeper()
{
this(
new Duration(0, SECONDS),
new Duration(5, SECONDS),
new Duration(100, MILLISECONDS),
2.0);
}

public void sleep()
{
try {
Thread.sleep(nextSleepTime.toMillis());
long incrementMillis = (long) (nextSleepTime.toMillis() * sleepIncrementFactor - nextSleepTime.toMillis());
if (incrementMillis < minSleepIncrement.toMillis()) {
incrementMillis = minSleepIncrement.toMillis();
}
nextSleepTime = new Duration(nextSleepTime.toMillis() + incrementMillis, MILLISECONDS);
if (nextSleepTime.compareTo(maxSleepTime) > 0) {
nextSleepTime = maxSleepTime;
}
}
catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(e);
}
}
}

@DataProvider
public Object[][] timestampPrecision()
{
Expand Down
8 changes: 2 additions & 6 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestParquetPageSkipping.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.plugin.hive;

import com.google.common.collect.ImmutableMap;
import io.airlift.units.Duration;
import io.trino.Session;
import io.trino.testing.AbstractTestQueryFramework;
import io.trino.testing.MaterializedResult;
Expand All @@ -26,7 +25,6 @@
import static io.trino.testing.QueryAssertions.assertEqualsIgnoreOrder;
import static io.trino.testing.sql.TestTable.randomTableSuffix;
import static java.lang.String.format;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.assertj.core.api.Assertions.assertThat;

public class TestParquetPageSkipping
Expand Down Expand Up @@ -174,8 +172,7 @@ private void assertRowGroupPruning(@Language("SQL") String sql)
assertThat(queryStats.getPhysicalInputPositions()).isGreaterThan(0);
assertThat(queryStats.getProcessedInputPositions()).isEqualTo(queryStats.getPhysicalInputPositions());
},
results -> assertThat(results.getRowCount()).isEqualTo(0),
new Duration(10, SECONDS));
results -> assertThat(results.getRowCount()).isEqualTo(0));

assertQueryStats(
getSession(),
Expand All @@ -184,8 +181,7 @@ private void assertRowGroupPruning(@Language("SQL") String sql)
assertThat(queryStats.getPhysicalInputPositions()).isEqualTo(0);
assertThat(queryStats.getProcessedInputPositions()).isEqualTo(0);
},
results -> assertThat(results.getRowCount()).isEqualTo(0),
new Duration(10, SECONDS));
results -> assertThat(results.getRowCount()).isEqualTo(0));
}

@DataProvider
Expand Down
78 changes: 29 additions & 49 deletions plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import io.airlift.units.DataSize;
import io.airlift.units.Duration;
import io.trino.Session;
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
Expand All @@ -43,7 +42,6 @@
import io.trino.testing.ResultWithQueryId;
import io.trino.testing.TestingConnectorBehavior;
import io.trino.testing.sql.TestTable;
import io.trino.testng.services.Flaky;
import io.trino.tpch.TpchTable;
import org.apache.avro.Schema;
import org.apache.avro.file.DataFileReader;
Expand All @@ -66,7 +64,6 @@
import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import java.util.function.Predicate;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -105,7 +102,6 @@
import static java.lang.String.join;
import static java.util.Collections.nCopies;
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.joining;
import static java.util.stream.IntStream.range;
import static org.apache.iceberg.FileFormat.ORC;
Expand Down Expand Up @@ -2420,7 +2416,6 @@ public void testIncorrectIcebergFileSizes()
}

@Test
@Flaky(issue = "https://github.com/trinodb/trino/issues/5172", match = "Couldn't find operator summary, probably due to query statistic collection error")
public void testSplitPruningForFilterOnPartitionColumn()
{
String tableName = "nation_partitioned_pruning";
Expand Down Expand Up @@ -2719,22 +2714,19 @@ public void testLocalDynamicFilteringWithSelectiveBuildSizeJoin()
.setSystemProperty(JOIN_DISTRIBUTION_TYPE, FeaturesConfig.JoinDistributionType.BROADCAST.name())
.build();

// TODO (https://github.com/trinodb/trino/issues/5172): remove assertEventually
assertEventually(() -> {
ResultWithQueryId<MaterializedResult> result = getDistributedQueryRunner().executeWithQueryId(
session,
"SELECT * FROM lineitem JOIN orders ON lineitem.orderkey = orders.orderkey AND orders.totalprice = 974.04");
assertEquals(result.getResult().getRowCount(), 1);
ResultWithQueryId<MaterializedResult> result = getDistributedQueryRunner().executeWithQueryId(
session,
"SELECT * FROM lineitem JOIN orders ON lineitem.orderkey = orders.orderkey AND orders.totalprice = 974.04");
assertEquals(result.getResult().getRowCount(), 1);

OperatorStats probeStats = searchScanFilterAndProjectOperatorStats(
result.getQueryId(),
new QualifiedObjectName(ICEBERG_CATALOG, "tpch", "lineitem"));
OperatorStats probeStats = searchScanFilterAndProjectOperatorStats(
result.getQueryId(),
new QualifiedObjectName(ICEBERG_CATALOG, "tpch", "lineitem"));

// Assert no split level pruning occurs. If this starts failing a new totalprice may need to be selected
assertThat(probeStats.getTotalDrivers()).isEqualTo(numberOfFiles);
// Assert some lineitem rows were filtered out on file level
assertThat(probeStats.getInputPositions()).isLessThan(fullTableScan);
});
// Assert no split level pruning occurs. If this starts failing a new totalprice may need to be selected
assertThat(probeStats.getTotalDrivers()).isEqualTo(numberOfFiles);
// Assert some lineitem rows were filtered out on file level
assertThat(probeStats.getInputPositions()).isLessThan(fullTableScan);
}

@Test(dataProvider = "repartitioningDataProvider")
Expand Down Expand Up @@ -2854,7 +2846,6 @@ private void testRepartitionData(Session session, String sourceRelation, boolean
}

@Test(dataProvider = "testDataMappingSmokeTestDataProvider")
@Flaky(issue = "https://github.com/trinodb/trino/issues/5172", match = "Couldn't find operator summary, probably due to query statistic collection error")
public void testSplitPruningForFilterOnNonPartitionColumn(DataMappingTestSetup testSetup)
{
if (testSetup.isUnsupportedType()) {
Expand Down Expand Up @@ -2911,34 +2902,26 @@ public void testSplitPruningFromDataFileStatistics(DataMappingTestSetup testSetu

private void verifySplitCount(String query, int expectedSplitCount)
{
// using assertEventually here as the operator stats mechanism is known to be best-effort and some late-stage stats are sometimes not recorded
// (see https://github.com/trinodb/trino/issues/5172)
assertEventually(new Duration(10, SECONDS), () -> {
ResultWithQueryId<MaterializedResult> selectAllPartitionsResult = getDistributedQueryRunner().executeWithQueryId(getSession(), query);
assertEqualsIgnoreOrder(selectAllPartitionsResult.getResult().getMaterializedRows(), computeActual(withoutPredicatePushdown(getSession()), query).getMaterializedRows());
verifySplitCount(selectAllPartitionsResult.getQueryId(), expectedSplitCount);
});
ResultWithQueryId<MaterializedResult> selectAllPartitionsResult = getDistributedQueryRunner().executeWithQueryId(getSession(), query);
assertEqualsIgnoreOrder(selectAllPartitionsResult.getResult().getMaterializedRows(), computeActual(withoutPredicatePushdown(getSession()), query).getMaterializedRows());
verifySplitCount(selectAllPartitionsResult.getQueryId(), expectedSplitCount);
}

private void verifyPredicatePushdownDataRead(@Language("SQL") String query, boolean supportsPushdown)
{
// using assertEventually here as the operator stats mechanism is known to be best-effort and some late-stage stats are sometimes not recorded
// (see https://github.com/trinodb/trino/issues/5172)
assertEventually(new Duration(10, TimeUnit.SECONDS), () -> {
ResultWithQueryId<MaterializedResult> resultWithPredicatePushdown = getDistributedQueryRunner().executeWithQueryId(getSession(), query);
ResultWithQueryId<MaterializedResult> resultWithoutPredicatePushdown = getDistributedQueryRunner().executeWithQueryId(
withoutPredicatePushdown(getSession()),
query);

DataSize withPushdownDataSize = getOperatorStats(resultWithPredicatePushdown.getQueryId()).getInputDataSize();
DataSize withoutPushdownDataSize = getOperatorStats(resultWithoutPredicatePushdown.getQueryId()).getInputDataSize();
if (supportsPushdown) {
assertThat(withPushdownDataSize).isLessThan(withoutPushdownDataSize);
}
else {
assertThat(withPushdownDataSize).isEqualTo(withoutPushdownDataSize);
}
});
ResultWithQueryId<MaterializedResult> resultWithPredicatePushdown = getDistributedQueryRunner().executeWithQueryId(getSession(), query);
ResultWithQueryId<MaterializedResult> resultWithoutPredicatePushdown = getDistributedQueryRunner().executeWithQueryId(
withoutPredicatePushdown(getSession()),
query);

DataSize withPushdownDataSize = getOperatorStats(resultWithPredicatePushdown.getQueryId()).getInputDataSize();
DataSize withoutPushdownDataSize = getOperatorStats(resultWithoutPredicatePushdown.getQueryId()).getInputDataSize();
if (supportsPushdown) {
assertThat(withPushdownDataSize).isLessThan(withoutPushdownDataSize);
}
else {
assertThat(withPushdownDataSize).isEqualTo(withoutPushdownDataSize);
}
}

private Session withoutPredicatePushdown(Session session)
Expand Down Expand Up @@ -3152,7 +3135,6 @@ public void testProjectionPushdownReadsLessData()
Session sessionWithoutPushdown = Session.builder(getSession())
.setCatalogSessionProperty(ICEBERG_CATALOG, "projection_pushdown_enabled", "false")
.build();
Duration timeout = new Duration(10, SECONDS);

assertQueryStats(
getSession(),
Expand All @@ -3163,11 +3145,9 @@ public void testProjectionPushdownReadsLessData()
sessionWithoutPushdown,
selectQuery,
statsWithoutPushdown -> assertThat(statsWithoutPushdown.getProcessedInputDataSize()).isGreaterThan(processedDataSizeWithPushdown),
results -> assertEquals(results.getOnlyColumnAsSet(), expected),
timeout);
results -> assertEquals(results.getOnlyColumnAsSet(), expected));
},
results -> assertEquals(results.getOnlyColumnAsSet(), expected),
timeout);
results -> assertEquals(results.getOnlyColumnAsSet(), expected));
}
finally {
assertUpdate("DROP TABLE IF EXISTS projection_pushdown_reads_less_data");
Expand Down
1 change: 0 additions & 1 deletion plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,6 @@ public void testSemiJoinDynamicFilteringBlockProbeSide(JoinDistributionType join
}

@Test
@Flaky(issue = "https://github.com/trinodb/trino/issues/5172", match = "Lists differ at element")
public void testCrossJoinDynamicFiltering()
{
assertUpdate("DROP TABLE IF EXISTS probe");
Expand Down
28 changes: 15 additions & 13 deletions testing/trino-testing/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,6 @@
<artifactId>trino-spi</artifactId>
</dependency>

<dependency>
<groupId>io.trino</groupId>
<artifactId>trino-testing-services</artifactId>
<exclusions>
<exclusion>
<!-- conflicts with test-scoped dependency declared below -->
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-core</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
<groupId>io.trino</groupId>
<artifactId>trino-tpch</artifactId>
Expand Down Expand Up @@ -161,7 +149,21 @@
<artifactId>testng</artifactId>
</dependency>

<!-- used by tests but also needed transitively -->
<dependency>
<!-- trino-testing is on test classpath of many modules. It's important to pull trino-testing-services as a dependency,
because trino-testing-services includes various test-related checkers -->
<groupId>io.trino</groupId>
<artifactId>trino-testing-services</artifactId>
<scope>runtime</scope>
<exclusions>
<exclusion>
<!-- conflicts with test-scoped dependency declared below -->
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-core</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
<groupId>io.airlift</groupId>
<artifactId>log-manager</artifactId>
Expand Down
2 changes: 0 additions & 2 deletions testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import io.trino.execution.QueryManager;
import io.trino.server.BasicQueryInfo;
import io.trino.testing.sql.TestTable;
import io.trino.testng.services.Flaky;
import org.intellij.lang.annotations.Language;
import org.testng.SkipException;
import org.testng.annotations.DataProvider;
Expand Down Expand Up @@ -1084,7 +1083,6 @@ public void testSymbolAliasing()
}

@Test
@Flaky(issue = "https://github.com/trinodb/trino/issues/5172", match = "AssertionError: expected \\[.*\\] but found \\[.*\\]")
public void testWrittenStats()
{
skipTestUnless(supportsCreateTable());
Expand Down
2 changes: 0 additions & 2 deletions testing/trino-testing/src/main/java/io/trino/testing/AbstractTestJoinQueries.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import io.trino.operator.OperatorStats;
import io.trino.spi.type.Decimals;
import io.trino.sql.analyzer.FeaturesConfig;
import io.trino.testng.services.Flaky;
import io.trino.tests.QueryTemplate;
import io.trino.tpch.TpchTable;
import org.intellij.lang.annotations.Language;
Expand Down Expand Up @@ -2303,7 +2302,6 @@ public void testMultiJoinWithEligibleForDynamicFiltering()
}

@Test
@Flaky(issue = "https://github.com/trinodb/trino/issues/5172", match = ".*expected.*but found.*")
public void testOutputDuplicatesInsensitiveJoin()
{
assertJoinOutputPositions(
Expand Down
Loading

0 comments on commit 2e35fbe

Please sign in to comment.