Use dynamic split sizes in hive connector by pranjalssh · Pull Request #22051 · prestodb/presto

pranjalssh · 2024-02-29T23:21:19Z

Description

FIxes #21911

presto scheduler creates splits according to file sizes - but does not take into account if we read only selected columns from the file. In general, we should be able to tune split sizes based on amount of data we select from the files - so we can have fewer splits and presto runs faster.

Motivation and Context

Impact

Test Plan

Added unit test, gated changes behind config, and ran test queries on a cluster

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Hive Changes
* Add session property ``hive.dynamic_split_sizes_enabled`` to use dynamic split sizes based on data selected by query.

mbasmanova

@pranjalssh Would you document the new configuration property?

https://prestodb.io/docs/current/connector/hive.html#hive-configuration-properties

CC: @steveburnett

mbasmanova

@pranjalssh Thank you for working on this. What kind of speed up have you observed on production queries? It would be nice to update commit message to provide more details about this change. Is there a way to see whether this optimization kicked in and what was the 'ratio' applied after the query finished running?

mbasmanova · 2024-03-01T07:53:53Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitSource.java

        this.splitWeightProvider = isSizeBasedSplitWeightsEnabled(session) ? new SizeBasedSplitWeightProvider(getMinimumAssignedSplitWeight(session), maxSplitSize) : HiveSplitWeightProvider.uniformStandardWeightProvider();
+        // Clamp value within [0.1, 1.0].
+        // This ratio will be used to increase split sizes. The range implies
+        // 1) We do not increase more than 10x(>= 0.1)


Curious why is this limit?

It would be helpful to update commit message to provide more details about the implementation and these kinds of limits.

Can you answer this question @pranjalssh?

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

mbasmanova · 2024-03-01T07:56:24Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

+            HiveTableHandle hiveTableHandle = new HiveTableHandle(tableName.getSchemaName(), tableName.getTableName());
+            List<HiveColumnHandle> allColumnHandles = new ArrayList<>();
+            allColumnHandles.addAll(getRegularColumnHandles(table));
+            allColumnHandles.addAll(getPartitionKeyColumnHandles(table));


Why do we include partitioning keys? These are not part of the file.

mbasmanova · 2024-03-01T08:02:29Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

