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

Support setting column types in PostgreSQL connector #15591

Merged

ebyhr merged 1 commit into trinodb:master from ebyhr:ebi/jdbc-change-column-type

Jan 10, 2023

Member

ebyhr commented Jan 4, 2023

Description

Relates to #15515

Additional context and related issues

Release notes

(x) Release notes are required, with the following suggested text:

# PostgreSQL
* Add support for changing column types. ({issue}`15515`)

cla-bot bot added the cla-signed label

github-actions bot added the tests:hive label

ebyhr mentioned this pull request

Implement ALTER COLUMN SET DATA TYPE statement in connectors #15515

Closed

10 tasks

ebyhr force-pushed the ebi/jdbc-change-column-type branch from abf6c4e to 433df5f Compare

January 4, 2023 09:44

ebyhr marked this pull request as ready for review

January 5, 2023 01:11

ebyhr force-pushed the ebi/jdbc-change-column-type branch 2 times, most recently from 976659d to 7458bc8 Compare

January 5, 2023 03:48

ebyhr requested review from findepi and hashhar

January 5, 2023 03:48

findepi approved these changes

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

    
                      JdbcTableHandle tableHandle = (JdbcTableHandle) table;

                      JdbcColumnHandle columnHandle = (JdbcColumnHandle) column;

                      verify(!tableHandle.isSynthetic(), "Not a table reference: %s", tableHandle);

                      jdbcClient.setColumnType(session, tableHandle, columnHandle, type);

Member

findepi Jan 5, 2023

jdbcClient can be given the table name (JdbcNamedRelationHandle), not the full handle

Member Author

ebyhr Jan 6, 2023

The context of existing full handle seems that third-party developers may use authorization field. Let me keep as-is in this PR.

Member

hashhar Jan 6, 2023

cc: @Praveen2112 It seems if authorization field is non-empty we should fail all operations?

Member

Praveen2112 Jan 6, 2023

