Test datetime type in MariaDB connector #20241

abusk · 2023-12-29T10:28:44Z

Description

Is this change a fix, improvement, new feature, refactoring, or other?
improvement
Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
MariaDB connector
How would you describe this change to a non-technical end user or system administrator?
Support datetime type in MariaDB connector

Additional context and related issues

Fixes #17242

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

cla-bot · 2023-12-29T10:28:47Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2023-12-29T11:36:59Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbClient.java

ebyhr · 2023-12-29T13:28:49Z

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/TestMariaDbTypeMapping.java

+                .build();
+
+        SqlDataTypeTest.create()
+                .addRoundTrip("datetime(3)", "'1958-01-01 13:18:03.123'", createTimestampType(3), "TIMESTAMP '1958-01-01 13:18:03.123'")


Please update testTimestamp(ZoneId sessionZone) method to accept both timestamp and datetime.

MariaDB stores values that use the DATETIME data type in a format that supports values between 1000-01-01 00:00:00.000000 and 9999-12-31 23:59:59.999999 which is different from TIMESTAMP (https://mariadb.com/kb/en/timestamp/#supported-values). As the test cases has different ranges of values, so it is better to keep in separate.

Please leave the code comment and add roundtip with min and max values.

Also, we can still extract the common roundtrips.

Yea, adding a common roudrips method for datetime and timestamp test.

docs/src/main/sphinx/connector/mariadb.md

ebyhr · 2023-12-29T13:40:18Z

Added Support for datetime type in MariaDB connector

datetime is already supported as far as I know. Also, please avoid past tense in the commit title.
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/BaseMariaDbConnectorTest.java

cla-bot · 2024-01-01T10:25:40Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-01-01T11:22:04Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/TestMariaDbTypeMapping.java

cla-bot · 2024-01-03T10:27:37Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-01-03T11:57:23Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

abusk · 2024-01-06T09:21:40Z

@ebyhr please review the latest PR

ebyhr

Please squash commits into one and fix the commit title as "Test datetime type in MariaDB connector"

plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbClient.java

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/TestMariaDbTypeMapping.java

ebyhr · 2024-01-12T06:25:15Z

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/TestMariaDbTypeMapping.java

+                // min value 1970-01-01 00:00:00
+                .addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:01.000'", createTimestampType(3), "TIMESTAMP '1970-01-01 00:00:01.000'")
+                .addRoundTrip("timestamp(6)", "TIMESTAMP '1970-01-01 00:00:01.000000'", createTimestampType(6), "TIMESTAMP '1970-01-01 00:00:01.000000'")


The min value in a comment is different from the roundtrip.

ebyhr · 2024-01-12T06:35:36Z

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/TestMariaDbTypeMapping.java

+                // min value 1000-01-01 00:00:00
+                .addRoundTrip("datetime(3)", "TIMESTAMP '1000-01-01 00:00:00.000'", createTimestampType(3), "TIMESTAMP '1000-01-01 00:00:00.000'")


This value isn't the min value in MariaDB as far as I confirmed:

create table tpch.test(a datetime); insert into tpch.test values (TIMESTAMP '0999-12-31 00:00:00.000'); select * from tpch.test; -- 0999-12-31 00:00:00.000

Ah it is supporting min value of 0001-01-01 00:00:00. I followed this doc https://mariadb.com/kb/en/datetime/#supported-values where mentioned min value is 1000-01-01 00:00:00

github-actions bot added the docs label Dec 29, 2023

ebyhr reviewed Dec 29, 2023

View reviewed changes

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/BaseMariaDbConnectorTest.java Outdated Show resolved Hide resolved

abusk changed the title ~~Added Support for datetime type in MariaDB connector~~ Add Support for datetime type in MariaDB connector Jan 1, 2024

ebyhr reviewed Jan 2, 2024

View reviewed changes

plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/TestMariaDbTypeMapping.java Outdated Show resolved Hide resolved

ebyhr assigned abusk Jan 2, 2024

abusk requested a review from ebyhr January 3, 2024 10:42

ebyhr changed the title ~~Add Support for datetime type in MariaDB connector~~ Test datetime type in MariaDB connector Jan 8, 2024

ebyhr reviewed Jan 8, 2024

View reviewed changes

abusk force-pushed the support_mariadb_datetime branch from a79d296 to ca41544 Compare January 11, 2024 07:02

cla-bot bot added the cla-signed label Jan 11, 2024

abusk force-pushed the support_mariadb_datetime branch from ca41544 to acc7ca7 Compare January 11, 2024 07:11

ebyhr reviewed Jan 12, 2024

View reviewed changes

abusk force-pushed the support_mariadb_datetime branch from acc7ca7 to 66471f6 Compare January 12, 2024 07:08

ebyhr approved these changes Jan 14, 2024

View reviewed changes

Test datetime type in MariaDB connector

ea13678

ebyhr force-pushed the support_mariadb_datetime branch from 66471f6 to ea13678 Compare January 14, 2024 23:44

ebyhr merged commit a604d20 into trinodb:master Jan 14, 2024
4 of 13 checks passed

github-actions bot added this to the 437 milestone Jan 14, 2024

colebow mentioned this pull request Jan 17, 2024

Add Trino 437 release notes #20404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test datetime type in MariaDB connector #20241

Test datetime type in MariaDB connector #20241

abusk commented Dec 29, 2023 •

edited by ebyhr

Loading

cla-bot bot commented Dec 29, 2023

cla-bot bot commented Dec 29, 2023

ebyhr Dec 29, 2023

abusk Jan 1, 2024

ebyhr Jan 2, 2024 •

edited

Loading

abusk Jan 3, 2024

ebyhr commented Dec 29, 2023

cla-bot bot commented Jan 1, 2024

cla-bot bot commented Jan 1, 2024

cla-bot bot commented Jan 3, 2024

cla-bot bot commented Jan 3, 2024

abusk commented Jan 6, 2024

ebyhr left a comment

ebyhr Jan 12, 2024

ebyhr Jan 12, 2024

abusk Jan 12, 2024 •

edited

Loading

abusk Jan 12, 2024

		// min value 1000-01-01 00:00:00
		.addRoundTrip("datetime(3)", "TIMESTAMP '1000-01-01 00:00:00.000'", createTimestampType(3), "TIMESTAMP '1000-01-01 00:00:00.000'")

Test datetime type in MariaDB connector #20241

Test datetime type in MariaDB connector #20241

Conversation

abusk commented Dec 29, 2023 • edited by ebyhr Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Dec 29, 2023

cla-bot bot commented Dec 29, 2023

ebyhr Dec 29, 2023

Choose a reason for hiding this comment

abusk Jan 1, 2024

Choose a reason for hiding this comment

ebyhr Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

abusk Jan 3, 2024

Choose a reason for hiding this comment

ebyhr commented Dec 29, 2023

cla-bot bot commented Jan 1, 2024

cla-bot bot commented Jan 1, 2024

cla-bot bot commented Jan 3, 2024

cla-bot bot commented Jan 3, 2024

abusk commented Jan 6, 2024

ebyhr left a comment

Choose a reason for hiding this comment

ebyhr Jan 12, 2024

Choose a reason for hiding this comment

ebyhr Jan 12, 2024

Choose a reason for hiding this comment

abusk Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

abusk Jan 12, 2024

Choose a reason for hiding this comment

abusk commented Dec 29, 2023 •

edited by ebyhr

Loading

ebyhr Jan 2, 2024 •

edited

Loading

abusk Jan 12, 2024 •

edited

Loading