Support automatic type coercion in Iceberg table creation #13981

mdesmet · 2022-09-03T22:57:34Z

Description

The connector can specify the actual Trino types that will be used to store the table for CTAS statements. This permits the core engine to perform the necessary coercions and casts.

Is this change a fix, improvement, new feature, refactoring, or other?

Enhancement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Change to Iceberg connector

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Fixes Support CTAS into Iceberg with timestamp(3) #6658

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

martint

This change has broader ranging implications around the semantics of CREATE TABLE and CREATE TABLE ... AS, and it's not as simple as adding support for precision <= 6 to the connector.

We had a discussion about this exact topic a couple of years ago, but I can't find it right now. @findepi, do you remember?

mdesmet · 2022-09-04T19:01:14Z

This change has broader ranging implications around the semantics of CREATE TABLE and CREATE TABLE ... AS, and it's not as simple as adding support for precision <= 6 to the connector.

Agreed, I'm not saying this PR is fully done or right at this moment.

The current behaviour in a CREATE TABLE is different from the CTAS. For example: the user created a table with TIMESTAMP(6), so we might say that the user opted for this precision and is OK with rounding (note that I have not checked the SQL spec on this but this is what is happening now).

create table iceberg.default.test (a TIMESTAMP(6));

insert into iceberg.default.test values TIMESTAMP '9999-12-31 23:59:59.999999999';
-- no error

select a from iceberg.default.test;
-- value is rounded

Now in the case of a CTAS statement this PR changes the behaviour to distinguish between the target table type and the query type.

The table type can be specified/overridden by the connector. For Iceberg we define the table type as TIMESTAMP(6) and the query type might be either lower, same or higher precision.

I think what is missing is that we should throw an exception in case of loss of precision. I thought this was the goal of the noTruncationCast but it just does a CAST for anything non VARCHAR/CHAR which might result in precision loss which should be explicit.

As far as I can see that is the impact, actually if a connector doesn't implement metadata.getNewTableMetadata(), there shouldn't be any impact as the table and query type is the same.

findepi · 2022-09-05T10:51:03Z

We had a discussion about this exact topic a couple of years ago, but I can't find it right now. @findepi, do you remember?

@martint Yeah, don't know where it was (slack, PRs or issues).

AFAIR we discussed being able to CTAS into a JDBC connector and we decided we will allow users to export data, even if we cannot create columns of requested types (eg more or less precision than requested). This was the driving use-case (obviously at odds with our desire to create table schema exactly as requested) and this is how it got implemented in JDBC connectors.

For reference, this is probably the first commit implementing this logic: 425c3b0 (part of #5342).

Now in the case of a CTAS statement this PR changes the behaviour to distinguish between the target table type and the query type.

This is a good improvement, as it allows us to differentiate between CTAS's implicit schema and CT explicit schema and handle them differently, addressing the above mentioned desire to create table schema with types exactly as requested by the user, or fail if it's not possible.

Involving engine in the conversion is a good change too. In 425c3b0 (and other such implementations), the connector needs to take care of CAST's rounding logic upon CTAS and doesn't have to do this upon iNSERT. This is unnecessary complexity. This is also much harder for connectors like Iceberg and Hive, where the conversion would be implemented in the file writers layer, or come at a performance cost.

Letting the connector tell the engine "hey, this is the effective table schema" during CTAS frees the connector from that logic.

I think what is missing is that we should throw an exception in case of loss of precision.

Just like in INSERT, we should round.
This is what we deliberately chose to do in JDBC connectors.

cc @losipiuk

findepi · 2022-09-05T10:52:53Z

@mdesmet I'd suggest to separate Allow connectors to specify tableMetadata for CTAS into its own PR. This should come with simplification changes in the JDBC connectors, thus exercising the logic being introduced.
Once we have this, we should return to the Iceberg portion of the changes.

mdesmet · 2022-09-05T21:51:34Z

@mdesmet I'd suggest to separate Allow connectors to specify tableMetadata for CTAS into its own PR. This should come with simplification changes in the JDBC connectors, thus exercising the logic being introduced. Once we have this, we should return to the Iceberg portion of the changes.

Created #13994

bitsondatadev · 2023-03-20T00:04:20Z

@mdesmet I'd suggest to separate Allow connectors to specify tableMetadata for CTAS into its own PR. This should come with simplification changes in the JDBC connectors, thus exercising the logic being introduced. Once we have this, we should return to the Iceberg portion of the changes.

Created #13994

@mdesmet
Any update here?

findinpath · 2023-09-04T19:00:26Z

Build is 🔴

BaseConnectorTest.testSetColumnTypes:2938 expected [time(3)] but found [time(6)]

findinpath · 2023-09-04T19:20:59Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TypeConverter.java

            return Types.TimeType.get();
        }
-        if (type.equals(TIMESTAMP_MICROS)) {
+        if (type instanceof TimestampType timestampType) {
+            if (timestampType.getPrecision() > 6) {


Considering that the table foo has a column col of type timestamp(6), why would we allow statements like the following:

ALTER TABLE foo ALTER COLUMN col SET DATA TYPE timestamp(3)

?

My understanding is that even when internally iceberg stores time values using microseconds precision. We could still allow any precision in Trino.

However I agree this goes beyond the scope of this PR, I will restore the existing behaviour.

findinpath · 2023-09-05T12:11:26Z

Build is still 🔴

Still the same method is failing: TestIcebergParquetConnectorTest>BaseConnectorTest.testSetFieldTypes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

findinpath

Tested successfully with manually on my local machine.
There is still a bit of testing to be added.
Looks good overall.

findinpath · 2023-09-15T08:33:33Z

Build is 🔴

2023-09-15T07:41:29.8421334Z [ERROR] io.trino.plugin.iceberg.TestIcebergMinioOrcConnectorTest.testTimestampPrecisionOnCreateTableAsSelect[TIMESTAMP '1969-12-31 23:59:59.9999994', timestamp(6), TIMESTAMP '1969-12-31 23:59:59.999999'](25) -- Time elapsed: 0.113 s <<< FAILURE!
2023-09-15T07:41:29.8422428Z java.lang.AssertionError: 
2023-09-15T07:41:29.8422880Z For query 20230915_074023_07236_8ek3b: 
2023-09-15T07:41:29.8423359Z  SELECT * FROM test_coercion_show_create_table8cn1wk1x4e
2023-09-15T07:41:29.8423781Z not equal
2023-09-15T07:41:29.8424161Z Actual rows (up to 100 of 1 extra rows shown, 1 rows in total):
2023-09-15T07:41:29.8424588Z     [1970-01-01T00:00:00.999999]
2023-09-15T07:41:29.8424961Z Expected rows (up to 100 of 1 missing rows shown, 1 rows in total):
2023-09-15T07:41:29.8425505Z     [1969-12-31T23:59:59.999999]

mdesmet · 2023-09-19T08:19:00Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorTest.java

+    {
+        if (setup.sourceValueLiteral().equals("TIMESTAMP '1969-12-31 23:59:59.999999499999'")) {
+            return Optional.of(setup.withNewValueLiteral("TIMESTAMP '1970-01-01 00:00:00.999999'"));
+        }
+        if (setup.sourceValueLiteral().equals("TIMESTAMP '1969-12-31 23:59:59.9999994'")) {
+            return Optional.of(setup.withNewValueLiteral("TIMESTAMP '1970-01-01 00:00:00.999999'"));
+        }
+        return Optional.of(setup);


@findepi : Do you know if this is something ORC related?

not sure i have context. this being what exactly?

Providing a timestamp like TIMESTAMP '1969-12-31 23:59:59.9999994 and TIMESTAMP '1969-12-31 23:59:59.999999499999' as an input value for a CTAS statement results in a Iceberg table with value TIMESTAMP '1970-01-01 00:00:00.999999.

Is this expected for ORC files?

this looks like a bug. i remember there were some +/-1s for at-the-epoch values in ORC, but @dain will know more

are you able to reproduce this without coercions, i.e. with direct insert of a timestamp(6) value into Iceberg?

This seems also prevalent in Hive:

trino:default> CREATE table r AS SELECT TIMESTAMP '1969-12-31 23:59:59.999' a; CREATE TABLE: 1 row Query 20230920_082205_00009_62iqv, FINISHED, 1 node Splits: 18 total, 18 done (100,00%) 0,42 [0 rows, 0B] [0 rows/s, 0B/s] trino:default> select * from r; a ------------------------- 1970-01-01 00:00:00.999 (1 row) Query 20230920_082212_00010_62iqv, FINISHED, 1 node Splits: 1 total, 1 done (100,00%) 0,31 [1 rows, 259B] [3 rows/s, 849B/s]

mdesmet · 2023-09-19T09:01:42Z

@martint : We have completed the implementation of the engine changes in #13994. Could you remove the requested changes on this PR as this is just the connector specific implementation.

findinpath · 2023-09-20T09:00:57Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorTest.java

+            return Optional.of(setup.withNewValueLiteral("TIMESTAMP '1970-01-01 00:00:00.999999'"));
+        }
+        if (setup.sourceValueLiteral().equals("TIMESTAMP '1969-12-31 23:59:59.9999994'")) {
+            return Optional.of(setup.withNewValueLiteral("TIMESTAMP '1970-01-01 00:00:00.999999'"));


Please add a comment why this happens

Required engine changes were implemented elsewhere

mdesmet · 2023-09-21T04:33:35Z

@findepi : PTAL

findepi · 2023-10-09T10:35:37Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+    public Optional<io.trino.spi.type.Type> getSupportedType(ConnectorSession session, io.trino.spi.type.Type type)
+    {
+        if (type instanceof TimestampWithTimeZoneType) {
+            return Optional.of(TIMESTAMP_TZ_MICROS);


Support time types with precison <= 6 in Iceberg CT and CTAS

Change title

replace time types with date/time types

replace with precison <= 6 with with precison != 6

i changed the PR title to much commit title.

Similar to trinodb#13981, extend type coercion support to char type.

Similar to #13981, extend type coercion support to char type.

cla-bot bot added the cla-signed label Sep 3, 2022

mdesmet force-pushed the feature/typecoercion_iceberg_ctas branch from 6f29241 to 4993fb1 Compare September 4, 2022 09:58

martint previously requested changes Sep 4, 2022

View reviewed changes

mdesmet mentioned this pull request Sep 6, 2022

Implement snapshot materialization starburstdata/dbt-trino#120

Merged

4 tasks

hashhar mentioned this pull request Sep 12, 2022

Support automatic type coercion for table creation #13994

Merged

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

bitsondatadev mentioned this pull request Mar 20, 2023

Support CTAS into Iceberg with timestamp(3) #6658

Closed

mdesmet force-pushed the feature/typecoercion_iceberg_ctas branch from 4993fb1 to 9ccbae3 Compare September 4, 2023 17:03

github-actions bot added the iceberg Iceberg connector label Sep 4, 2023

findinpath reviewed Sep 4, 2023

View reviewed changes

mdesmet force-pushed the feature/typecoercion_iceberg_ctas branch from 9ccbae3 to 6bdafb9 Compare September 12, 2023 03:03