Yes.

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java Outdated

    
                  protected SetColumnTypeSetup applyColumnMapping(SetColumnTypeSetup setup)

                  {

                      if (setup.oldColumnType().equals("timestamp(3) with time zone")) {

                          return new SetColumnTypeSetup(setup.oldColumnType(), setup.newColumnType(), setup.oldValueLiteral(), "timestamp '2020-02-12 14:03:00.123000 +00:00'");

Member

findepi Jan 5, 2023

Add a comment

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                              .build();

                  }

                  public record SetColumnTypeSetup(String oldColumnType, String newColumnType, String oldValueLiteral, String newValueLiteral) {}

Member

findepi Jan 5, 2023

add null checks

public SetColumnTypeSetup
{
    requireNonNull(oldColumnType, "oldColumnType is null");
    requireNonNull(newColumnType, "newColumnType is null");
    requireNonNull(oldValueLiteral, "oldValueLiteral is null");
    requireNonNull(newValueLiteral, "newValueLiteral is null");
}

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

    
                  public void testSetColumnType(SetColumnTypeSetup setup)

                  {

                      if (!hasBehavior(SUPPORTS_SET_COLUMN_TYPE)) {

                          assertQueryFails("ALTER TABLE nation ALTER COLUMN nationkey SET DATA TYPE bigint", "This connector does not support setting column types");

Member

findepi Jan 5, 2023

the check ignores @DataProvider (and that's fine), so it should be invoked once
not @DataProvider's element count times

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                                  .matches("VALUES " + setup.newValueLiteral);

                      }

                      finally {

                          assertUpdate("DROP TABLE IF EXISTS " + tableName);

Member

findepi Jan 5, 2023

Use TestTable please

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                              .add(new SetColumnTypeSetup("smallint", "integer", "32767", "32767"))

                              .add(new SetColumnTypeSetup("integer", "bigint", "2147483647", "bigint '2147483647'"))

                              .add(new SetColumnTypeSetup("bigint", "integer", "-2147483648", "-2147483648"))

                              .add(new SetColumnTypeSetup("decimal(5,3)", "decimal(38,3)", "12.345", "12.345"))

Member

findepi Jan 5, 2023

for decimals, let's exercise
short -> short
long -> long
short -> long

also, we should exercise precision reduction

when actual values can be converted without loss
when actual values need to be rounded (i think they should round, instead of faliing)

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                              .add(new SetColumnTypeSetup("decimal(5,3)", "decimal(38,3)", "12.345", "12.345"))

                              .add(new SetColumnTypeSetup("date", "date", "date '2023-01-04'", "date '2023-01-04'"))

                              .add(new SetColumnTypeSetup("time(3)", "time(6)", "time '15:03:00.123'", "time '15:03:00.123000'"))

                              .add(new SetColumnTypeSetup("timestamp(3)", "timestamp(6)", "timestamp '2020-02-12 15:03:00.123'", "timestamp '2020-02-12 15:03:00.123000'"))

Member

findepi Jan 5, 2023

Add tests with time/timestamp precision reduction

when actual values can be converted without loss
when actual values need to be rounded (i think they should round, instead of faliing)

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                              .add(new SetColumnTypeSetup("timestamp(3)", "timestamp(6)", "timestamp '2020-02-12 15:03:00.123'", "timestamp '2020-02-12 15:03:00.123000'"))

                              .add(new SetColumnTypeSetup("timestamp(3) with time zone", "timestamp(6) with time zone", "timestamp '2020-02-12 15:03:00.123 +01:00'", "timestamp '2020-02-12 15:03:00.123000 +01:00'"))

                              .add(new SetColumnTypeSetup("char(100)", "varchar", "'char-to-varchar'", "'char-to-varchar'"))

                              .add(new SetColumnTypeSetup("varchar", "char(100)", "'varchar-to-char'", "cast('varchar-to-char' as char(100))"))

Member

findepi Jan 5, 2023

add char & varchar tests with length reduction

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                              .collect(toDataProvider());

                  }

                  protected SetColumnTypeSetup applyColumnMapping(SetColumnTypeSetup setup)

Member

findepi Jan 5, 2023

Can we give this method a better name?

also

how a connector can say this particular conversion isn't support in this connector case? (ie turn positive test case into a negative one)
- see io.trino.testing.BaseConnectorTest.DataMappingTestSetup#asUnsupported
how a connector can say this particular conversion is problematic, needs to be skipped (TODO + issue)?
- see io.trino.testing.BaseConnectorTest#filterDataMappingSmokeTestData, it returns Optional for this reason

Member Author

ebyhr Jan 6, 2023

how a connector can say this particular conversion isn't support in this connector case? (ie turn positive test case into a negative one)

Let me handle this in follow-up as the logic is unreachable in this PR.

how a connector can say this particular conversion is problematic, needs to be skipped (TODO + issue)?

Changed to optional type.

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

    
                  }

                  @DataProvider

                  public Object[][] setColumnTypesProvider()

Member

findepi Jan 5, 2023

Provider -> DataProvider (at least that's my convention)

ebyhr force-pushed the ebi/jdbc-change-column-type branch 2 times, most recently from 2a73a62 to b60541d Compare

January 6, 2023 02:17

ebyhr self-assigned this

hashhar reviewed

View reviewed changes

Member

hashhar left a comment

I don't have useful comments - just about enum ordering. Sorry.

Looks good otherwise.

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseConnectorTest.java Outdated

Comment on lines 73 to 75

    
                          case SUPPORTS_SET_COLUMN_TYPE:

                              return false;

Member

hashhar Jan 6, 2023

above SUPPORTS_DELETE?

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java Outdated

    
            @@ -87,6 +87,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_COMMENT_ON_COLUMN:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT?

...rino-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java Outdated

    
            @@ -96,6 +96,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_COMMENT_ON_COLUMN:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftConnectorTest.java

    
            @@ -70,6 +70,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_CREATE_TABLE_WITH_COLUMN_COMMENT:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT?

...trino-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java

    
            @@ -86,6 +86,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_ADD_COLUMN_WITH_COMMENT:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

    
            @@ -105,6 +105,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_ADD_COLUMN_WITH_COMMENT:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT?

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

    
            @@ -76,6 +76,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_ADD_COLUMN_WITH_COMMENT:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

    
            @@ -78,6 +78,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_ADD_COLUMN_WITH_COMMENT:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT

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

    
            @@ -66,6 +66,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
          
                          case SUPPORTS_ADD_COLUMN_WITH_COMMENT:

                              return false;

                          case SUPPORTS_SET_COLUMN_TYPE:

Member

hashhar Jan 6, 2023

together with ADD_COLUMN_WITH_COMMENT

ebyhr force-pushed the ebi/jdbc-change-column-type branch 2 times, most recently from d657f05 to e217f11 Compare

January 6, 2023 07:28


          Support setting column types in PostgreSQL connector

ebyhr force-pushed the ebi/jdbc-change-column-type branch from e217f11 to 5575667 Compare

January 6, 2023 12:32

ebyhr merged commit bbfa269 into trinodb:master

ebyhr deleted the ebi/jdbc-change-column-type branch

January 10, 2023 01:04

github-actions bot added this to the 406 milestone

ebyhr mentioned this pull request

Release notes for 406 #15563

Closed

colebow mentioned this pull request

Add Trino 406 release notes #15625

Merged

Member

colebow commented Jan 10, 2023

@ebyhr should we add any docs for this?

Member Author

ebyhr commented Jan 10, 2023 •

edited

Loading

@colebow We need to refactor the docs first as the current granularity is ALTER TABLE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels