Fix `getNextValue()` to increase the number of milliseconds if needed #18588

findinpath · 2023-08-08T15:47:58Z

Description

This is a potential correctness issue when using as input

LongTimestampWithTimeZone[epochMillis=1483228799999, picosOfMilli=999000000, timeZoneKey=0]

getNextValue() will return

Optional[LongTimestampWithTimeZone[epochMillis=1483228799998, picosOfMilli=0, timeZoneKey=0]]

even though it should have been returning

Optional[LongTimestampWithTimeZone[epochMillis=1483228800000, picosOfMilli=0, timeZoneKey=0]]

Additional context and related issues

Cherry-picked from #14648

PR contains as well a cherry-pick from #18594 for JUnit5 related changes.

Release notes

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

# Iceberg
* Fix partition filter subsuming when dealing with `timestamp with time zone` columns. ({issue}`issuenumber`)

alexjo2144

Good catch

martint · 2023-08-08T16:00:31Z

Is there a test that reproduces the issue?

findinpath · 2023-08-08T16:19:55Z

Is there a test that reproduces the issue?

I stumbled over it on BaseIcebergConnectorTest#testYearTransformDate() on #14648

        assertThat(query("SELECT * FROM test_year_transform_timestamptz WHERE d BETWEEN with_timezone(DATE '2015-01-01', 'UTC') AND with_timezone(TIMESTAMP '2016-12-31 23:59:59.999999', 'UTC')"))
                .isFullyPushedDown();

I can add however a test in io.trino.type.TestLongTimestampWithTimeZoneType

findinpath · 2023-08-08T16:57:05Z

core/trino-main/src/test/java/io/trino/type/TestIntegerType.java

@@ -55,6 +56,7 @@ protected Object getGreaterValue(Object value)
    }

    @Override
+    @Test


Can we ensure automatically that this doesn't happen?
The method is silently ignored if not annotated with @Test

We shouldn’t be overriding tests like that. If necessary, add new tests specific for the type.

I don't follow.
This is pre-existing functionality.

Please add more detail to the comment.
Simply create a new method instead of doing overrides?

I'm just responding to your comment about "ensuring automatically it doesn't happen". Structuring tests that way is not a good practice. It makes it hard to reason about which tests apply where (in some cases, the tests in AbstractXXX apply, in others they are completely overwritten, etc. -- usual arguments about using inheritance for reusing behavior in other contexts). We should fix them separately and not do that going forward.

That is how ATDQ, BCT and other similar "super" test classes are designed to work. We'll need to rethink how to structure those before those can be migrated over safely - since without that it's hard to have re-usable tests which also evolve over time (e.g. testRange is overridden today to throw SkipException but in future once the feature is there the override can be removed).

cc: @ksobolew any ideas to how to get this to work or how to restructure tests to avoid subclassing when using JUnit5?

Well, I can see that the implementations in the base class assert that the relevant properties of the type are Optional.empty(). They could, instead, do some assertions if it's not empty, and use some protected methods to get the expected result. But I'm not sure if that would work with all the types. It's kind of similar to how hasBehavior is used in connector tests...

I can see there's already #18594 :)

That is how ATDQ, BCT and other similar "super" test classes are designed to work. We'll need to rethink how to structure those before those can be migrated over safely

I can think of many different, more "pure" approaches.
The whole idea of BCT is impure and we could remove it. Connectors should have tests for features they implement.

Of course, this is straw-man's proposal. While letting us free of test overrides, this would lead to much worse test coverage.

alexjo2144 · 2023-08-08T17:15:28Z

core/trino-main/src/test/java/io/trino/type/TestBigintType.java

@@ -16,6 +16,7 @@
 import io.trino.spi.block.Block;
 import io.trino.spi.block.BlockBuilder;
 import io.trino.spi.type.Type.Range;
+import org.junit.jupiter.api.Test;


For me, these all run properly as is if you change the parent class AbstractTestType to use the org.testng.annotations.Test annotation instead of the junit org.junit.jupiter.api.Test annotation.

Indeed, but that’s not how junit works — and we’re in the process of replacing testng with junit

We should invest into writing a checker for this for the transition period like we have multiple for TestNG already. It seems very easy to do a migration but not realize multiple tests aren't running anymore.

Yeah, this is a huge landmine. I already stepped on it some time ago and we briefly discussed countermeasures, but nothing came out of it. I suppose we could write an Error Prone check for this (I am surprised that there is no one already).

Requirement to put @Test on overridden test is just junit's "ux bug", or a quirk, and agreed that we definitely need a checker for that. Thanks @ebyhr for reporting #14027.

