Skip to content

IGNITE-16376: Sql. Add UUID custom data type.#1623

Merged
korlov42 merged 30 commits intoapache:mainfrom
gridgain:ignite-16376
Feb 16, 2023
Merged

IGNITE-16376: Sql. Add UUID custom data type.#1623
korlov42 merged 30 commits intoapache:mainfrom
gridgain:ignite-16376

Conversation

@lowka
Copy link
Contributor

@lowka lowka commented Feb 2, 2023

Adds UUID type.

@lowka lowka force-pushed the ignite-16376 branch 17 times, most recently from f957df4 to 559e02f Compare February 8, 2023 16:44
@lowka lowka marked this pull request as ready for review February 9, 2023 11:28
/**
* Tests for {@link org.apache.ignite.internal.sql.engine.type.UuidType} data type.
*/
public class ItUuidTest extends AbstractBasicIntegrationTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

will e good to add tests with UUID as Primary Key

Copy link
Contributor Author

@lowka lowka Feb 10, 2023

Choose a reason for hiding this comment

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

There are such tests in ItPublicApiColocationTest::colocationOneColumn

…l/engine/type/IgniteCustomType.java

Co-authored-by: Andrew V. Mashenkov <AMashenkov@users.noreply.github.com>
@AMashenkov AMashenkov changed the title IGNITE-16376: Investigate of support additional data types in Calcite. IGNITE-16376: Investigate of support additional data types in Calcite Feb 10, 2023
(short) typeNullable, false, (short) typeSearchable, false, false, false, "LONGVARBINARY", 0, 0,
Types.LONGVARBINARY, 0, null));

types.add(Arrays.asList("UUID", Types.OTHER, ColumnMetadata.UNDEFINED_PRECISION, "'", "'", null,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems we need additional test here ItJdbcMetadataSelfTest ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

assertQuery("SELECT ? = ?").withParams(uuid1, uuid2).returns(false).check();
assertQuery("SELECT ? != ?").withParams(uuid1, uuid2).returns(true).check();

assertQuery("SELECT ? > ?").withParams(uuid2, uuid1).returns(false).check();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertQuery("SELECT ? > ?").withParams(uuid2, uuid1).returns(false).check();
assertQuery("SELECT ? > ?").withParams(uuid2, uuid1).returns(false).check();
assertQuery("SELECT ? > ?").withParams(uuid1, uuid2).returns(true).check();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added additional checks that include for_less then and less_then_or_equals operators.

return Period.of((Integer) val / 12, (Integer) val % 12, 0);
} else if (storageType == byte[].class && val instanceof ByteString) {
return ((ByteString) val).getBytes();
} else if (storageType == UUID.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it just for assertion check and if so - is assertion is enough here?

Copy link
Contributor Author

@lowka lowka Feb 13, 2023

Choose a reason for hiding this comment

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

It is not sufficient because else branch is called for all other type/value combinations.
And if guard for UUID tests that a value of UUID data type is UUID and nothing else.

@lowka lowka changed the title IGNITE-16376: Investigate of support additional data types in Calcite IGNITE-16376: Sql. Add UUID custom data type. Feb 16, 2023
@korlov42 korlov42 merged commit 5918d4e into apache:main Feb 16, 2023
@korlov42 korlov42 deleted the ignite-16376 branch February 16, 2023 09:54
lowka added a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023
Co-authored-by: Andrew V. Mashenkov <AMashenkov@users.noreply.github.com>
Co-authored-by: korlov42 <korlov@gridgain.com>
lowka added a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
Co-authored-by: Andrew V. Mashenkov <AMashenkov@users.noreply.github.com>
Co-authored-by: korlov42 <korlov@gridgain.com>
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.

5 participants