Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement adaptive partitioning for FTE based on input #17024

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

linzebing
Copy link
Member

@linzebing linzebing commented Apr 13, 2023

Description

Extend adaptive partitioning to fault-tolerant execution. Testing on tpcds-sf100 shows 4% improvement on wall time and 8% improvement on CPU time. Testing on tpcds-sf1000 shows 4.2% improvement on wall time and 1.9% improvement on CPU time.

Additional context and related issues

#15489

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 13, 2023
Comment on lines 111 to 122
// Skip for write nodes since writing partitioned data with small amount of nodes could cause
// memory related issues even when the amount of data is small. Additionally, skip for FTE mode since we
// are not using estimated partitionCount in FTE scheduler.
if (PlanNodeSearcher.searchFrom(plan).whereIsInstanceOfAny(INSERT_NODES).matches()
|| getRetryPolicy(session) == RetryPolicy.TASK) {
// For streaming execution, skip for write nodes since writing partitioned data with small amount of nodes could cause
// memory related issues even when the amount of data is small.
if (!getRetryPolicy(session).equals(RetryPolicy.TASK) && PlanNodeSearcher.searchFrom(plan).whereIsInstanceOfAny(INSERT_NODES).matches()) {
return plan;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Adaptive partitioning is disabled for writes in streaming execution, is this a problem in FTE as well? @arhimondr ?

Copy link
Member

@losipiuk losipiuk Apr 13, 2023

Choose a reason for hiding this comment

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

Yes - it makes sense to keep lots of partitons if we are writing to avoid "too many open writers" problem. Not sure if we need upper bound always.

@linzebing linzebing requested review from arhimondr and losipiuk April 13, 2023 18:42
@linzebing
Copy link
Member Author

Also, do we need additional fte-specific configs of optimizer.min-input-size-per-task, optimizer.min-input-rows-per-task?

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Nice. LGTM.

@losipiuk
Copy link
Member

Also, do we need additional fte-specific configs of optimizer.min-input-size-per-task, optimizer.min-input-rows-per-task?

Technically we do not support mixed workloads - so maybe not. @arhimondr WDYT?

@linzebing linzebing force-pushed the fte-adaptive-partitioning branch from 6200393 to 4ec3971 Compare April 14, 2023 13:03
@losipiuk
Copy link
Member

testPlanWhenRetryPolicyIsTask fails as number of partitions changed.

@linzebing linzebing force-pushed the fte-adaptive-partitioning branch from 4ec3971 to fa54a3d Compare April 15, 2023 14:05
@linzebing
Copy link
Member Author

Failure is unrelated

@linzebing
Copy link
Member Author

@losipiuk @arhimondr : do you think it makes sense to introduce a flag to enable adaptive partitioning for writing? It's meaningful in the fact that we can enable this flag by setting fault_tolerant_execution_min_partition_count to a relative high number like 50.

@losipiuk
Copy link
Member

@losipiuk @arhimondr : do you think it makes sense to introduce a flag to enable adaptive partitioning for writing? It's meaningful in the fact that we can enable this flag by setting fault_tolerant_execution_min_partition_count to a relative high number like 50.

IDK how important it is in practice. But if sth I would rather allow for configuring different minimum for writer and non-writer queries.

@linzebing linzebing force-pushed the fte-adaptive-partitioning branch from fa54a3d to 6d80ea4 Compare April 19, 2023 18:55
@linzebing linzebing requested a review from gaurav8297 April 19, 2023 18:55
@linzebing linzebing force-pushed the fte-adaptive-partitioning branch from 6d80ea4 to 578a6c7 Compare April 20, 2023 15:04
@losipiuk losipiuk merged commit 6227c73 into trinodb:master Apr 21, 2023
@github-actions github-actions bot added this to the 415 milestone Apr 21, 2023
@linzebing linzebing changed the title Implement adaptive partitioning for FTE Implement adaptive partitioning for FTE based on input Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants