Skip to content

Add use-exact-partitioning-enabled session property#13354

Merged
arhimondr merged 3 commits intoprestodb:masterfrom
aweisberg:use_exact_partitioning
Jan 16, 2020
Merged

Add use-exact-partitioning-enabled session property#13354
arhimondr merged 3 commits intoprestodb:masterfrom
aweisberg:use_exact_partitioning

Conversation

@aweisberg
Copy link
Contributor

@aweisberg aweisberg commented Sep 6, 2019

When enabled this forces exchanges to occur unless the partitioning matches exactly

== RELEASE NOTES ==

General Changes
* Add use_exact_partitioning session property that forces repartitioning if repartitioning is possible

@aweisberg aweisberg force-pushed the use_exact_partitioning branch from 07c8cac to b086255 Compare September 9, 2019 21:30
@aweisberg aweisberg force-pushed the use_exact_partitioning branch 3 times, most recently from 9d7ffd9 to 988ae14 Compare November 21, 2019 22:35
@aweisberg aweisberg requested a review from arhimondr December 13, 2019 19:44
@aweisberg
Copy link
Contributor Author

A bunch of unrelated files snuck in, I'll clean that up now.

@aweisberg aweisberg force-pushed the use_exact_partitioning branch 2 times, most recently from 651259f to f6fa5e1 Compare December 13, 2019 19:54
Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good % couple of nits / questions.

This may potentially have some conflicts with #11262. I wonder if configuration for both patches can be unified.

CC: @jessesleeping @shixuan-fan

@aweisberg aweisberg force-pushed the use_exact_partitioning branch from f6fa5e1 to 82ce85a Compare December 16, 2019 19:11
@jessesleeping
Copy link
Contributor

jessesleeping commented Dec 16, 2019

I think we can merge the 2 PRs.

Both #11262 and this PR are trying to control the behavior when adding repartitions. In #11262 the main focus is to control the following 2 actions:
(1) Whether we merged parent partitioning preference before pushing it down to child
(2) If Yes for (1), whether we add repartition exchange based on the merged preference or the node's own preference

I added a session property to control this behavior:
LEGACY: Yes for (1) and use the node's own preference for (2). This is the current behavior of Presto.
TOP_DOWN: Yes for (1) and use the merged preference for (2).
BOTTOM_UP: No for (1).

In this PR, we introduce a more strict version of BOTTOM_UP, which is "No for (1) and use the node's own preference but only if it's exactly matched".

PS:
In #11262 I initially made the change to all nodes that invoke mergeWithParent(), but later on I restricted the behavior to only aggregation node in concern of minimizing the behavior change. We can generalize it again.

cc @aweisberg @arhimondr

@aweisberg
Copy link
Contributor Author

@jessesleeping Yeah it looks like exact partitioning, just wants a specific set of behaviors out of partition merging strategy. Maybe it makes sense to back out my changes to mergeWithParent and have use exact partitioning set partition merging strategy appropriately.

@jessesleeping
Copy link
Contributor

@aweisberg How about merging the session properties we introduced into a single one that has 4 options: LEGACY, TOP_DONW, BOTTOM_UP and EXACT? The first 3 ones are those introduced in #11262 and the last one is the one implemented here.

@aweisberg
Copy link
Contributor Author

@jessesleeping So exact would be the behavior I implemented in this PR? Makes sense to me.

@aweisberg aweisberg force-pushed the use_exact_partitioning branch from 2fbdfae to 4739d1d Compare December 20, 2019 22:21
@aweisberg aweisberg requested a review from arhimondr December 20, 2019 22:22
Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also suggest to merge two configuration into one (effectively adding one more option to the enum LEGACY, TOP_DOWN, BOTTOM_UP, EXACT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what i understand there are only 2 differences between EXACT and MERGE_PARTITION_PREFERENCE: when EXACT is chosen - the local properties are not merged, and if the current property requirement has no partitioning requirement - parent partitioning requirement is taken.

The localProperties in the PreferredProperties (and expecially merging them) makes no sense. So I checked how the localProperties properties are used, and I have found that the localProperties are effectively unused. Thus I wouldn't bother about the localProperties for now. Ideally it would be nice to have a refactoring PR, and completely remove this field to avoid confusion in the future.

For the "no partitioning" requirement case - I don't think there's any practical difference.

Thus, I'm trying to imply that we should simply have a boolean flag here.

@aweisberg aweisberg force-pushed the use_exact_partitioning branch 2 times, most recently from 12f1d9e to 744e2ba Compare December 23, 2019 22:20
@aweisberg aweisberg requested a review from arhimondr December 23, 2019 22:20
@aweisberg aweisberg force-pushed the use_exact_partitioning branch 3 times, most recently from 08394ac to e0638a4 Compare January 9, 2020 17:49
@aweisberg
Copy link
Contributor Author

I think this is ready to look at again. I think CI is failing on an unrelated timeout. It still seems to be making progress but it is taking too long?
https://api.travis-ci.org/v3/job/634863030/log.txt

It also just failed here https://travis-ci.org/prestodb/presto/jobs/634881878?utm_medium=notification&utm_source=github_status

Going to run it locally to see what happens.

@aweisberg
Copy link
Contributor Author

So this is test-hive-pushdown-filter-queries-basic. I really we wish we had "real" CI so I could go back to older builds and figure out if this has been getting slower over time.

It has two tests in it. We could split the two tests into separate jobs.

@aweisberg aweisberg force-pushed the use_exact_partitioning branch from 120211a to 5a3725d Compare January 10, 2020 15:49
Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash unnecessary commits before merge

@aweisberg aweisberg force-pushed the use_exact_partitioning branch from 5a3725d to 67449a1 Compare January 14, 2020 23:35
@aweisberg
Copy link
Contributor Author

Rebased and removed the commits that showed the tests are still passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants