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

Rely on noMoreOperators for dynamic filters collection #12594

Merged
merged 1 commit into from
May 31, 2022

Conversation

raunaqmorarka
Copy link
Member

Relying on noMoreOperators from DynamicFilterSourceOperatorFactory
for detecting completion of dynamic filter collection will simplify
the implementation for fault tolerant execution where the collection
may take place in a source stage.

Relying on noMoreOperators from DynamicFilterSourceOperatorFactory
for detecting completion of dynamic filter collection will simplify
the implementation for fault tolerant execution where the collection
may take place in a source stage.
@sopel39
Copy link
Member

sopel39 commented May 30, 2022

Can it increase DF collection latency?

@raunaqmorarka
Copy link
Member Author

Can it increase DF collection latency?

Normally OperatorFactory#noMoreOperators should fire before or around when the last operator finishes. Do you see a reason to suspect impact to latency ? I can run the usual benchmarks to confirm if needed.

@sopel39
Copy link
Member

sopel39 commented May 30, 2022

Normally OperatorFactory#noMoreOperators should fire before or around when the last operator finishes

I think we can bail-out faster before noMoreOperators is called. Idk, maybe it's not an issue. Haven't looked at the code for a while

@raunaqmorarka
Copy link
Member Author

Normally OperatorFactory#noMoreOperators should fire before or around when the last operator finishes

I think we can bail-out faster before noMoreOperators is called. Idk, maybe it's not an issue. Haven't looked at the code for a while

I've run TPC benchmarks and confirmed no regressions. Let me know if you have any input about the code or any problematic scenarios in mind.

@sopel39
Copy link
Member

sopel39 commented May 31, 2022

Let me know if you have any input about the code or any problematic scenarios in mind.

Nope. Just wanted to confirm latency is not harmed

@losipiuk losipiuk merged commit c49be84 into trinodb:master May 31, 2022
@github-actions github-actions bot added this to the 383 milestone May 31, 2022
import io.trino.spi.predicate.TupleDomain;
import io.trino.sql.planner.plan.DynamicFilterId;

public interface DynamicFilterSourceConsumer
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: DynamicFilterConsumer, or maybe simply drop this interface and override the method you need for testing?

@colebow
Copy link
Member

colebow commented May 31, 2022

Do we want a release notes entry for this?

@raunaqmorarka raunaqmorarka deleted the df-partitions branch May 31, 2022 16:10
@raunaqmorarka
Copy link
Member Author

Do we want a release notes entry for this?

This is just an internal refactoring, no user facing change

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.

5 participants