From 3219ce5ebd8fc7e337e0ebcaec8752c40b9078ba Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 17:44:10 +0100 Subject: [PATCH 1/8] Fix typo --- .../java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index e2ea22c7d46a..e89e35ef453f 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1273,13 +1273,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 @@ -1415,7 +1415,7 @@ protected boolean expectJoinPushdown(String operator) throw new AssertionError(); // unreachable } - 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; From f6a53b99a2c62871b634c2ee8071001f9cc0241e Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 15:26:47 +0100 Subject: [PATCH 2/8] Fix code indentation --- .../test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index e89e35ef453f..27ea0e6544e3 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -2055,8 +2055,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(); } } From 93236a5eb4f6689b19bab357acdfb0f28e8db4c5 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 17:55:02 +0100 Subject: [PATCH 3/8] Improve exceptions when test fails For a test code like this ``` for (String operator : nonEqualities) { assertJoinConditionallyPushedDown( ... ``` the raised exception would contain no information which case was the failing one. --- .../plugin/jdbc/BaseJdbcConnectorTest.java | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index 27ea0e6544e3..1c63e6b76231 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1373,11 +1373,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( @@ -1386,12 +1392,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; } } From 0a9f3055193038f0f030a9dd3b90f6959a587130 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 17:52:58 +0100 Subject: [PATCH 4/8] Use switch expression --- .../plugin/jdbc/BaseJdbcConnectorTest.java | 34 +++++-------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index 1c63e6b76231..09684a16e22d 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1413,18 +1413,10 @@ 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 expectJoinPushdownOnInequalityOperator(JoinOperator joinOperator) @@ -1439,19 +1431,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) From 82a2d102b51067614861792b733a33c25ccc3aae Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 18:07:39 +0100 Subject: [PATCH 5/8] Fix erroneous returns after test parameters removed Apparently a syntactic transformation was applied: from "with parameters invoke test method" to "test method with a for loop over parameters". This didn't account for return statement that could potentially lead to cutting off portion of test coverage. --- .../trino/plugin/jdbc/BaseJdbcConnectorTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index 09684a16e22d..a2b7d170f33b 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1212,18 +1212,18 @@ public void verifySupportsJoinPushdownWithFullJoinDeclaration() @Test public void testJoinPushdown() { - for (JoinOperator joinOperator : JoinOperator.values()) { - Session session = joinPushdownEnabled(getSession()); + Session session = joinPushdownEnabled(getSession()); - 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; - } + 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; + } + for (JoinOperator joinOperator : JoinOperator.values()) { if (joinOperator == FULL_JOIN && !hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_FULL_JOIN)) { // Covered by verifySupportsJoinPushdownWithFullJoinDeclaration - return; + continue; } // 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. From f381cf0b6e16415e77a907e5c7012a98d244315c Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 18:10:04 +0100 Subject: [PATCH 6/8] Simplify test methods around full join pushdown No need for separate method, if all testing is already covered by `testJoinPushdown`. --- .../plugin/jdbc/BaseJdbcConnectorTest.java | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index a2b7d170f33b..b38c6b95b626 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1194,21 +1194,6 @@ public void verifySupportsJoinPushdownDeclaration() .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 - 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() { @@ -1222,7 +1207,8 @@ public void testJoinPushdown() for (JoinOperator joinOperator : JoinOperator.values()) { if (joinOperator == FULL_JOIN && !hasBehavior(SUPPORTS_JOIN_PUSHDOWN_WITH_FULL_JOIN)) { - // Covered by verifySupportsJoinPushdownWithFullJoinDeclaration + assertThat(query(session, "SELECT r.name, n.name FROM nation n FULL JOIN region r ON n.regionkey = r.regionkey")) + .joinIsNotFullyPushedDown(); continue; } From e708c5a4cbf6774f99f7590cb68bc241e33cf99a Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 1 Dec 2023 18:11:08 +0100 Subject: [PATCH 7/8] Create test table once in testJoinPushdown There is no need to recreate the test table for overy of the `for (JoinOperator` loop runs. --- .../plugin/jdbc/BaseJdbcConnectorTest.java | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index b38c6b95b626..58f8b07d228f 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1205,31 +1205,31 @@ public void testJoinPushdown() return; } - 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; - } + 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; + } + + // 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 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 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(); From e94369a37c64bfa07f616b28b8a85d9f8400e03d Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Fri, 8 Dec 2023 13:48:31 +0100 Subject: [PATCH 8/8] Remove redundant test Verification that "!SUPPORTS_JOIN_PUSHDOWN => join pushdown does not occur" is covered by `testJoinPushdown`, so `verifySupportsJoinPushdownDeclaration` was redundant. --- .../trino/plugin/jdbc/BaseJdbcConnectorTest.java | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index 58f8b07d228f..97c158051b2b 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -1179,21 +1179,6 @@ public void testJoinPushdownDisabled() .joinIsNotFullyPushedDown(); } - /** - * Verify !SUPPORTS_JOIN_PUSHDOWN declaration is true. - */ - @Test - public void verifySupportsJoinPushdownDeclaration() - { - if (hasBehavior(SUPPORTS_JOIN_PUSHDOWN)) { - // Covered by testJoinPushdown - return; - } - - assertThat(query(joinPushdownEnabled(getSession()), "SELECT r.name, n.name FROM nation n JOIN region r ON n.regionkey = r.regionkey")) - .joinIsNotFullyPushedDown(); - } - @Test public void testJoinPushdown() {