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

Use precomputed hash in probe side of HashSemiJoinOperator when available #767

Merged
merged 1 commit into from May 15, 2019

Conversation

2 participants
@stagraqubole
Copy link
Contributor

commented May 14, 2019

fixes #766

@cla-bot cla-bot bot added the cla-signed label May 14, 2019

@stagraqubole stagraqubole force-pushed the stagraqubole:semiJoinOpt branch from d4b6870 to d9d5d64 May 14, 2019

@dain
Copy link
Member

left a comment

Two minor comments, but otherwise looks good.

One last thing, can you change the commit message to just Use precomputed hash in probe side of HashSemiJoinOperator, so it doesn't wrap?

}
}

private final int probeJoinChannel;
private final ListenableFuture<ChannelSet> channelSetFuture;
private final Optional<Integer> probeHashChannel;

private ChannelSet channelSet;
private Page outputPage;
private boolean finishing;

public HashSemiJoinOperator(OperatorContext operatorContext, SetSupplier channelSetFuture, int probeJoinChannel)

This comment has been minimized.

Copy link
@dain

dain May 14, 2019

Member

HashSemiJoinOperator is an internal detail of Presto, so we don't need to maintain API backwards compatibility. This constructor is no longer used, so let's just remove it.

boolean contains = channelSet.contains(position, probeJoinPage);
boolean contains;
if (probeHashChannel.isPresent()) {
long rawHash = BIGINT.getLong(page.getBlock(probeHashChannel.get()), position);

This comment has been minimized.

Copy link
@dain

dain May 14, 2019

Member

Maybe hoist page.getBlock(probeHashChannel.get() outside of the for loop:

Optional<Block> hashBlock = probeHashChannel.map(page::getBlock);
for (...) {
...
    if (hashBlock.isPresent()) {
        long rawHash = BIGINT.getLong(hashBlock.get(), position);
        contains = channelSet.contains(position, probeJoinPage, rawHash);
    }
    else {
...

@stagraqubole stagraqubole force-pushed the stagraqubole:semiJoinOpt branch from d9d5d64 to 2765eae May 15, 2019

@stagraqubole

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Two minor comments, but otherwise looks good.

One last thing, can you change the commit message to just Use precomputed hash in probe side of HashSemiJoinOperator, so it doesn't wrap?

Done

@dain

dain approved these changes May 15, 2019

@dain dain merged commit fd5f498 into prestosql:master May 15, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details
@dain

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Merged. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.