Skip to content

Add UUID ordering tests #12913

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.

Sign up for GitHub

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

Merged
findepi merged 2 commits into from
Jun 21, 2022
Merged

Add UUID ordering tests #12913

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdcb928
Fix TestUuidType.getGreaterValue
findepi Jun 20, 2022
a3a3b17
Add UUID ordering tests
findepi Jun 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion core/trino-main/src/test/java/io/trino/type/TestUuidType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,23 @@
import io.airlift.slice.Slices;
import io.trino.spi.block.Block;
import io.trino.spi.block.BlockBuilder;
import io.trino.spi.type.TypeOperators;
import org.testng.annotations.Test;

import java.lang.invoke.MethodHandle;

import static io.airlift.slice.SizeOf.SIZE_OF_LONG;
import static io.airlift.slice.Slices.utf8Slice;
import static io.trino.spi.function.InvocationConvention.InvocationArgumentConvention.BLOCK_POSITION;
import static io.trino.spi.function.InvocationConvention.InvocationArgumentConvention.NEVER_NULL;
import static io.trino.spi.function.InvocationConvention.InvocationReturnConvention.FAIL_ON_NULL;
import static io.trino.spi.function.InvocationConvention.simpleConvention;
import static io.trino.spi.predicate.Utils.nativeValueToBlock;
import static io.trino.spi.type.UuidType.UUID;
import static io.trino.spi.type.UuidType.javaUuidToTrinoUuid;
import static io.trino.type.UuidOperators.castFromVarcharToUuid;
import static java.lang.Long.reverseBytes;
import static org.assertj.core.api.Assertions.assertThat;
import static org.testng.Assert.assertEquals;

public class TestUuidType
Expand All @@ -47,7 +58,7 @@ public static Block createTestBlock()
protected Object getGreaterValue(Object value)
{
Slice slice = (Slice) value;
return Slices.wrappedLongArray(slice.getLong(0), slice.getLong(SIZE_OF_LONG) + 1);
return Slices.wrappedLongArray(slice.getLong(0), reverseBytes(reverseBytes(slice.getLong(SIZE_OF_LONG)) + 1));
}

@Override
Expand All @@ -61,4 +72,46 @@ public void testDisplayName()
{
assertEquals(UUID.getDisplayName(), "uuid");
}

@Test
public void testJavaUuidToTrinoUuid()
{
assertThat(javaUuidToTrinoUuid(java.util.UUID.fromString("00000000-0000-0000-0000-000000000001")))
.isEqualTo(castFromVarcharToUuid(utf8Slice("00000000-0000-0000-0000-000000000001")));

assertThat(javaUuidToTrinoUuid(java.util.UUID.fromString("f79c3e09-677c-4bbd-a479-3f349cb785e7")))
.isEqualTo(castFromVarcharToUuid(utf8Slice("f79c3e09-677c-4bbd-a479-3f349cb785e7")));
}

@Test
public void testOrdering()
throws Throwable
{
String lowerAsString = "406caec7-68b9-4778-81b2-a12ece70c8b1";
String higherAsString = "f79c3e09-677c-4bbd-a479-3f349cb785e7";
java.util.UUID lower = java.util.UUID.fromString(lowerAsString);
java.util.UUID higher = java.util.UUID.fromString(higherAsString);
// Java UUID's comparison is not consitent with RFC 4122, see https://bugs.openjdk.org/browse/JDK-7025832
assertThat(higher).isLessThan(lower);
Copy link
Copy Markdown
Member

@martint martint Jun 20, 2022

Choose a reason for hiding this comment

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

What's the purpose of testing the JDK implementation of isLessThan? I don't see anything below that relies on this behavior.

Copy link
Copy Markdown
Member Author

@findepi findepi Jun 21, 2022

Choose a reason for hiding this comment

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

The purpose was to show and document acknowledgement to the fact that JDK UUID behaves differently than our implementation, and also to anchor the explanation, if someone would wonder in the future who's right.

Copy link
Copy Markdown
Member

@martint martint Jun 21, 2022

Choose a reason for hiding this comment

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

But we don't use Java UUIDs for ordering, so how is that behavior relevant? If anything, such documentation should be added to the UUID type class so that anyone using it in their connector is aware. This comment is going to be largely invisible for that purpose.

Copy link
Copy Markdown
Member Author

@findepi findepi Jun 21, 2022

Choose a reason for hiding this comment

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

But we don't use Java UUIDs for ordering

we use it in H2QueryRunner (implicitly)
and in Iceberg library (implicitly, apparently)

such documentation should be added to the UUID type class so that anyone using it in their connector is aware

This isn't supposed to be consumer facing documentation.
It's supposed to capture information for future Trino maintainers, including future myself

Copy link
Copy Markdown
Member

@martint martint Jun 21, 2022

Choose a reason for hiding this comment

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

Given that we have javaUuidToTrinoUuid/trinoUuidToJavaUuid public methods in UuidType, that information seems relevant for anyone manipulating UUIDs, no?

Copy link
Copy Markdown
Member Author

@findepi findepi Jun 21, 2022

Choose a reason for hiding this comment

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

I agree that there can be some other place where the information would be useful too.
it doesn't make it unnecessary in the uuid ordering test.

Copy link
Copy Markdown
Contributor

@kbendick kbendick Jun 21, 2022

Choose a reason for hiding this comment

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

As somebody who tends to look at unit tests quite often when getting an understanding of things, I do appreciate this form of documentation for something a bit... quirky / not intuitively obvious when looking at the UUID type class directly.

This could possibly be documented elsewhere as mentioned, but I agree with @findepi that it has utility as a unit test. Particularly, the rest of the test seems to testing that two Trino UUID's are comparable as well as how to do it. If I were working on a feature (say for the Iceberg connector), this is amongst the first places I would likely go for looking at using the code.

If this isn't going to be user facing documentation, then I think this comparison test for the JDK UUIDs is beneficial. Though a comment in UuidType would also suffice for me - but that's more up to you guys. I think the rest of the test is beneficial though.


Slice lowerSlice = javaUuidToTrinoUuid(lower);
Slice higherSlice = javaUuidToTrinoUuid(higher);

MethodHandle compareByValue = new TypeOperators().getComparisonUnorderedFirstOperator(UUID, simpleConvention(FAIL_ON_NULL, NEVER_NULL, NEVER_NULL));
long comparisonByValue = (long) compareByValue.invoke(lowerSlice, higherSlice);
assertThat(comparisonByValue)
.as("value comparison operator result")
.isLessThan(0);

MethodHandle compareFromBlock = new TypeOperators().getComparisonUnorderedFirstOperator(UUID, simpleConvention(FAIL_ON_NULL, BLOCK_POSITION, BLOCK_POSITION));
long comparisonFromBlock = (long) compareFromBlock.invoke(nativeValueToBlock(UUID, lowerSlice), 0, nativeValueToBlock(UUID, higherSlice), 0);
assertThat(comparisonFromBlock)
.as("block-position comparison operator result")
.isLessThan(0);

// UUID ordering should be consistent with lexicographical order of unsigned bytes the UUID is comprised of
assertThat(lowerSlice)
.as("comparing slices lexicographically")
.isLessThan(higherSlice);
}
}