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

Add DynamicFilter#isAwaitable method #5043

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

sopel39
Copy link
Member

@sopel39 sopel39 commented Sep 1, 2020

DynamicFilter#isAwaitable method can be used together with
DynamicFilter#isBlocked to wait for narrowed dynamic filters.
Some dynamic filters cannot be waited for (e.g for replicated joins).
Therefore existing DynamicFilter#isComplete method cannot be used
to determine if one can still wait for dynamic filter to be narrowed
(isComplete could return false while isBlocked would return completed
future immediatelly)

@sopel39 sopel39 requested a review from findepi September 8, 2020 18:57
@findepi
Copy link
Member

findepi commented Sep 8, 2020

in some cases dynamic filtes cannot be waited for (e.g for replicated joins).
Therefore DynamicFilter#isComplete should return "true" for such cases.

Maybe we need a tri-state logic for this?

@sopel39
Copy link
Member Author

sopel39 commented Sep 9, 2020

Maybe we need a tri-state logic for this?

I thought about it. However, let's say that you obtain info that DF is not complete yet, but you have no means to block while waiting for it. Then the only mean to wait for it would be via active loop.

Additionally, we want/will be supporting laziness for most DFs (replicated, re-partitioned) with exception for large replicated DFs, which should be rarity (and repartition joins too)

@findepi
Copy link
Member

findepi commented Sep 9, 2020

I thought about it. However, let's say that you obtain info that DF is not complete yet, but you have no means to block while waiting for it. Then the only mean to wait for it would be via active loop.

That's up to connector to decide what they do.
For example, it may reconsult the DF during split generation (doesn't require busy wait).
Or, it may throttle split generation temporarily, with the hope that split created later will be cheaper to process.

Even if we do not consider these as viable options today, I am generally concerned about removing factually incorrect information from the API.

If you do not like tri-state, maybe let's rename the method isComplete to eg isAwaitable?

@sopel39
Copy link
Member Author

sopel39 commented Sep 9, 2020

For example, it may reconsult the DF during split generation (doesn't require busy wait).

It can still do that via io.prestosql.spi.connector.DynamicFilter#getCurrentPredicate. However, connector doesn't have knowledge which DFs (column handles) might be collected.

Perhaps, we should give that knowledge to connector, but not part of this PR.

If you do not like tri-state, maybe let's rename the method isComplete to eg isAwaitable?

I could do that. It's a change in relatively fresh SPI though, which i think is fine

@sopel39 sopel39 changed the title Make DynamicFilter#isComplete return "true" for non-lazy filters Add DynamicFilter#isAwaitable method Sep 9, 2020
@sopel39
Copy link
Member Author

sopel39 commented Sep 9, 2020

@findepi ac

@Override
public synchronized boolean isAwaitable()
{
return futuresLeft > 0;
Copy link
Member

Choose a reason for hiding this comment

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

or !isBlocked.isDone() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

isBlocked will unblock when DF is narrowed down partially, but DF is still awaitable

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but then new isBlocked should be created, right?
So, in every moment current value of isBlocked.isBlocked() could be used here -- am i right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but then new isBlocked should be created, right?

Ah, right. Although futuresLeft > 0; seems more explicit

DynamicFilter#isAwaitable method can be used together with
DynamicFilter#isBlocked to wait for narrowed dynamic filters.
Some dynamic filters cannot be waited for (e.g for replicated joins).
Therefore existing DynamicFilter#isComplete method cannot be used
to determine if one can still wait for dynamic filter to be narrowed
(isComplete could return false while isBlocked would return completed
future immediatelly).
@sopel39 sopel39 merged commit cb300dc into trinodb:master Sep 11, 2020
@sopel39 sopel39 deleted the ks/fix_complete branch September 11, 2020 09:53
@sopel39 sopel39 mentioned this pull request Sep 11, 2020
9 tasks
@sopel39
Copy link
Member Author

sopel39 commented Sep 11, 2020

Thanks, @findepi

@martint martint added this to the 342 milestone Sep 24, 2020
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.

4 participants