Skip to content

Test code cleanup around testJoinPushdown #19998

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 8 commits into from
Dec 8, 2023
Merged

Test code cleanup around testJoinPushdown #19998

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3219ce5
Fix typo
findepi Dec 1, 2023
f6a53b9
Fix code indentation
findepi Dec 1, 2023
93236a5
Improve exceptions when test fails
findepi Dec 1, 2023
0a9f305
Use switch expression
findepi Dec 1, 2023
82a2d10
Fix erroneous returns after test parameters removed
findepi Dec 1, 2023
f381cf0
Simplify test methods around full join pushdown
findepi Dec 1, 2023
e708c5a
Create test table once in testJoinPushdown
findepi Dec 1, 2023
e94369a
Remove redundant test
findepi Dec 8, 2023
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
157 changes: 62 additions & 95 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,71 +1179,42 @@ public void testJoinPushdownDisabled()
.joinIsNotFullyPushedDown();
}

/**
* Verify !SUPPORTS_JOIN_PUSHDOWN declaration is true.
*/
@Test
public void verifySupportsJoinPushdownDeclaration()
public void testJoinPushdown()
{
if (hasBehavior(SUPPORTS_JOIN_PUSHDOWN)) {
// Covered by testJoinPushdown
return;
}
Session session = joinPushdownEnabled(getSession());

assertThat(query(joinPushdownEnabled(getSession()), "SELECT r.name, n.name FROM nation n JOIN region r ON n.regionkey = r.regionkey"))
.joinIsNotFullyPushedDown();
}

/**
* Verify !SUPPORTS_JOIN_PUSHDOWN_WITH_FULL_JOIN declaration is true.
*/
@Test
public void verifySupportsJoinPushdownWithFullJoinDeclaration()
{
if (hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_FULL_JOIN)) {
// Covered by testJoinPushdown
if (!hasBehavior(SUPPORTS_JOIN_PUSHDOWN)) {
assertThat(query(session, "SELECT r.name, n.name FROM nation n JOIN region r ON n.regionkey = r.regionkey"))
.joinIsNotFullyPushedDown();
return;
}

assertThat(query(joinPushdownEnabled(getSession()), "SELECT r.name, n.name FROM nation n FULL JOIN region r ON n.regionkey = r.regionkey"))
.joinIsNotFullyPushedDown();
}

@Test
public void testJoinPushdown()
{
for (JoinOperator joinOperator : JoinOperator.values()) {
Session session = joinPushdownEnabled(getSession());
try (TestTable nationLowercaseTable = new TestTable(
// If a connector supports Join pushdown, but does not allow CTAS, we need to make the table creation here overridable.
getQueryRunner()::execute,
"nation_lowercase",
"AS SELECT nationkey, lower(name) name, regionkey FROM nation")) {
for (JoinOperator joinOperator : JoinOperator.values()) {
if (joinOperator == FULL_JOIN && !hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_FULL_JOIN)) {
assertThat(query(session, "SELECT r.name, n.name FROM nation n FULL JOIN region r ON n.regionkey = r.regionkey"))
.joinIsNotFullyPushedDown();
continue;
}

if (!hasBehavior(SUPPORTS_JOIN_PUSHDOWN)) {
assertThat(query(session, "SELECT r.name, n.name FROM nation n JOIN region r ON n.regionkey = r.regionkey"))
.joinIsNotFullyPushedDown();
return;
}
// Disable DF here for the sake of negative test cases' expected plan. With DF enabled, some operators return in DF's FilterNode and some do not.
Session withoutDynamicFiltering = Session.builder(session)
.setSystemProperty("enable_dynamic_filtering", "false")
.build();

if (joinOperator == FULL_JOIN && !hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_FULL_JOIN)) {
// Covered by verifySupportsJoinPushdownWithFullJoinDeclaration
return;
}
String notDistinctOperator = "IS NOT DISTINCT FROM";
List<String> nonEqualities = Stream.concat(
Stream.of(JoinCondition.Operator.values())
.filter(operator -> operator != JoinCondition.Operator.EQUAL)
.map(JoinCondition.Operator::getValue),
Stream.of(notDistinctOperator))
.collect(toImmutableList());

// Disable DF here for the sake of negative test cases' expected plan. With DF enabled, some operators return in DF's FilterNode and some do not.
Session withoutDynamicFiltering = Session.builder(session)
.setSystemProperty("enable_dynamic_filtering", "false")
.build();

String notDistinctOperator = "IS NOT DISTINCT FROM";
List<String> nonEqualities = Stream.concat(
Stream.of(JoinCondition.Operator.values())
.filter(operator -> operator != JoinCondition.Operator.EQUAL)
.map(JoinCondition.Operator::getValue),
Stream.of(notDistinctOperator))
.collect(toImmutableList());

try (TestTable nationLowercaseTable = new TestTable(
// If a connector supports Join pushdown, but does not allow CTAS, we need to make the table creation here overridable.
getQueryRunner()::execute,
"nation_lowercase",
"AS SELECT nationkey, lower(name) name, regionkey FROM nation")) {
// basic case
assertThat(query(session, format("SELECT r.name, n.name FROM nation n %s region r ON n.regionkey = r.regionkey", joinOperator))).isFullyPushedDown();

Expand Down Expand Up @@ -1273,13 +1244,13 @@ public void testJoinPushdown()
assertJoinConditionallyPushedDown(
withoutDynamicFiltering,
format("SELECT r.name, n.name FROM nation n %s region r ON n.regionkey %s r.regionkey", joinOperator, operator),
expectJoinPushdown(operator) && expectJoinPushdowOnInequalityOperator(joinOperator));
expectJoinPushdown(operator) && expectJoinPushdownOnInequalityOperator(joinOperator));

// varchar inequality predicate
assertJoinConditionallyPushedDown(
withoutDynamicFiltering,
format("SELECT n.name, nl.name FROM nation n %s %s nl ON n.name %s nl.name", joinOperator, nationLowercaseTable.getName(), operator),
expectVarcharJoinPushdown(operator) && expectJoinPushdowOnInequalityOperator(joinOperator));
expectVarcharJoinPushdown(operator) && expectJoinPushdownOnInequalityOperator(joinOperator));
}

