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 IS DISTINCT FROM for decimals with precision > 18 #13668

Merged
merged 1 commit into from
Jan 2, 2020

Conversation

ptkool
Copy link
Contributor

@ptkool ptkool commented Nov 7, 2019

The current logic in method distinctBlockPositionLongLong compares the left value to itself. This PR corrects this by assigning the proper values to rightLow and rightHigh.

Fixes #13667.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 7, 2019

CLA Check
The committers are authorized under a signed CLA.

  • ✅ Michael Styles (d5593acf6479ef97fd3d462af9bc7f74368996f2)

@ajaygeorge
Copy link
Contributor

@ptkool , thanks for catching the bug and adding a fix.

Couple of observations

  • Looking at the changes to fix this issue it is not very obvious how the issue is only for precision > 19 . Can you please help me understand that.
  • Since this bug can be exposed by a failing unit test I would highly recommend adding one if there is not one present already.

@ptkool ptkool changed the title Resolve issue with IS DISTINCT FROM and decimal values with precision > 19 Resolve issue with IS DISTINCT FROM and decimal values with precision > 18 Nov 8, 2019
@ptkool
Copy link
Contributor Author

ptkool commented Nov 8, 2019

@ajaygeorge The issue is actually with decimal values with precision > 18, not 19.

IS DISTINCT FROM processing follows a different code path for decimal values with precision > 18.

Precision Method Invoked
<= 18 DecimalInequalityOperators.distinctBlockPositionShortShort
> 18 DecimalInequalityOperators.distinctBlockPositionLongLong

Note that this issue only occurs when reading decimal values from a table and Presto creates instances of Int128ArrayBlock to represent these values.

@ajaygeorge ajaygeorge self-requested a review November 11, 2019 18:56
Copy link
Contributor

@ajaygeorge ajaygeorge left a comment

Choose a reason for hiding this comment

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

LGTM.
you might need to update Release Notes in the PR since this is a user impacting change.
Also please update the issue description in #13667 to reflect that the issue is for precision > 18
Adding @cemcayiroglu and @mbasmanova who can help review and merge this.

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@ptkool Michael, looks good. Thanks for fixing this issue. Please squash the commits and make commit message clearer. Perhaps, Fix IS DISTINCT FROM for long decimals.

@Test
public void testIsDistinctFrom()
{
long[] arr1 = {112L, 0L}; // DECIMAL '1.12'
Copy link
Contributor

Choose a reason for hiding this comment

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

  • don't use a1, a2, etc. variables names; here, I'd just inline arr1 and arr2
  • static import distinctBlockPositionLongLong
    @Test
    public void testIsDistinctFrom()
    {
        Block left = new Int128ArrayBlock(1, Optional.empty(), new long[] {112L, 0L});
        Block right = new Int128ArrayBlock(1, Optional.empty(), new long[] {185L, 0L});

        assertFalse(distinctBlockPositionLongLong(left, 0, left, 0));
        assertTrue(distinctBlockPositionLongLong(left, 0, right, 0));
    }

@mbasmanova mbasmanova changed the title Resolve issue with IS DISTINCT FROM and decimal values with precision > 18 Fix IS DISTINCT FROM for decimals with precision > 18 Nov 12, 2019
@ptkool ptkool force-pushed the fix_decimal_is_distinct_from branch 2 times, most recently from 45a4ca9 to a7fb354 Compare November 12, 2019 13:20
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@ptkool Looks good. Please remove the following from commit message

Add new test

Clean up based on feedback

@ptkool ptkool force-pushed the fix_decimal_is_distinct_from branch from a7fb354 to 6f212b3 Compare November 14, 2019 10:34
@mbasmanova
Copy link
Contributor

@ptkool Michael, I noticed that you updated the PR, but commit message didn't change. You need to use git commit --amend command to edit the commit message.

@ptkool
Copy link
Contributor Author

ptkool commented Dec 21, 2019

@mbasmanova Commit message has been updated. Can you review this again?

@mbasmanova
Copy link
Contributor

@ptkool Hmm... I don't see commit message updated. Here is what I see.

Screen Shot 2019-12-21 at 8 50 05 AM

@ptkool ptkool force-pushed the fix_decimal_is_distinct_from branch from 6f212b3 to aa170b8 Compare December 27, 2019 12:09
@ptkool ptkool force-pushed the fix_decimal_is_distinct_from branch from aa170b8 to cea8be9 Compare December 27, 2019 12:14
@ptkool
Copy link
Contributor Author

ptkool commented Dec 27, 2019

@mbasmanova Commit message has been fixed.

@mbasmanova
Copy link
Contributor

@ptkool Thank you for the contribution.

@mbasmanova mbasmanova merged commit 1a3d5ff into prestodb:master Jan 2, 2020
@aweisberg aweisberg mentioned this pull request Jan 17, 2020
7 tasks
@ptkool ptkool deleted the fix_decimal_is_distinct_from branch January 18, 2020 12:09
@caithagoras caithagoras mentioned this pull request Jan 22, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IS DISTINCT FROM returns incorrect result for decimal values with precision > 18
3 participants