+            double totalSize = tableStatistics.getTotalSize().getValue();
+            double requiredSize = 0;
+            double rowCount = tableStatistics.getRowCount().getValue();
+            for (Map.Entry<ColumnHandle, ColumnStatistics> entry : tableStatistics.getColumnStatistics().entrySet()) {


Here we loop over all columns (can be thousands), but process only a set of readColumnHandles columns (can be a handful). Would it make sense to change this to loop over readColumnHandles instead?

Refactored to only query for read columns. I previously incorrectly assumed totalSize for summed for just columns provided

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

mbasmanova · 2024-03-01T08:05:12Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

+                if (readColumnHandles.contains(entry.getKey())) {
+                    double value = entry.getValue().getDataSize().getValue();
+                    // We do not compute total size stats for fixed width types, so count them manually.
+                    if (!isFinite(value) && isFinite(rowCount)) {


isFinite(rowCount)

Can we move this check before the loop?

mbasmanova · 2024-03-01T08:06:08Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

+            for (Map.Entry<ColumnHandle, ColumnStatistics> entry : tableStatistics.getColumnStatistics().entrySet()) {
+                if (readColumnHandles.contains(entry.getKey())) {
+                    double value = entry.getValue().getDataSize().getValue();
+                    // We do not compute total size stats for fixed width types, so count them manually.


Does that mean 'totalSize' doesn't include fixed-width columns?

It does, its just missing in column stats. Refactored

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSplitScheduling.java

mbasmanova · 2024-03-01T08:09:44Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSplitScheduling.java

+            TestHiveEventListenerPlugin.TestingHiveEventListener eventListener = getEventListener();
+
+            // Wait for previous events to finish
+            Thread.sleep(2000);


Is there a way to avoid explicit sleep calls?

I found that event manager is synchronous in these tests, so we can just remove all sleep calls

kaikalur · 2024-03-01T21:44:44Z

I have seen 90% reduction in number of splits for some queries with this option (380K vs 3M)! And corresponding reduction in latency. so this can help sometimes especially in things like count( * ) queries

steveburnett · 2024-03-04T14:35:50Z

@pranjalssh Would you document the new configuration property?

https://prestodb.io/docs/current/connector/hive.html#hive-configuration-properties

CC: @steveburnett

@pranjalssh, when you can, please add documentation as suggested by @mbasmanova and recommended in the Documentation topic of the Review and Commit Guidelines.

When you do, you can use the request review feature to tag me so I'll be notified and able to respond in a timely way and not delay the PR. Thanks!

pranjalssh · 2024-03-04T19:52:51Z

Addressed comments @steveburnett @mbasmanova

github-actions · 2024-03-04T19:53:56Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff ece7741...ce14167.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/connector/hive.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local build, everything looks good.

rschlussel · 2024-03-04T21:05:03Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

+        if (!isDynamicSplitSizesEnabled(session)) {
+            return ratio;
+        }
+        HiveTableHandle hiveTableHandle = new HiveTableHandle(tableName.getSchemaName(), tableName.getTableName());


use the function mergeRequestedAndPredicateColumns(). It will also handle if the same struct columns appear in both sets with different subfields.

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

rschlussel · 2024-03-04T21:13:43Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

        return splitSource;
    }

+    private double getSplitScanRatio(


Can you clarify what splitScanRatio means? The name is confusing to me. It looks like it's the proportion of bytes we expect to read based on column stats of columns we actually use compared to the total data size.

rschlussel · 2024-03-04T21:14:26Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

+
+        TableStatistics tableStatistics = metadata.getHiveStatisticsProvider().getTableStatistics(session, tableName, readColumns, readColumnTypes, partitions);
+        double totalSize = tableStatistics.getTotalSize().getValue();
+        double requiredSize = 0;


what does requiredSize mean? it looks like we're getting the total size of all columns we're selecting. What's "required" about it?

Updated to readSize

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

rschlussel

looks good

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

rschlussel · 2024-03-05T20:13:51Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitSource.java

        this.splitWeightProvider = isSizeBasedSplitWeightsEnabled(session) ? new SizeBasedSplitWeightProvider(getMinimumAssignedSplitWeight(session), maxSplitSize) : HiveSplitWeightProvider.uniformStandardWeightProvider();
+        // Clamp value within [0.1, 1.0].
+        // This ratio will be used to increase split sizes. The range implies
+        // 1) We do not increase more than 10x(>= 0.1)


Can you answer this question @pranjalssh?

If data scanned by query is smaller than total size of files, we use larger splits than configured by Hive. We schedule upto 10x larger splits - which is a very generous limit - and being conservative not to schedule splits with too many rows

pranjalssh · 2024-03-05T21:02:28Z

@rschlussel updated comment

        // We schedule only upto 10x larger splits - being conservative not to schedule splits with too many rows.
        // For default size of 64MB, this will keep split sizes sent within 1GB. Usually files are smaller than this.

pranjalssh requested a review from a team as a code owner February 29, 2024 23:21

pranjalssh requested a review from presto-oss February 29, 2024 23:21

pranjalssh force-pushed the dynamic_splits2 branch from 32f5022 to 4114f9d Compare March 1, 2024 00:15

pranjalssh requested review from feilong-liu, jainxrohit, kaikalur and rschlussel March 1, 2024 00:15

pranjalssh force-pushed the dynamic_splits2 branch 2 times, most recently from a5ada00 to 6e52f59 Compare March 1, 2024 00:53

pranjalssh requested a review from a team as a code owner March 1, 2024 00:53

pranjalssh force-pushed the dynamic_splits2 branch from 6e52f59 to 9099406 Compare March 1, 2024 00:56

mbasmanova reviewed Mar 1, 2024

View reviewed changes

pranjalssh force-pushed the dynamic_splits2 branch 3 times, most recently from a47125b to 99d773e Compare March 4, 2024 19:52

pranjalssh requested review from mbasmanova and steveburnett March 4, 2024 19:52

steveburnett previously approved these changes Mar 4, 2024

View reviewed changes

pranjalssh dismissed steveburnett’s stale review via b1e2cb2 March 4, 2024 21:08

pranjalssh force-pushed the dynamic_splits2 branch from 99d773e to b1e2cb2 Compare March 4, 2024 21:08

rschlussel reviewed Mar 4, 2024

View reviewed changes

pranjalssh force-pushed the dynamic_splits2 branch from b1e2cb2 to f8a2b3f Compare March 5, 2024 18:31

pranjalssh requested a review from rschlussel March 5, 2024 18:31

rschlussel reviewed Mar 5, 2024

View reviewed changes

Use dynamic sized splits in Hive

ce14167

If data scanned by query is smaller than total size of files, we use larger splits than configured by Hive. We schedule upto 10x larger splits - which is a very generous limit - and being conservative not to schedule splits with too many rows

pranjalssh force-pushed the dynamic_splits2 branch from f8a2b3f to ce14167 Compare March 5, 2024 21:01

pranjalssh requested a review from rschlussel March 5, 2024 21:01

rschlussel approved these changes Mar 5, 2024

View reviewed changes

pranjalssh merged commit 96187ad into prestodb:master Mar 5, 2024

kaikalur mentioned this pull request Mar 5, 2024

Presto Hive Connector creates too many small splits #21911

Closed

pranjalssh mentioned this pull request Apr 30, 2024

Adjust split weights by actual data read #22635

Merged

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Conversation

pranjalssh commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaikalur commented Mar 1, 2024

Uh oh!

steveburnett commented Mar 4, 2024

Uh oh!

pranjalssh commented Mar 4, 2024

Uh oh!

github-actions bot commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranjalssh commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pranjalssh commented Feb 29, 2024 •

edited

Loading

github-actions bot commented Mar 4, 2024 •

edited

Loading

pranjalssh commented Mar 5, 2024 •

edited

Loading