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.

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

Jump to bottom

Round TimeWithTimeZone when supportsParametricDateTime is false #14950

Merged
merged 3 commits into from
Nov 9, 2022
Merged

Round TimeWithTimeZone when supportsParametricDateTime is false #14950

merged 3 commits into from
Nov 9, 2022

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Nov 8, 2022

Description

When PARAMETRIC_DATETIME is not set, all temporal types should be rounded up to precision 3. However TimeWithTimeZone is not rounded.

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Round TimeWithTimeZone values when `PARAMETRIC_DATETIME` is not set

@cla-bot cla-bot bot added the cla-signed label Nov 8, 2022
hashhar
hashhar reviewed Nov 8, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

FYI @findepi/@martint

core/trino-main/src/main/java/io/trino/server/protocol/QueryResultRows.java
Comment on lines +370 to +373

if (type instanceof TimeWithTimeZoneType) {
return ((SqlTimeWithTimeZone) value).roundTo(3);
}
Copy link
Member

Choose a reason for hiding this comment

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

good catch. I didn't notice this myself when I shared the link. 👍

@mdesmet mdesmet marked this pull request as ready for review November 8, 2022 17:30
martint
martint requested changes Nov 8, 2022
View reviewed changes
core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTime.java Outdated
*/
package io.trino.spi.type;

import org.testng.annotations.Test;
Copy link
Member

Choose a reason for hiding this comment

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

Use junit for new tests.

core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTimeWithTimeZone.java Outdated
Comment on lines 26 to 38
assertThat(newInstance(0, 0, 1).toString()).isEqualTo("00:00:00+00:01");
assertThat(newInstance(1, 0, 1).toString()).isEqualTo("00:00:00.0+00:01");
assertThat(newInstance(2, 0, 1).toString()).isEqualTo("00:00:00.00+00:01");
assertThat(newInstance(3, 0, 1).toString()).isEqualTo("00:00:00.000+00:01");
assertThat(newInstance(4, 0, 1).toString()).isEqualTo("00:00:00.0000+00:01");
assertThat(newInstance(5, 0, 1).toString()).isEqualTo("00:00:00.00000+00:01");
assertThat(newInstance(6, 0, 1).toString()).isEqualTo("00:00:00.000000+00:01");
assertThat(newInstance(7, 0, 1).toString()).isEqualTo("00:00:00.0000000+00:01");
assertThat(newInstance(8, 0, 1).toString()).isEqualTo("00:00:00.00000000+00:01");
assertThat(newInstance(9, 0, 1).toString()).isEqualTo("00:00:00.000000000+00:01");
assertThat(newInstance(10, 0, 1).toString()).isEqualTo("00:00:00.0000000000+00:01");
assertThat(newInstance(11, 0, 1).toString()).isEqualTo("00:00:00.00000000000+00:01");
assertThat(newInstance(12, 0, 1).toString()).isEqualTo("00:00:00.000000000000+00:01");
Copy link
Member

Choose a reason for hiding this comment

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

Don't convert to String before comparing. Compare directly against instances created with SqlTimeWithTimeZone.newInstance

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 agree in case of all other tests except here we don't apply any transformation, so we have to compare against another representation.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, I meant to put the comment in the methods below. For this one, rename the test to testToString, as that's what's being tested here.

Also, this test is unrelated to the fix in this commit, so move it to a separate commit.

core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTimeWithTimeZone.java Outdated
@@ -15,14 +15,66 @@

import org.testng.annotations.Test;

import static io.trino.spi.type.SqlTimeWithTimeZone.newInstance;
Copy link
Member

Choose a reason for hiding this comment

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

Don't static import this method. The name is too ambiguous.

core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTime.java
import static io.trino.spi.type.SqlTime.newInstance;
import static org.assertj.core.api.Assertions.assertThat;

public class TestSqlTime
Copy link
Member

Choose a reason for hiding this comment

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

This test is unrelated to the fix in this commit. Move it to a separate commit.

martint
martint reviewed Nov 8, 2022
View reviewed changes
core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTimeWithTimeZone.java Outdated
Comment on lines 26 to 38
assertThat(newInstance(0, 0, 1).toString()).isEqualTo("00:00:00+00:01");
assertThat(newInstance(1, 0, 1).toString()).isEqualTo("00:00:00.0+00:01");
assertThat(newInstance(2, 0, 1).toString()).isEqualTo("00:00:00.00+00:01");
assertThat(newInstance(3, 0, 1).toString()).isEqualTo("00:00:00.000+00:01");
assertThat(newInstance(4, 0, 1).toString()).isEqualTo("00:00:00.0000+00:01");
assertThat(newInstance(5, 0, 1).toString()).isEqualTo("00:00:00.00000+00:01");
assertThat(newInstance(6, 0, 1).toString()).isEqualTo("00:00:00.000000+00:01");
assertThat(newInstance(7, 0, 1).toString()).isEqualTo("00:00:00.0000000+00:01");
assertThat(newInstance(8, 0, 1).toString()).isEqualTo("00:00:00.00000000+00:01");
assertThat(newInstance(9, 0, 1).toString()).isEqualTo("00:00:00.000000000+00:01");
assertThat(newInstance(10, 0, 1).toString()).isEqualTo("00:00:00.0000000000+00:01");
assertThat(newInstance(11, 0, 1).toString()).isEqualTo("00:00:00.00000000000+00:01");
assertThat(newInstance(12, 0, 1).toString()).isEqualTo("00:00:00.000000000000+00:01");
Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, I meant to put the comment in the methods below. For this one, rename the test to testToString, as that's what's being tested here.

Also, this test is unrelated to the fix in this commit, so move it to a separate commit.

core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTime.java Outdated
public class TestSqlTime
{
@Test
public void testBaseline()
Copy link
Member

Choose a reason for hiding this comment

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

Rename to testToString

@mdesmet mdesmet force-pushed the bug/sqltime-not-rounded branch from a369a2d to f83ecc2 Compare November 8, 2022 21:17
@mdesmet mdesmet force-pushed the bug/sqltime-not-rounded branch from f83ecc2 to 4e42bd4 Compare November 8, 2022 21:34
martint
martint reviewed Nov 8, 2022
View reviewed changes
core/trino-spi/src/test/java/io/trino/spi/type/TestSqlTimeWithTimeZone.java Outdated
@@ -40,7 +40,40 @@ public void testToString()
@Test
public void testEqualsDifferentZone()
{
assertThat(SqlTimeWithTimeZone.newInstance(12, 0, 0))
.isNotEqualTo(SqlTimeWithTimeZone.newInstance(12, 0, 1));
assertThat(SqlTimeWithTimeZone.newInstance(12, 0, 0)).isNotEqualTo(SqlTimeWithTimeZone.newInstance(12, 0, 1));
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for that, removed!

@mdesmet mdesmet force-pushed the bug/sqltime-not-rounded branch from 4e42bd4 to 6a28dfb Compare November 9, 2022 13:50
@martint martint merged commit 811e435 into trinodb:master Nov 9, 2022
@github-actions github-actions bot added this to the 403 milestone Nov 9, 2022
@colebow colebow mentioned this pull request Nov 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar hashhar left review comments

@martint martint martint requested changes

Assignees
No one assigned
Labels
cla-signed
Milestone
403
Development

Successfully merging this pull request may close these issues.
3 participants
@mdesmet @martint @hashhar