Replace partial topN with limit when reading pre-sorted input #6634

raunaqmorarka · 2021-01-18T16:38:01Z

Fixes #5372

sopel39

small comments. @findepi @electrum do you want to take a look?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBucketHandle.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

plugin/trino-hive/src/test/sql/create-test.sql

findepi · 2021-01-19T10:43:47Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBucketHandle.java

@@ -16,6 +16,7 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.collect.ImmutableList;
+import io.trino.plugin.hive.metastore.SortingColumn;


This is metastore-specific class and it could be better to use a dedicaed, metastore-agnostic representation here.

i cannot offer a concrete proposal here yet, though

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBucketHandle.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveSessionProperties.java

plugin/trino-hive/src/test/sql/create-test.sql

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

sopel39

small comments

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBucketHandle.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveSplitManager.java

core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestEliminatePartialSort.java

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/AddLocalExchanges.java

sopel39

small comments

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

sopel39 · 2021-01-22T10:47:00Z

mind automation

sopel39

We could add integration test to TestHiveIntegrationSmokeTest too what verifies plan with sort and limit

sopel39 · 2021-01-22T12:01:57Z

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/AddExchanges.java

@@ -414,6 +414,16 @@ public PlanWithProperties visitTopN(TopNNode node, PreferredProperties preferred
                    break;
                case PARTIAL:
                    child = planChild(node, PreferredProperties.any());
+                    // If source is pre-sorted, partial topN can be replaced with partial Limit N
+                    List<LocalProperty<Symbol>> desiredProperties = node.getOrderingScheme().getOrderBy().stream()


This most likely deserves a separate rule but such rule would have to derive properties too (which is not the rule way). I think it's fine to keep simplification here since it's rather trivial and all information are at hand. cc @martint

raunaqmorarka · 2021-01-22T12:27:21Z

We could add integration test to TestHiveIntegrationSmokeTest too what verifies plan with sort and limit

Added testOptimizeSortedFileRead in TestHiveIntegrationSmokeTest which verifies change in plan with sorted hive table

raunaqmorarka · 2021-05-26T09:02:00Z

For kicks, I hooked this up with the Phoenix connector and it works perfectly - with noticeable performance improvements.

@lhofhansl you can try this one now with Phoenix connector

cla-bot bot added the cla-signed label Jan 18, 2021

raunaqmorarka requested a review from sopel39 January 18, 2021 16:38

raunaqmorarka force-pushed the sorted_by_local branch from 0bd174c to 785422b Compare January 18, 2021 20:13

sopel39 reviewed Jan 19, 2021

View reviewed changes

findepi reviewed Jan 19, 2021

View reviewed changes

raunaqmorarka force-pushed the sorted_by_local branch 3 times, most recently from 0e253a4 to 274fdba Compare January 19, 2021 15:34

raunaqmorarka requested review from sopel39 and findepi January 19, 2021 16:58

raunaqmorarka changed the title ~~Populate local sorting property for bucketed sorted hive tables~~ Avoid partial sort when reading pre-sorted input Jan 19, 2021

sopel39 reviewed Jan 20, 2021

View reviewed changes

raunaqmorarka force-pushed the sorted_by_local branch from 0e63e6a to eb0f971 Compare January 21, 2021 11:23

sopel39 reviewed Jan 21, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/AddLocalExchanges.java Outdated Show resolved Hide resolved

raunaqmorarka force-pushed the sorted_by_local branch 3 times, most recently from 2bab2b4 to b38589f Compare January 21, 2021 17:26

raunaqmorarka changed the title ~~Avoid partial sort when reading pre-sorted input~~ Replace partial topN with limit when reading pre-sorted input Jan 21, 2021

raunaqmorarka force-pushed the sorted_by_local branch from b38589f to 02440f1 Compare January 22, 2021 09:55

raunaqmorarka requested a review from sopel39 January 22, 2021 10:04

sopel39 approved these changes Jan 22, 2021

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java Show resolved Hide resolved

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java Show resolved Hide resolved

sopel39 reviewed Jan 22, 2021

View reviewed changes

raunaqmorarka force-pushed the sorted_by_local branch from 44713e1 to 10805d5 Compare January 22, 2021 11:02

sopel39 reviewed Jan 22, 2021

View reviewed changes

raunaqmorarka force-pushed the sorted_by_local branch from 10805d5 to 5825227 Compare January 22, 2021 12:25

raunaqmorarka force-pushed the sorted_by_local branch from 5825227 to 3cefc58 Compare January 22, 2021 14:12

findepi requested review from kasiafi and martint January 22, 2021 16:35

raunaqmorarka requested a review from martint May 25, 2021 07:00

raunaqmorarka force-pushed the sorted_by_local branch from 6fb8227 to 9598eeb Compare May 25, 2021 07:14

Replace partial topN with limit when reading pre-sorted input

26b7574

raunaqmorarka force-pushed the sorted_by_local branch from 9598eeb to 26b7574 Compare May 25, 2021 08:25

martint merged commit 4103acd into trinodb:master May 25, 2021

martint added this to the 358 milestone May 25, 2021

raunaqmorarka deleted the sorted_by_local branch May 26, 2021 02:53

martint mentioned this pull request May 28, 2021

Release notes for 358 #8040

Closed

11 tasks

lhofhansl mentioned this pull request Jun 1, 2021

Allow Phoenix to replace partial topn with limit. #8171

Merged

findepi mentioned this pull request Jun 2, 2021

Allow TPC-H to replace partial TopN with LIMIT #8175

Closed

raunaqmorarka mentioned this pull request Jul 12, 2021

Query failures with Expected Limit input to be sorted by: [...], but was [] #8526

Closed

lhofhansl mentioned this pull request Jul 16, 2021

Document Top-N pushdown #8468

Merged

pettyjamesm mentioned this pull request Nov 23, 2022

Fix incorrect plan property derivations #15154

Closed

findepi mentioned this pull request Dec 12, 2022

Support sorted writes in the Iceberg connector #14891

Merged

raunaqmorarka mentioned this pull request Jul 25, 2023

Rewrite partial top-n node to LimitNode or LastNNode #18384

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace partial topN with limit when reading pre-sorted input #6634

Replace partial topN with limit when reading pre-sorted input #6634

raunaqmorarka commented Jan 18, 2021 •

edited by sopel39

Loading

sopel39 left a comment

findepi Jan 19, 2021

sopel39 left a comment

sopel39 left a comment

sopel39 commented Jan 22, 2021

sopel39 left a comment

sopel39 Jan 22, 2021

raunaqmorarka commented Jan 22, 2021

raunaqmorarka commented May 26, 2021

Replace partial topN with limit when reading pre-sorted input #6634

Replace partial topN with limit when reading pre-sorted input #6634

Conversation

raunaqmorarka commented Jan 18, 2021 • edited by sopel39 Loading

sopel39 left a comment

Choose a reason for hiding this comment

findepi Jan 19, 2021

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 commented Jan 22, 2021

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 Jan 22, 2021

Choose a reason for hiding this comment

raunaqmorarka commented Jan 22, 2021

raunaqmorarka commented May 26, 2021

raunaqmorarka commented Jan 18, 2021 •

edited by sopel39

Loading