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 distinct/groupby on UUID #1180

Merged
merged 1 commit into from Aug 7, 2019

Conversation

@guyco33
Copy link
Contributor

commented Jul 25, 2019

Fixes issue #1170

@cla-bot cla-bot bot added the cla-signed label Jul 25, 2019

@findepi findepi requested a review from dain Jul 25, 2019

@findepi

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@dain would it be possible to extend AbstractTestType to catch this type of problems in a generic way?

The class already attempts to cover .compareTo in

if (type.isOrderable()) {
assertTrue(ASC_NULLS_FIRST.compareBlockValue(type, block, position, expectedBlock, 0) == 0);
assertTrue(ASC_NULLS_LAST.compareBlockValue(type, block, position, expectedBlock, 0) == 0);
assertTrue(DESC_NULLS_FIRST.compareBlockValue(type, block, position, expectedBlock, 0) == 0);
assertTrue(DESC_NULLS_LAST.compareBlockValue(type, block, position, expectedBlock, 0) == 0);
}
else {
try {
type.compareTo(block, position, expectedBlock, 0);
fail("Expected UnsupportedOperationException");
}
catch (UnsupportedOperationException expected) {
}
}

@findepi

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@dain would it be possible to extend AbstractTestType to catch this type of problems in a generic way?

especially that, as @Praveen2112 has checked, same problem occurs for IPADDRESS type too.

@@ -272,4 +272,14 @@ public void testMixedDistinctWithFilter()
"FROM (VALUES (1, 3), (2, 4), (2, 4), (4, 5)) t (x, y)",
"VALUES (BIGINT '0', CAST(NULL AS BIGINT))");
}

@Test
public void testUuidDistinct()

This comment has been minimized.

Copy link
@Praveen2112

Praveen2112 Jul 25, 2019

Member

Can we add a similar test for testIPAddressDistinct() as it faces similar issue

This comment has been minimized.

Copy link
@findepi

findepi Jul 25, 2019

Member

@Praveen2112 I wish none of these was necessary -- #1180 (comment)

This comment has been minimized.

Copy link
@guyco33

guyco33 Aug 4, 2019

Author Contributor

Can we add a similar test for testIPAddressDistinct() as it faces similar issue

@Praveen2112 There's no issue with distinct and group by for IPAddress
This one works:
SELECT DISTINCT ipaddress_col FROM (VALUES (IPADDRESS '10.0.0.1'),(IPADDRESS '10.0.0.1')) AS t (ipaddress_col)

and IpAddressDistinctFromOperator#isDistinctFrom is not called

@electrum

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Nit: the commit title is too long (notice how GitHub truncates it). Please limit to ~50 characters. We follow this guide: https://chris.beams.io/posts/git-commit/

You can shorten the title and add the detail to the description:

Fix distinct/groupby on UUID

Add compareTo implementation in Int128ArrayBlockBuilder

@guyco33 guyco33 force-pushed the guyco33:fix_1170 branch from 349613c to e93fd51 Jul 26, 2019

@dain

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

@dain would it be possible to extend AbstractTestType to catch this type of problems in a generic way?

It would be nice, but the problem is not in the Type itself and instead exists in the isDistinctFrom operator. IMO, using block.compareTo is not a good approach, and I'd use return UUID.equalTo(left, leftPosition, right, rightPosition); instead.

That said, if you can come up with a simple way to add this to the abstract type test, I fully support that.

public int compareTo(int leftPosition, int leftOffset, int leftLength, Block rightBlock, int rightPosition, int rightOffset, int rightLength)
{
checkReadablePosition(leftPosition);
int compare = Long.compare(getLong(leftPosition, SIZE_OF_LONG), rightBlock.getLong(rightPosition, SIZE_OF_LONG));

This comment has been minimized.

Copy link
@dain

dain Jul 27, 2019

Member

I believe this is the first code in this class that assumes a specific encoding of 128 numbers. In this case It would be 128bit little endian, so this would need to be documented somewhere.

BTW, the only reason we have compareTo (and equalsTo) was to make equals and comparison of VARCHAR and VARBINAY more efficient by removing the need to create a Slice object. Not for this PR... but we should check if this is still a win, and if so, is there a better way to do this now that doesn't require these methods.

This comment has been minimized.

Copy link
@guyco33

guyco33 Aug 6, 2019

Author Contributor

I believe this is the first code in this class that assumes a specific encoding of 128 numbers. In this case It would be 128bit little endian, so this would need to be documented somewhere.

My concern here is that it's not a pure 128bit little endian encoding, but a comparison of 2 big endian signed longs where the most significant one is on the right.

BTW, the only reason we have compareTo (and equalsTo) was to make equals and comparison of VARCHAR and VARBINAY more efficient by removing the need to create a Slice object. Not for this PR... but we should check if this is still a win, and if so, is there a better way to do this now that doesn't require these methods.

It's possible to skip now the compareTo by minor change in UuidDistinctFromOperator#isDistinctFrom
so, I will go with this solution leaving Int128ArrayBlockBuilder without any specific encoding assumption.

@guyco33 guyco33 force-pushed the guyco33:fix_1170 branch from e93fd51 to 0f3860f Aug 6, 2019

@guyco33 guyco33 changed the title Add compareTo implementation in Int128ArrayBlockBuilder to fix distinct/groupby on UUID Fix distinct/groupby on UUID Aug 6, 2019

@dain dain assigned dain and unassigned guyco33 Aug 6, 2019

@@ -199,7 +199,7 @@ public static boolean isDistinctFrom(
if (left.isNull(leftPosition)) {
return false;
}
return left.compareTo(leftPosition, 0, UUID.getFixedSize(), right, rightPosition, 0, UUID.getFixedSize()) != 0;
return UUID.compareTo(left, leftPosition, right, rightPosition) != 0;

This comment has been minimized.

Copy link
@dain

dain Aug 7, 2019

Member

I think you want !UUID. equalTo(left, leftPosition, right, rightPosition)

This comment has been minimized.

Copy link
@guyco33

guyco33 Aug 7, 2019

Author Contributor

Thanks @dain
Code is much cleaner now

@guyco33 guyco33 force-pushed the guyco33:fix_1170 branch from 0f3860f to ffe8b28 Aug 7, 2019

@dain

dain approved these changes Aug 7, 2019

@dain dain merged commit 2597804 into prestosql:master Aug 7, 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 Aug 7, 2019

Merged! Thanks!

@dain dain referenced this pull request Aug 7, 2019
2 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.