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

Negate method invocation #10728

Conversation

skrzypo987
Copy link
Member

No description provided.

This is a dead code, but should be correct anyway
@cla-bot cla-bot bot added the cla-signed label Jan 21, 2022
@skrzypo987 skrzypo987 requested a review from sopel39 January 21, 2022 12:18
@findepi
Copy link
Member

findepi commented Jan 21, 2022

This looks like a correctness fix. Is it testable?

@skrzypo987
Copy link
Member Author

The actual implementation that is used is bytecode-generated. This is not used anywhere, so no tests so far

@@ -215,7 +215,7 @@ public boolean positionNotDistinctFromRow(int leftBlockIndex, int leftPosition,
Block leftBlock = channels.get(hashChannel).get(leftBlockIndex);
Block rightBlock = page.getBlock(rightChannels[i]);
BlockPositionIsDistinctFrom isDistinctFromOperator = isDistinctFromOperators.get(i);
if (!isDistinctFromOperator.isDistinctFrom(leftBlock, leftPosition, rightBlock, rightPosition)) {
if (isDistinctFromOperator.isDistinctFrom(leftBlock, leftPosition, rightBlock, rightPosition)) {
Copy link
Member

Choose a reason for hiding this comment

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

please add a test to io.trino.operator.TestSimplePagesHashStrategy
It's not dead code yet (although it should be)

        // if compilation fails, use interpreter
        return new SimplePagesHashStrategy(

I think we should remove that fallback

@mosabua
Copy link
Member

mosabua commented Nov 3, 2022

👋 @skrzypo987 and @sopel39 .. can this be merged?

@sopel39 sopel39 merged commit 999cb4b into trinodb:master Nov 4, 2022
@sopel39
Copy link
Member

sopel39 commented Nov 4, 2022

no release notes needed

@github-actions github-actions bot added this to the 403 milestone Nov 4, 2022
@mosabua
Copy link
Member

mosabua commented Nov 5, 2022

Sweet.. thank @sopel39

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