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

Add time/timestamp 6 connector smoke test coverage #12577

Merged
merged 3 commits into from
Jun 1, 2022
Merged

Add time/timestamp 6 connector smoke test coverage #12577

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b270e4
Add time/timestamp 6 connector smoke test coverage
findepi May 27, 2022
42c4a8b
empty
findepi May 30, 2022
d713008
empty
findepi Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions plugin/trino-accumulo/src/test/java/io/trino/plugin/accumulo/TestAccumuloConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,10 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.startsWith("decimal(")
|| typeName.equals("time(6)")
Copy link
Member

Choose a reason for hiding this comment

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

Q: Would EnumSet be a better "logical" way to group these ? (this would be a change to how we usually do this - so this is just a question, not specifically about this particular PR/change)

private static final EnumSet UNSUPPORTED_TYPES = EnumSet.of(TIME_6, TIMESTAMP_6, TIMESTAMP_WITH_TZ_3, TIMESTAMP_WITH_TZ_6).addAll(EnumSet.range(DECIMAL_3_0, DECIMAL_38_0));
[...]
TrinoType type = TrinoType.fromName(typeName);
if (UNSUPPORTED_TYPES.contains(type) {
    return Optional.of(dataMappingTestSetup.asUnsupported());
}

Pros:

  • faster and less error prone (they work like a bitset / it's not a string comparison)
  • logic of what's supported is defined at the field level (here it's in a Test class, but still useful to have it grouped/defined in one place, rather than littered about in test - how do we know if we've changed all the if methods to include the new type?,etc,
    Cons:
  • harder to do "fuzzy" matching (e.g., startsWith("decimal")), but it's still possible to have a "fromName(String typeName)" which does that matching.
  • might be difficult to maintain across different DB/drivers (and a lot of boilerplate for types if we duplicate it for each driver).

Copy link
Member Author

Choose a reason for hiding this comment

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

With parametric types, there could be plenty of enums like this.

Copy link
Member

@leveyja leveyja Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

True, but the set is finite / reflects the types we support / do not support. It's the same as the number of String representations of types, except the metadata about the precision, etc, can be stored with the enum type, and there's a centralised place to see information about the types. (Find Usages can also be used in the IDE, instead of full text searching for logic related to a particular type, etc).

public enum TrinoType {
    [...]
    TEXT(Integer.MAX_VALUE/2, "Text type", ""),
    [...]
    TIME(?, "description", "other properties),
    TIME3(3, "description", "other properties),
    TIME6(6, "description", "other properties),
    [...]
    ;
    int precision;
    String description;
    Function<T, R> converter;
    public TrinoType(int precision, String description, Function<T, R> converter(?)) {
        this.precision = precision; this.description = description; // etc
   }
}

Some benefits:

  • A developer can create a set containing all time types using EnumSet.range(TIME3, TIMESTAMP6_TZ),
  • The set of supported / unsupported types can be declared in an EnumSet for each connector (there's also an EnumSet.noneOf(...) which creates the compliment of a set), allowing for declarative type support (not having to check if statements in multiple places for each connector (e.g., filterDataMappingSmokeTestData, etc).
  • Adding a "new" type would simply be a matter of adding it to "supported" or "unsupported" EnumSets.
  • Right now, in test code we have if (typeName.startsWith(...)) or .contains(...), which means adding a new type is not always safe / will match new types accidentally. It also means modifying if else logic in several places to define whether a type is supported/tested or not, and it's hard to spot "errors of omission" ( e.g., the test returns "Optional.empty()" if the type isn't recognised, and we just don't "see" that a type is missing in the review, etc.)

(Final random example from searching the code)

    @DataProvider
    public static Object[][] timestampTypes()
    {
        // Timestamp with timezone is not supported by the SqlServer connector
        return new Object[][] {
                {"timestamp"},
                {"timestamp(3)"},
                {"timestamp(6)"},
                {"timestamp(9)"},
                {"timestamp(12)"}
        };
    }

I'd prefer to see

public class SqlServerClient
{
...
public static final SUPPORTED_TIMESTAMP_TYPES = EnumSet.of(....);         // Timestamp with timezone is not supported by the SqlServer connector

}

public class TestSqlServerConnectorTest
{

    @DataProvider
    public static Object[][] timestampTypes()
    {
//        return asObjectArray(SqlServerClient.SUPPORTED_TIMESTAMP_TYPES);
        return SUPPORTED_TIMESTAMP_TYPES.stream()
                .map(trinoTypes -> new Object[]{trinoTypes.name()})
                .collect(Collectors.toList()).toArray(Object[][]::new); // could probably use a utility method for this :-)
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but the set is finite / reflects the types we support / do not support

The number of (parameterized) types we do support is more than 2 billion.

Of course, we don't want to test every each of them separately, so we're narrowing the list to types tested here.
Since it's single use, string matching isn't so bad an idea -- because, in the worst case, the test will test too much (not too little).

Copy link
Member

Choose a reason for hiding this comment

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

Right - but in code I see "time(6)" - I don't see "time(5)" - so these can be enumerated and be < 1000.
It's String Literal versus "Type" - I'm not suggesting we move to Scala, I'm suggesting we enumerate the types we support in a single place (again, this is a high-level question revolving around "is there a better way?", not directly related to this PR, etc). I'll investigate a bit more and if I come up with a way to consolidate this (as we have with BCT feature support) I'll propose something more concrete than these "review comment" suggestions 👍

|| typeName.equals("timestamp(6)")
|| typeName.equals("timestamp(3) with time zone")
|| typeName.equals("timestamp(6) with time zone")
|| typeName.startsWith("char(")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}
Expand Down
5 changes: 5 additions & 0 deletions ...n/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,11 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
// TODO this should either work or fail cleanly
return Optional.empty();
}
if (typeName.equals("time(6)") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}
return Optional.of(dataMappingTestSetup);
}

Expand Down
3 changes: 3 additions & 0 deletions ...rino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,8 +496,11 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
return Optional.empty();

case "time":
case "time(6)":
case "timestamp":
case "timestamp(6)":
case "timestamp(3) with time zone":
case "timestamp(6) with time zone":
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
7 changes: 6 additions & 1 deletion ...o-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeMinioConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,12 @@ protected Optional<DataMappingTestSetup> filterCaseSensitiveDataMappingTestData(
protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMappingTestSetup dataMappingTestSetup)
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("time") || typeName.equals("timestamp") || typeName.equals("char(3)")) {
if (typeName.equals("time") ||
typeName.equals("time(6)") ||
typeName.equals("timestamp") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(6) with time zone") ||
typeName.equals("char(3)")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}
return Optional.of(dataMappingTestSetup);
Expand Down
8 changes: 7 additions & 1 deletion plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8634,9 +8634,15 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("time")
|| typeName.equals("timestamp(3) with time zone")) {
|| typeName.equals("time(6)")
|| typeName.equals("timestamp(3) with time zone")
|| typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}
if (typeName.equals("timestamp(6)")) {
// It's supported depending on hive timestamp precision configuration, so the exception message doesn't match the expected for asUnsupported().
return Optional.empty();
}

return Optional.of(dataMappingTestSetup);
}
Expand Down
14 changes: 4 additions & 10 deletions plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2854,16 +2854,10 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
}

// According to Iceberg specification all time and timestamp values are stored with microsecond precision.
if (typeName.equals("time")) {
return Optional.of(new DataMappingTestSetup("time(6)", "TIME '15:03:00'", "TIME '23:59:59.999999'"));
}

if (typeName.equals("timestamp")) {
return Optional.of(new DataMappingTestSetup("timestamp(6)", "TIMESTAMP '2020-02-12 15:03:00'", "TIMESTAMP '2199-12-31 23:59:59.999999'"));
}

if (typeName.equals("timestamp(3) with time zone")) {
return Optional.of(new DataMappingTestSetup("timestamp(6) with time zone", "TIMESTAMP '2020-02-12 15:03:00 +01:00'", "TIMESTAMP '9999-12-31 23:59:59.999999 +12:00'"));
if (typeName.equals("time") ||
typeName.equals("timestamp") ||
typeName.equals("timestamp(3) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

return Optional.of(dataMappingTestSetup);
Expand Down
5 changes: 4 additions & 1 deletion plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,10 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("time")
|| typeName.equals("timestamp(3) with time zone")) {
|| typeName.equals("time(6)")
|| typeName.equals("timestamp(6)")
|| typeName.equals("timestamp(3) with time zone")
|| typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
6 changes: 4 additions & 2 deletions plugin/trino-mariadb/src/test/java/io/trino/plugin/mariadb/BaseMariaDbConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,10 @@ protected boolean isColumnNameRejected(Exception exception, String columnName, b
protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMappingTestSetup dataMappingTestSetup)
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("timestamp(3) with time zone")
|| typeName.equals("timestamp")) {
if (typeName.equals("timestamp") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(3) with time zone") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
13 changes: 13 additions & 0 deletions plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.Date;
import java.util.Optional;

import static java.lang.String.format;
import static java.nio.charset.StandardCharsets.UTF_8;
Expand Down Expand Up @@ -102,6 +103,18 @@ public void testSortItemsReflectedInExplain()
"TopNPartial\\[5 by \\(nationkey DESC");
}

@Override
protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMappingTestSetup dataMappingTestSetup)
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("time(6)") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}
return Optional.of(dataMappingTestSetup);
}

@Test(dataProvider = "guessFieldTypesProvider")
public void testGuessFieldTypes(String mongoValue, String trinoValue)
{
Expand Down
3 changes: 2 additions & 1 deletion plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ protected boolean isColumnNameRejected(Exception exception, String columnName, b
protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMappingTestSetup dataMappingTestSetup)
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("timestamp(3) with time zone")) {
if (typeName.equals("timestamp(3) with time zone") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
7 changes: 5 additions & 2 deletions plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,11 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
return Optional.empty();
}
}
if (typeName.equals("time")) {
return Optional.empty();
if (typeName.equals("time") ||
typeName.equals("time(6)") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}
if (typeName.equals("boolean")) {
// Oracle does not have native support for boolean however usually it is represented as number(1)
Expand Down
7 changes: 5 additions & 2 deletions plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,11 @@ protected boolean isColumnNameRejected(Exception exception, String columnName, b
protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMappingTestSetup dataMappingTestSetup)
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("timestamp")
|| typeName.equals("timestamp(3) with time zone")) {
if (typeName.equals("time(6)") ||
typeName.equals("timestamp") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(3) with time zone") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
7 changes: 5 additions & 2 deletions plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,11 @@ protected boolean isColumnNameRejected(Exception exception, String columnName, b
protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMappingTestSetup dataMappingTestSetup)
{
String typeName = dataMappingTestSetup.getTrinoTypeName();
if (typeName.equals("timestamp")
|| typeName.equals("timestamp(3) with time zone")) {
if (typeName.equals("time(6)") ||
typeName.equals("timestamp") ||
typeName.equals("timestamp(6)") ||
typeName.equals("timestamp(3) with time zone") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
3 changes: 3 additions & 0 deletions ...no-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
|| typeName.equals("real")
|| typeName.startsWith("decimal(")
|| typeName.equals("time")
|| typeName.equals("time(6)")
|| typeName.equals("timestamp(6)")
|| typeName.equals("timestamp(3) with time zone")
|| typeName.equals("timestamp(6) with time zone")
|| typeName.startsWith("char(")) {
//TODO this should either work or fail cleanly
return Optional.empty();
Expand Down
3 changes: 2 additions & 1 deletion ...o-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
return Optional.empty();
}

if (typeName.equals("timestamp(3) with time zone")) {
if (typeName.equals("timestamp(3) with time zone") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
3 changes: 2 additions & 1 deletion ...n/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
return Optional.empty();
}
}
if (typeName.equals("timestamp(3) with time zone")) {
if (typeName.equals("timestamp(3) with time zone") ||
typeName.equals("timestamp(6) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
3 changes: 3 additions & 0 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3335,8 +3335,11 @@ private List<DataMappingTestSetup> testDataMappingSmokeTestData()
.add(new DataMappingTestSetup("date", "DATE '1582-10-05'", "DATE '1582-10-14'")) // during julian->gregorian switch
.add(new DataMappingTestSetup("date", "DATE '2020-02-12'", "DATE '9999-12-31'"))
.add(new DataMappingTestSetup("time", "TIME '15:03:00'", "TIME '23:59:59.999'"))
.add(new DataMappingTestSetup("time(6)", "TIME '15:03:00'", "TIME '23:59:59.999999'"))
.add(new DataMappingTestSetup("timestamp", "TIMESTAMP '2020-02-12 15:03:00'", "TIMESTAMP '2199-12-31 23:59:59.999'"))
.add(new DataMappingTestSetup("timestamp(6)", "TIMESTAMP '2020-02-12 15:03:00'", "TIMESTAMP '2199-12-31 23:59:59.999999'"))
.add(new DataMappingTestSetup("timestamp(3) with time zone", "TIMESTAMP '2020-02-12 15:03:00 +01:00'", "TIMESTAMP '9999-12-31 23:59:59.999 +12:00'"))
.add(new DataMappingTestSetup("timestamp(6) with time zone", "TIMESTAMP '2020-02-12 15:03:00 +01:00'", "TIMESTAMP '9999-12-31 23:59:59.999999 +12:00'"))
.add(new DataMappingTestSetup("char(3)", "'ab'", "'zzz'"))
.add(new DataMappingTestSetup("varchar(3)", "'de'", "'zzz'"))
.add(new DataMappingTestSetup("varchar", "'łąka for the win'", "'ŻŻŻŻŻŻŻŻŻŻ'"))
Expand Down