Add support for TopN pushdown in JDBC connectors #6847

wendigo · 2021-02-08T12:03:55Z

Partially based on #4784.

The main difference is to use JdbcQueryRelationHandle, instead of carrying List<SortItem> in the JdbcTableHandle.

wendigo · 2021-02-08T12:33:45Z

trino:public> set session postgresql."topn_pushdown_enabled" = false;
SET SESSION
trino:public> SELECT * FROM orders_big ORDER by totalprice DESC LIMIT 10;
 orderkey | custkey | orderstatus | totalprice | orderdate  |  orderpriority  |      clerk      | shippriority |                                    comment
----------+---------+-------------+------------+------------+-----------------+-----------------+--------------+-------------------------------------------------------------------------------
  1750466 |   21433 | F           |  555285.16 | 1992-11-30 | 4-NOT SPECIFIED | Clerk#000000040 |            0 |  ironic packages. even notornis integrate. fluffily regular foxes snooze furi
  4722021 |  128120 | F           |  544089.09 | 1994-04-07 | 1-URGENT        | Clerk#000000230 |            0 | al, express pinto beans are after the careful
  3043270 |  144617 | O           |  530604.44 | 1997-02-12 | 5-LOW           | Clerk#000000699 |            0 | riously final deposits?
  4576548 |  108931 | O           |  525590.57 | 1997-12-26 | 1-URGENT        | Clerk#000000336 |            0 | he slyly ironic requests. regular, bold deposits cajol
  2232932 |   13940 | O           |  522720.61 | 1997-04-13 | 2-HIGH          | Clerk#000000245 |            0 | lithely stealthy accounts are slyly against the pend
  3586919 |   24049 | F           |  522644.48 | 1992-11-07 | 1-URGENT        | Clerk#000000924 |            0 | are alongside of the pending deposits. quick
  2199712 |   66790 | O           |  515531.82 | 1996-09-30 | 2-HIGH          | Clerk#000000650 |            0 |  the final, ironic deposits inte
  2185667 |   51796 | F           |  511359.88 | 1992-10-08 | 1-URGENT        | Clerk#000000574 |            0 | . deposits wake quickly unusual deposits. express dependencies wake.
  4515876 |  100685 | F           |   510061.6 | 1993-11-02 | 4-NOT SPECIFIED | Clerk#000000105 |            0 | carefully accounts: slyly ironic pinto beans detect. brave somas a
   972901 |  141100 | F           |  508668.52 | 1992-07-18 | 3-MEDIUM        | Clerk#000000667 |            0 | pinto beans according to the blithely express dolphins cajo
(10 rows)

Query 20210208_123038_00029_3ip8n, FINISHED, 2 nodes
Splits: 6 total, 6 done (100.00%)
12.27 [1.5M rows, 0B] [122K rows/s, 0B/s]

trino:public> set session postgresql."topn_pushdown_enabled" = true;
SET SESSION
trino:public> SELECT * FROM orders_big ORDER by totalprice DESC LIMIT 10;
 orderkey | custkey | orderstatus | totalprice | orderdate  |  orderpriority  |      clerk      | shippriority |                                    comment
----------+---------+-------------+------------+------------+-----------------+-----------------+--------------+-------------------------------------------------------------------------------
  1750466 |   21433 | F           |  555285.16 | 1992-11-30 | 4-NOT SPECIFIED | Clerk#000000040 |            0 |  ironic packages. even notornis integrate. fluffily regular foxes snooze furi
  4722021 |  128120 | F           |  544089.09 | 1994-04-07 | 1-URGENT        | Clerk#000000230 |            0 | al, express pinto beans are after the careful
  3043270 |  144617 | O           |  530604.44 | 1997-02-12 | 5-LOW           | Clerk#000000699 |            0 | riously final deposits?
  4576548 |  108931 | O           |  525590.57 | 1997-12-26 | 1-URGENT        | Clerk#000000336 |            0 | he slyly ironic requests. regular, bold deposits cajol
  2232932 |   13940 | O           |  522720.61 | 1997-04-13 | 2-HIGH          | Clerk#000000245 |            0 | lithely stealthy accounts are slyly against the pend
  3586919 |   24049 | F           |  522644.48 | 1992-11-07 | 1-URGENT        | Clerk#000000924 |            0 | are alongside of the pending deposits. quick
  2199712 |   66790 | O           |  515531.82 | 1996-09-30 | 2-HIGH          | Clerk#000000650 |            0 |  the final, ironic deposits inte
  2185667 |   51796 | F           |  511359.88 | 1992-10-08 | 1-URGENT        | Clerk#000000574 |            0 | . deposits wake quickly unusual deposits. express dependencies wake.
  4515876 |  100685 | F           |   510061.6 | 1993-11-02 | 4-NOT SPECIFIED | Clerk#000000105 |            0 | carefully accounts: slyly ironic pinto beans detect. brave somas a
   972901 |  141100 | F           |  508668.52 | 1992-07-18 | 3-MEDIUM        | Clerk#000000667 |            0 | pinto beans according to the blithely express dolphins cajo
(10 rows)

Query 20210208_123059_00031_3ip8n, FINISHED, 2 nodes
Splits: 5 total, 5 done (100.00%)
0.25 [10 rows, 0B] [40 rows/s, 0B/s]

wendigo · 2021-02-08T13:00:39Z

trino:public> set session postgresql."topn_pushdown_enabled" = false;
SET SESSION
trino:public> SELECT sum(totalprice) FROM orders_sf1 WHERE orderpriority = '1-URGENT' GROUP by custkey ORDER BY sum(totalprice) ASC, custkey DESC LIMIT 10;
  _col0
---------
  965.09
  982.94
 1041.55
 1094.89
 1099.96
 1101.75
 1106.45
 1119.87
 1120.89
 1121.88
(10 rows)

Query 20210208_125938_00033_4b5qn, FINISHED, 2 nodes
Splits: 6 total, 6 done (100.00%)
0.83 [92.3K rows, 0B] [111K rows/s, 0B/s]

trino:public> SELECT sum(totalprice) FROM orders_sf1 WHERE orderpriority = '1-URGENT' GROUP by custkey ORDER BY sum(totalprice) ASC, custkey DESC LIMIT 10;
  _col0
---------
  965.09
  982.94
 1041.55
 1094.89
 1099.96
 1101.75
 1106.45
 1119.87
 1120.89
 1121.88
(10 rows)

Query 20210208_125932_00031_4b5qn, FINISHED, 1 node
Splits: 5 total, 5 done (100.00%)
0.47 [10 rows, 0B] [21 rows/s, 0B/s]

wendigo · 2021-02-08T14:31:20Z

Failures are unrelated:

error-prone: failed to download maven dependencies
trino-kafka: failed to start container

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

losipiuk

Some whining about functional programming.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/PreparedQuery.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcTableHandle.java

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestIntegrationSmokeTest.java

wendigo · 2021-02-17T12:52:33Z

AC @findepi

core/trino-spi/src/main/java/io/trino/spi/connector/SortItem.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

findepi · 2021-02-18T13:03:06Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

+
+        if (handle.getSortOrder().isPresent() || handle.getLimit().isPresent()) {
+            if (handle.getLimit().equals(OptionalLong.of(topNCount)) && handle.getSortOrder().equals(Optional.of(resultSortOrder))) {
+                return Optional.of(new TopNApplicationResult<>(handle, jdbcClient.isTopNLimitGuaranteed(session)));


I think we should return Optional.empty here, per

trino/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

Lines 1067 to 1069 in 9957543

* <b>Note</b>: it's critical for connectors to return {@link Optional#empty()} if calling this method has no effect for that

* invocation, even if the connector generally supports topN pushdown. Doing otherwise can cause the optimizer

* to loop indefinitely.

returning a TopNApplicationResult will cause the optimize to loop when isTopNLimitGuaranteed = false, but IMO we should return empty regardless.

That breaks the code so it doesn't work anymore

What breaks? i didn't get this one

If I return Optional.empty() there, assertions isFullyPushedDown will fail as top-level TopN and Exchange nodes will remain in a plan.

We need to see whether we can defer creation of partial TopN, so that we don't need to deal with partial/final TopN in the TopN pushdown.
Then the engine rule would be narrowed down to SINGLE TopN s only.
Do you feel like you could investigate this?

I've applied changes to the optimizers and PushTopNIntoTableScan rule. PTAL now.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

findepi

Looks good overall!
couple editorial comments

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushTopNIntoTableScan.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java

findepi · 2021-02-25T08:24:47Z

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestQueries.java

@@ -746,7 +746,32 @@ public void testSelectCaseInsensitive()
    @Test
    public void testTopN()
    {
-        assertQuery("SELECT n.name, r.name FROM nation n LEFT JOIN region r ON n.regionkey = r.regionkey ORDER BY n.name LIMIT 1");


Let's keep the original short test method.
It doesn't make sense to put the connector under the test if it does not support TopN pushdown (and most do not today).

Ideally, we should remove this test from here and into Engine-only queries (do we have it?)
and have a BCT test that verified that either connector does not support TopN pushdown or is thoroughly tested, like we do with eg inserts.

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

wendigo · 2021-02-25T16:47:39Z

Ptal @findepi

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

findepi · 2021-02-26T16:14:35Z

@wendigo please check the compilation issue

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

Co-authored-by: pbrahmbhatt <brahmbhatt.parth@gmail.com>

wendigo · 2021-03-02T11:00:59Z

Failed due to:

2021-03-02T09:57:15.6330682Z [ERROR] Tests run: 398, Failures: 1, Errors: 0, Skipped: 22, Time elapsed: 1,317.346 s <<< FAILURE! - in TestSuite
2021-03-02T09:57:15.6338364Z [ERROR] init(io.trino.plugin.oracle.TestOracleCaseInsensitiveMapping)  Time elapsed: 296.399 s  <<< FAILURE!
2021-03-02T09:57:15.6344290Z java.lang.RuntimeException: java.sql.SQLRecoverableException: IO Error: Connection reset by peer, Authentication lapse 0 ms.

cla-bot bot added the cla-signed label Feb 8, 2021

wendigo requested a review from findepi February 8, 2021 12:04

wendigo force-pushed the serafin/topn-pushdown branch from 3b84164 to 4f92c24 Compare February 8, 2021 13:01

wendigo marked this pull request as ready for review February 8, 2021 14:31

wendigo requested a review from losipiuk February 8, 2021 14:31

losipiuk reviewed Feb 8, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

losipiuk reviewed Feb 8, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

losipiuk reviewed Feb 8, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

losipiuk reviewed Feb 8, 2021

View reviewed changes

findepi requested changes Feb 8, 2021

View reviewed changes

wendigo force-pushed the serafin/topn-pushdown branch from 4f92c24 to 70ed668 Compare February 11, 2021 11:08

findepi requested changes Feb 11, 2021

View reviewed changes

wendigo force-pushed the serafin/topn-pushdown branch 3 times, most recently from e7756c9 to 069943a Compare February 16, 2021 12:06

findepi requested changes Feb 17, 2021

View reviewed changes

wendigo force-pushed the serafin/topn-pushdown branch from 069943a to 025fb25 Compare February 17, 2021 12:48

wendigo requested a review from findepi February 17, 2021 12:52

findepi reviewed Feb 18, 2021

View reviewed changes

wendigo force-pushed the serafin/topn-pushdown branch from 025fb25 to f91c8b9 Compare February 18, 2021 13:49

wendigo requested a review from findepi February 18, 2021 18:17

wendigo force-pushed the serafin/topn-pushdown branch from f91c8b9 to 643aed4 Compare February 22, 2021 13:40

findepi mentioned this pull request Feb 23, 2021

SPI and optimizer rule for connectors that can support complete topN … #4249

Merged

wendigo force-pushed the serafin/topn-pushdown branch from 643aed4 to 5035072 Compare February 24, 2021 11:32

findepi referenced this pull request Feb 25, 2021

Push TopN through Project

bd14140

findepi mentioned this pull request Feb 25, 2021

Push PARTIAL TopN into TableScan #7028

Closed

findepi mentioned this pull request Feb 25, 2021

Add PushdownTopN through Project and Outer Join rules #419

Merged

findepi reviewed Feb 25, 2021

View reviewed changes

wendigo force-pushed the serafin/topn-pushdown branch from 0fa9634 to 1545f85 Compare February 25, 2021 10:00

wendigo requested a review from findepi February 25, 2021 10:04

wendigo force-pushed the serafin/topn-pushdown branch 2 times, most recently from d100bc9 to 2e276ac Compare February 25, 2021 14:14

findepi approved these changes Feb 26, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/topn-pushdown branch from 2e276ac to 36ea6c1 Compare February 26, 2021 12:38

findepi approved these changes Feb 26, 2021

View reviewed changes

wendigo force-pushed the serafin/topn-pushdown branch 2 times, most recently from ed81595 to 7ca452f Compare March 1, 2021 11:41

wendigo requested a review from findepi March 1, 2021 11:43

findepi approved these changes Mar 1, 2021

View reviewed changes

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java Show resolved Hide resolved

wendigo and others added 4 commits March 2, 2021 09:53

Add support for TopN pushdown in JDBC connectors

75957ba

Co-authored-by: pbrahmbhatt <brahmbhatt.parth@gmail.com>

fixup! Add support for TopN pushdown in JDBC connectors

ca5676e

fixup! Add support for TopN pushdown in JDBC connectors

667bbf1

fixup! Add support for TopN pushdown in JDBC connectors

9c5f492

wendigo force-pushed the serafin/topn-pushdown branch from 18a8ab5 to 9c5f492 Compare March 2, 2021 09:10

wendigo requested a review from findepi March 2, 2021 11:15

findepi merged commit 1bae748 into trinodb:master Mar 2, 2021

This was referenced Mar 2, 2021

Implement topN pushdown for JDBC connectors #4784

Closed

Implement TopN pushdown for JDBC connectors #4769

Closed

wendigo deleted the serafin/topn-pushdown branch March 2, 2021 12:09

findepi added this to the 353 milestone Mar 2, 2021

findepi mentioned this pull request Mar 2, 2021

Release notes for 353 #6858

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TopN pushdown in JDBC connectors #6847

Add support for TopN pushdown in JDBC connectors #6847

wendigo commented Feb 8, 2021

wendigo commented Feb 8, 2021

wendigo commented Feb 8, 2021

wendigo commented Feb 8, 2021

losipiuk left a comment

wendigo commented Feb 17, 2021

findepi Feb 18, 2021

wendigo Feb 18, 2021

findepi Feb 18, 2021

wendigo Feb 18, 2021

findepi Feb 19, 2021

wendigo Feb 24, 2021

findepi left a comment

findepi Feb 25, 2021

wendigo commented Feb 25, 2021

findepi commented Feb 26, 2021

wendigo commented Mar 2, 2021

	* <b>Note</b>: it's critical for connectors to return {@link Optional#empty()} if calling this method has no effect for that
	* invocation, even if the connector generally supports topN pushdown. Doing otherwise can cause the optimizer
	* to loop indefinitely.

Add support for TopN pushdown in JDBC connectors #6847

Add support for TopN pushdown in JDBC connectors #6847

Conversation

wendigo commented Feb 8, 2021

wendigo commented Feb 8, 2021

wendigo commented Feb 8, 2021

wendigo commented Feb 8, 2021

losipiuk left a comment

Choose a reason for hiding this comment

wendigo commented Feb 17, 2021

findepi Feb 18, 2021

Choose a reason for hiding this comment

wendigo Feb 18, 2021

Choose a reason for hiding this comment

findepi Feb 18, 2021

Choose a reason for hiding this comment

wendigo Feb 18, 2021

Choose a reason for hiding this comment

findepi Feb 19, 2021

Choose a reason for hiding this comment

wendigo Feb 24, 2021

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi Feb 25, 2021

Choose a reason for hiding this comment

wendigo commented Feb 25, 2021

findepi commented Feb 26, 2021

wendigo commented Mar 2, 2021