// inequality along with an equality, which constitutes an equi-condition and allows filter to remain as part of the Join
Expand Down Expand Up @@ -1373,11 +1344,17 @@ protected QueryAssert assertJoinConditionallyPushedDown(
@Language("SQL") String query,
boolean condition)
{
QueryAssert queryAssert = assertThat(query(session, query));
if (condition) {
return queryAssert.isFullyPushedDown();
try {
QueryAssert queryAssert = assertThat(query(session, query));
if (condition) {
return queryAssert.isFullyPushedDown();
}
return queryAssert.joinIsNotFullyPushedDown();
}
catch (Throwable e) {
e.addSuppressed(new Exception("Query: " + query));
throw e;
}
return queryAssert.joinIsNotFullyPushedDown();
}

protected void assertConditionallyOrderedPushedDown(
Expand All @@ -1386,12 +1363,18 @@ protected void assertConditionallyOrderedPushedDown(
boolean condition,
PlanMatchPattern otherwiseExpected)
{
QueryAssert queryAssert = assertThat(query(session, query)).ordered();
if (condition) {
queryAssert.isFullyPushedDown();
try {
QueryAssert queryAssert = assertThat(query(session, query)).ordered();
if (condition) {
queryAssert.isFullyPushedDown();
}
else {
queryAssert.isNotFullyPushedDown(otherwiseExpected);
}
}
else {
queryAssert.isNotFullyPushedDown(otherwiseExpected);
catch (Throwable e) {
e.addSuppressed(new Exception("Query: " + query));
throw e;
}
}

Expand All @@ -1401,21 +1384,13 @@ protected boolean expectJoinPushdown(String operator)
// TODO (https://github.com/trinodb/trino/issues/6967) support join pushdown for IS NOT DISTINCT FROM
return false;
}
switch (toJoinConditionOperator(operator)) {
case EQUAL:
case NOT_EQUAL:
case LESS_THAN:
case LESS_THAN_OR_EQUAL:
case GREATER_THAN:
case GREATER_THAN_OR_EQUAL:
return true;
case IS_DISTINCT_FROM:
return hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_DISTINCT_FROM);
}
throw new AssertionError(); // unreachable
return switch (toJoinConditionOperator(operator)) {
case EQUAL, NOT_EQUAL, LESS_THAN, LESS_THAN_OR_EQUAL, GREATER_THAN, GREATER_THAN_OR_EQUAL -> true;
case IS_DISTINCT_FROM -> hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_DISTINCT_FROM);
};
}

protected boolean expectJoinPushdowOnInequalityOperator(JoinOperator joinOperator)
protected boolean expectJoinPushdownOnInequalityOperator(JoinOperator joinOperator)
{
// Currently no pushdown as inequality predicate is removed from Join to maintain Cross Join and Filter as separate nodes
return joinOperator != JOIN;
Expand All @@ -1427,19 +1402,11 @@ private boolean expectVarcharJoinPushdown(String operator)
// TODO (https://github.com/trinodb/trino/issues/6967) support join pushdown for IS NOT DISTINCT FROM
return false;
}
switch (toJoinConditionOperator(operator)) {
case EQUAL:
case NOT_EQUAL:
return hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_EQUALITY);
case LESS_THAN:
case LESS_THAN_OR_EQUAL:
case GREATER_THAN:
case GREATER_THAN_OR_EQUAL:
return hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_INEQUALITY);
case IS_DISTINCT_FROM:
return hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_DISTINCT_FROM) && hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_EQUALITY);
}
throw new AssertionError(); // unreachable
return switch (toJoinConditionOperator(operator)) {
case EQUAL, NOT_EQUAL -> hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_EQUALITY);
case LESS_THAN, LESS_THAN_OR_EQUAL, GREATER_THAN, GREATER_THAN_OR_EQUAL -> hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_INEQUALITY);
case IS_DISTINCT_FROM -> hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_DISTINCT_FROM) && hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_EQUALITY);
};
}

private JoinCondition.Operator toJoinConditionOperator(String operator)
Expand Down Expand Up @@ -2055,8 +2022,8 @@ public void testJoinPushdownWithLongIdentifiers()
try (TestTable left = new TestTable(getQueryRunner()::execute, "test_long_id_l", format("(%s BIGINT)", validColumnName));
TestTable right = new TestTable(getQueryRunner()::execute, "test_long_id_r", format("(%s BIGINT)", validColumnName))) {
assertThat(query(joinPushdownEnabled(getSession()), """
SELECT l.%1$s, r.%1$s
FROM %2$s l JOIN %3$s r ON l.%1$s = r.%1$s""".formatted(validColumnName, left.getName(), right.getName())))
SELECT l.%1$s, r.%1$s
FROM %2$s l JOIN %3$s r ON l.%1$s = r.%1$s""".formatted(validColumnName, left.getName(), right.getName())))
.isFullyPushedDown();
}
}
Expand Down