Convert TestMemSqlTypeMapping to use SqlDataTypeTest by Atema · Pull Request #7202 · trinodb/trino

Atema · 2021-03-07T22:16:16Z

Hej, we* did our best to resolve issue #6393 by converting the test in class testMemSqlTypeMapping to use SqlDataTypeTest instead of DataTypeTest.

We have succeeded on most methods, with one exception being the Real type (both in testBasicType() and testFloat(), which resulted in the exception below. I don't know where exactly it stems from, but I believe it may be worth further investigation.

[ERROR]   TestMemSqlTypeMapping.testBasicTypes:112 [Rows]
Expecting:
  <>
to contain exactly in any order:
  <[(found)]>
but could not find the following elements:
  <(found)>

[ERROR]   TestMemSqlTypeMapping.testFloat:119 [Rows]
Expecting:
  <>
to contain exactly in any order:
  <[(found)]>
but could not find the following elements:
  <(found)>

Let me know if you have any questions/comments.

* We are a group of five students (me, @caillouc, @hallon-heyman, @linnea-bonnevier, and @aoutir) who worked on this issue as a university assignment. All of us have sent in the CLA, and the ownership of the contributed code is personal.

Fixes #6393

…ataTypeTest

cla-bot · 2021-03-07T22:16:17Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

findepi · 2021-03-09T10:16:43Z

We are a group of five students (me, @caillouc, @hallon-heyman, @linnea-bonnevier, and @aoutir) who worked on this issue as a university assignment.

Very cool!

I guess your intention is to keep those contributions somewhat separate (instead of squashing into single commit, with Co-Authored-By).

You can also prefer to put up 5 separate PRs. Of course they would be conflicts, so we would need to review them one by one.

Or, I review this as is, let me know your preference.

Atema · 2021-03-09T11:02:04Z

All of us are fine with reviewing as a whole, and squashing the commits together!

findepi · 2021-03-10T10:50:17Z

@martint can you please help with the CLA verification here?

hashhar

Looks good to me. Requested a few changes in decimalTests (test for NULL is missing) and rest of it are editorial comments.

The SqlDataTypeTest failure with REAL seems concerning but it's not the tests fault, most likely some issue with toWriteMapping used for REAL (or MemSQL itself).

hashhar · 2021-03-11T05:45:41Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

-                .addRoundTrip(decimalDataType(38, 0), new BigDecimal("27182818284590452353602874713526624977"))
-                .addRoundTrip(decimalDataType(38, 0), new BigDecimal("-27182818284590452353602874713526624977"));
+        return SqlDataTypeTest.create()
+                .addRoundTrip("decimal(3, 0)", "CAST('193' AS decimal(3, 0))", createDecimalType(3, 0), "CAST('193' AS decimal(3, 0))")


Can you add a test where we try writing NULL into a decimal(3, 0) and another where we write NULL into a decimal(38, 0)?

hashhar · 2021-03-11T05:46:59Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

+                .addRoundTrip("char(1)", "'a'", createCharType(1), "CAST('a' AS char(1))")
+                .addRoundTrip("char(8)", "'abc'", createCharType(8), "CAST('abc' AS char(8))")
+                .addRoundTrip("char(8)", "'12345678'", createCharType(8), "CAST('12345678' AS char(8))")
+                .addRoundTrip("char(255)", String.format("'%s'", "a".repeat(255)), createCharType(255), String.format("CAST('%s' AS char(255))", "a".repeat(255)));


nit: Add static import for String.format.

hashhar · 2021-03-11T05:50:16Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

-                .addRoundTrip(varcharDataType(20000, CHARACTER_SET_UTF8), sampleUnicodeText)
+        String sampleUnicodeText = "'\u653b\u6bbb\u6a5f\u52d5\u968a'";
+        SqlDataTypeTest.create()
+                .addRoundTrip("tinytext " + CHARACTER_SET_UTF8, sampleUnicodeText, createVarcharType(255), "CAST(" + sampleUnicodeText + "AS varchar(255))")


nit: missing whitespace before AS inside quotes.

Suggested change

.addRoundTrip("tinytext " + CHARACTER_SET_UTF8, sampleUnicodeText, createVarcharType(255), "CAST(" + sampleUnicodeText + "AS varchar(255))")

.addRoundTrip("tinytext " + CHARACTER_SET_UTF8, sampleUnicodeText, createVarcharType(255), "CAST(" + sampleUnicodeText + " AS varchar(255))")

hashhar · 2021-03-11T05:52:28Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

-                .addRoundTrip(dateDataType, dateOfLocalTimeChangeForwardAtMidnightInJvmZone)
-                .addRoundTrip(dateDataType, dateOfLocalTimeChangeForwardAtMidnightInSomeZone)
-                .addRoundTrip(dateDataType, dateOfLocalTimeChangeBackwardAtMidnightInSomeZone);
+        for (String timeZoneId : ImmutableList.of(UTC_KEY.getId(), jvmZone.getId(), someZone.getId())) {


This loop works but for future reference take a look at PostgresqlTypeMappingTest to see how it uses DataProvider. The benefit is that if a test fails you can know exactly which timezone it failed for rather than having to guess which iteration of the loop it failed in.

No change requested since it's pre-existing. Can be a follow up.

hashhar · 2021-03-11T05:54:12Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

        return new CreateAndInsertDataSetup(memSqlServer::execute, tableNamePrefix);
    }

-    private static DataType<LocalDate> memSqlDateDataType(Function<LocalDate, String> toLiteral)


Thank you for cleaning up.

hashhar · 2021-03-11T05:57:46Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

+                .addRoundTrip("smallint", "32456", SMALLINT, "SMALLINT '32456'")
+                .addRoundTrip("tinyint", "125", TINYINT, "TINYINT '125'")
+                .addRoundTrip("double", "123.45", DOUBLE, "DOUBLE '123.45'")
+                // TODO: Real doesn't work with SqlDataTypeTest (see below)


Suggested change

// TODO: Real doesn't work with SqlDataTypeTest (see below)

// TODO: fails in SqlDataTypeTest#verifyPredicate

Same change in the comment in testFloat too.

hashhar · 2021-03-11T05:57:58Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

+                .addRoundTrip(memSqlFloatDataType(), null)
+                .execute(getQueryRunner(), memSqlCreateAndInsert("tpch.memsql_test_float"));
+
+        // TODO: Changing to SqlDataTypeTest with Real does not work: assertion in SqlDataTypeTest.verifyPredicate() fails


Remove the commented code. And move the TODO to the start of the function.

- Remove commented code & change comment - Add whitespae before AS - Add static String.format - Add NULL cases for decimal

cla-bot · 2021-03-13T11:31:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Atema · 2021-03-13T11:32:12Z

@hashhar Thanks for the comments. I've pushed the requested changes

hashhar

Looks good to be merged % the REAL data-type issue with verifyPredicate.

The issue with REAL type mapping is pre-existing so we can tackle that in a follow-up IMO.

hashhar · 2021-03-13T12:12:01Z

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

-                .addRoundTrip(varcharDataType(sampleUnicodeText.length(), CHARACTER_SET_UTF8), sampleUnicodeText)
-                .addRoundTrip(varcharDataType(32, CHARACTER_SET_UTF8), sampleUnicodeText)
-                .addRoundTrip(varcharDataType(20000, CHARACTER_SET_UTF8), sampleUnicodeText)
+        String sampleUnicodeText = "'\u653b\u6bbb\u6a5f\u52d5\u968a'";


nit: rename it to sampleUnicodeLiteral since the ' are not actually part of the tested text.

cla-bot · 2021-03-13T12:33:49Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

findepi · 2021-03-15T21:18:26Z

@martint can you please help with the CLA verification here?

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java

findepi · 2021-03-23T15:06:19Z

@martint can you please help with the CLA here?

ebyhr · 2021-09-25T12:45:59Z

@cla-bot check

cla-bot · 2021-09-25T12:46:02Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-09-25T12:46:03Z

The cla-bot has been summoned, and re-checked this pull request!

ebyhr · 2021-11-19T02:27:01Z

The SqlDataTypeTest failure with REAL seems concerning but it's not the tests fault, most likely some issue with toWriteMapping used for REAL (or MemSQL itself).

Confirmed that it's product bug in MySQL and MemSQL connectors. Filed #9998

ebyhr · 2021-12-28T06:48:04Z

Merged this commit in #10296. Thanks!

Atema and others added 5 commits March 4, 2021 15:27

#6393: Convert TestMemSqlTypeMapping unsigned, decimal & char to SqlD…

c67f796

…ataTypeTest

#6393: TestMemSqlTypeMapping param. char & varchar to SqlDataTypeTest

bc01a1d

#6393: TestMemSqlTypeMapping par. varchar to SqlDataTypeTest

c625ad3

#6393: TestMemSqlTypeMapping json and date to SqlDataTypeTest

770482e

#6393: TestMemSqlTypeMapping basic & floating to SqlDataTypeTest

03ccadf

Atema requested a review from findepi March 7, 2021 22:17

findepi requested a review from hashhar March 9, 2021 11:35

findepi changed the title ~~Convert TestMemSqlTypeMapping to use SqlDataTypeTest (#6393)~~ Convert TestMemSqlTypeMapping to use SqlDataTypeTest Mar 9, 2021

findepi approved these changes Mar 10, 2021

View reviewed changes

hashhar reviewed Mar 11, 2021

View reviewed changes

Fix-up for PR

bd9bf80

- Remove commented code & change comment - Add whitespae before AS - Add static String.format - Add NULL cases for decimal

Atema requested a review from hashhar March 13, 2021 11:32

hashhar approved these changes Mar 13, 2021

View reviewed changes

Rename sampleUnicodeText to -Literal

f40eec5

aidanBurton reviewed Mar 21, 2021

View reviewed changes

plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java Show resolved Hide resolved

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

findepi mentioned this pull request Aug 3, 2021

Convert TestMemSqlTypeMapping to use SqlDataTypeTest #6393

Closed

ebyhr mentioned this pull request Dec 14, 2021

Use SqlDataTypeTest in MemSQL connector #10296

Merged

ebyhr closed this Dec 28, 2021

	.addRoundTrip("tinytext " + CHARACTER_SET_UTF8, sampleUnicodeText, createVarcharType(255), "CAST(" + sampleUnicodeText + "AS varchar(255))")
	.addRoundTrip("tinytext " + CHARACTER_SET_UTF8, sampleUnicodeText, createVarcharType(255), "CAST(" + sampleUnicodeText + " AS varchar(255))")

	// TODO: Real doesn't work with SqlDataTypeTest (see below)
	// TODO: fails in SqlDataTypeTest#verifyPredicate

Conversation

Atema commented Mar 7, 2021 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-bot bot commented Mar 7, 2021

Uh oh!

findepi commented Mar 9, 2021

Uh oh!

Atema commented Mar 9, 2021

Uh oh!

findepi commented Mar 10, 2021

Uh oh!

hashhar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Mar 13, 2021

Uh oh!

Atema commented Mar 13, 2021

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Mar 13, 2021

Uh oh!

findepi commented Mar 15, 2021

Uh oh!

Uh oh!

findepi commented Mar 23, 2021

Uh oh!

ebyhr commented Sep 25, 2021

Uh oh!

cla-bot bot commented Sep 25, 2021

Uh oh!

cla-bot bot commented Sep 25, 2021

Uh oh!

ebyhr commented Nov 19, 2021

Uh oh!

ebyhr commented Dec 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

9 participants

Atema commented Mar 7, 2021 •

edited by findepi

Loading

hashhar left a comment •

edited

Loading

hashhar Mar 11, 2021 •

edited

Loading