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

Fix + post merge Port Join operator to WorkProcessor improvements #1942

Merged
merged 6 commits into from Nov 4, 2019

Conversation

@sopel39
Copy link
Member

sopel39 commented Nov 3, 2019

Fixes compilation errors after merging #1256. #1256 was quite old and LazyBlocks implementation changed during that period.

@sopel39 sopel39 requested review from dain, findepi and Praveen2112 Nov 3, 2019
@cla-bot cla-bot bot added the cla-signed label Nov 3, 2019
@sopel39 sopel39 force-pushed the starburstdata:ks/fix_join_wp branch 3 times, most recently from cd177da to b953293 Nov 3, 2019
@sopel39 sopel39 added the bug label Nov 3, 2019
@sopel39 sopel39 force-pushed the starburstdata:ks/fix_join_wp branch from b953293 to e5fbb94 Nov 3, 2019
@@ -677,14 +677,14 @@ private boolean outerJoinCurrentPosition()
}

boolean consumedInput = false;
if (needsInput()) {
addInput(element);
if (needsInput() && inputPage != null) {

This comment has been minimized.

Copy link
@Praveen2112

Praveen2112 Nov 4, 2019

Member

But isn't inputPage != null redundant here ? Bcoz needsInput() will be false once finishing is set to true which happens in this snippet

if (innputPage == null) {
    finish();
}

This comment has been minimized.

Copy link
@sopel39

sopel39 Nov 4, 2019

Author Member

It's not redundant. finish() might not set finishing when spill is in progress. Therefore needsInput might be true once the spill is done, but inputPage is null

sopel39 added 4 commits Nov 3, 2019
When page is null, then finish() will be called.
However, finish() might not set finishing to true
because of spill in progress. Thus there is a race
condition between needsMoreData() and spill finishing.
needsMoreData() could potentially return true
when input page is null.
@sopel39 sopel39 force-pushed the starburstdata:ks/fix_join_wp branch from e5fbb94 to 58ed645 Nov 4, 2019
@dain
dain approved these changes Nov 4, 2019
sopel39 added 2 commits Nov 3, 2019
getRegion and getPositions do not load
underlying blocks now.
@sopel39 sopel39 force-pushed the starburstdata:ks/fix_join_wp branch from 58ed645 to 1346c6c Nov 4, 2019
@sopel39 sopel39 merged commit 444e75a into prestosql:master Nov 4, 2019
1 of 2 checks passed
1 of 2 checks passed
Travis CI - Pull Request Build Errored
Details
verification/cla-signed
Details
@sopel39 sopel39 deleted the starburstdata:ks/fix_join_wp branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.