findinpath · 2023-08-11T04:39:28Z

Rebased on master to re-use the commits from #18594

core/trino-spi/src/main/java/io/trino/spi/type/LongTimestampWithTimeZoneType.java

core/trino-main/src/test/java/io/trino/type/TestLongTimestampWithTimeZoneType.java

…noseconds

cla-bot bot added the cla-signed label Aug 8, 2023

findinpath requested a review from findepi August 8, 2023 15:48

findinpath added bug Something isn't working no-release-notes This pull request does not require release notes entry labels Aug 8, 2023

findinpath requested review from alexjo2144 and anusudarsan August 8, 2023 15:49

findinpath self-assigned this Aug 8, 2023

alexjo2144 approved these changes Aug 8, 2023

View reviewed changes

findinpath force-pushed the findinpath/fix-next-value branch from 2b3dd7c to 138c307 Compare August 8, 2023 16:56

findinpath commented Aug 8, 2023

View reviewed changes

findinpath force-pushed the findinpath/fix-next-value branch from 138c307 to 4d6ae7d Compare August 8, 2023 17:04

alexjo2144 reviewed Aug 8, 2023

View reviewed changes

ebyhr mentioned this pull request Aug 9, 2023

Add checker for JUnit @Test annotation in overridden tests #14027

Closed

findepi added the correctness label Aug 9, 2023

findinpath force-pushed the findinpath/fix-next-value branch 2 times, most recently from e237c0a to 755d5e6 Compare August 11, 2023 04:36

findinpath requested a review from martint August 11, 2023 04:37

findepi approved these changes Aug 11, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/type/LongTimestampWithTimeZoneType.java Show resolved Hide resolved

core/trino-main/src/test/java/io/trino/type/TestLongTimestampWithTimeZoneType.java Show resolved Hide resolved

findinpath force-pushed the findinpath/fix-next-value branch from 9224a3a to 7a2bfab Compare August 11, 2023 08:21

findinpath added 2 commits August 11, 2023 13:22

Fix getNextValue() to increase the number of milliseconds if needed

1d2a2e1

Test getPreviousValue() on a timestamp with 0 milliseconds and 0 na…

d9f6b90

…noseconds

findinpath force-pushed the findinpath/fix-next-value branch from 7a2bfab to d9f6b90 Compare August 11, 2023 11:26

findinpath mentioned this pull request Aug 11, 2023

Rewrite casts in between predicate #14648

Open

martint mentioned this pull request Aug 11, 2023

Fix structure and execution of type tests #18594

Merged

findinpath requested a review from findepi August 11, 2023 22:05

findepi approved these changes Aug 14, 2023

View reviewed changes

findepi merged commit d2046df into trinodb:master Aug 14, 2023

github-actions bot added this to the 424 milestone Aug 14, 2023

findepi mentioned this pull request Aug 14, 2023

Release notes for 424 #18638

Closed

colebow mentioned this pull request Aug 16, 2023

Add Trino 424 release notes #18704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `getNextValue()` to increase the number of milliseconds if needed #18588

Fix `getNextValue()` to increase the number of milliseconds if needed #18588

findinpath commented Aug 8, 2023 •

edited

Loading

alexjo2144 left a comment

martint commented Aug 8, 2023

findinpath commented Aug 8, 2023

findinpath Aug 8, 2023

martint Aug 8, 2023

findinpath Aug 8, 2023

martint Aug 8, 2023 •

edited

Loading

hashhar Aug 8, 2023

hashhar Aug 9, 2023

ksobolew Aug 9, 2023

ksobolew Aug 9, 2023

findepi Aug 9, 2023

alexjo2144 Aug 8, 2023

martint Aug 8, 2023

hashhar Aug 8, 2023

ksobolew Aug 9, 2023

findepi Aug 9, 2023

findinpath commented Aug 11, 2023

Fix getNextValue() to increase the number of milliseconds if needed #18588

Fix getNextValue() to increase the number of milliseconds if needed #18588

Conversation

findinpath commented Aug 8, 2023 • edited Loading

Description

Additional context and related issues

Release notes

alexjo2144 left a comment

Choose a reason for hiding this comment

martint commented Aug 8, 2023

findinpath commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented Aug 11, 2023

Fix `getNextValue()` to increase the number of milliseconds if needed #18588

Fix `getNextValue()` to increase the number of milliseconds if needed #18588

findinpath commented Aug 8, 2023 •

edited

Loading

martint Aug 8, 2023 •

edited

Loading