Fix timestamp to varchar coercer in hive tables #17604

Praveen2112 · 2023-05-23T09:38:33Z

Description

Timestamp to varchar coercion which was introduced in #16869 has a few edge cases which are not covered.

Support for non-default precision in case of partitioned tables.
In case of partitioned tables - we don't consider
Restrict support for historical dates
Hive 2.+ and Hive 3.+ uses java.sql.Timestamp#toString for coercing Timestamp
to Varchar types. java.sql.Timestamp#toString doesn't capture the historical dates correctly

Additional context and related issues

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
* Fix some things. ({issue}`issuenumber`)

Use == for enum comparison

plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/TimestampCoercer.java

Praveen2112 · 2023-05-29T10:59:52Z

@findepi Can you please take a look at this PR during your spare time.

krvikash · 2023-05-30T05:32:01Z

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java

could be private?

It allows us to override the values for a specific distribution of hive or a customized egine

krvikash · 2023-05-30T05:34:28Z

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java

could be private?

It allows us to override the values for a specific distribution of hive or a customized egine

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java

findepi · 2023-06-01T08:41:53Z

...ct-tests/src/main/java/io/trino/tests/product/hive/TestHiveCoercionOnUnpartitionedTable.java

        // TODO: These expected failures should be fixed.
        return ImmutableMap.<ColumnContext, String>builder()
+                // Expected failures from BaseTestHiveCoercion
+                .putAll(super.expectedExceptionsWithTrinoContext())


this is meaningless since super is empty right now.
squash with commit that changes super

this is now squashed, but remains meaningless, as super remains empty.

maybe make super abstract instead?

I'll remove that method instead for now we don't have any exceptions in the base class which has to be skipped in each implementation.

findepi · 2023-06-01T08:43:57Z

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java

we should return same results as hive, not "somewhat similar"

the column is varchar. if hive returns 2121-07-15 15:30:12.123499999 (29 characters), Trino should also return 2121-07-15 15:30:12.123499999 (29 characters).

remove this method

So should it be irrespective of the timestamp precision specified as a part session or config property ?

Hive timestamp doesn't have the concept of precision - how do we handle them in trino

So should it be irrespective of the timestamp precision specified as a part session or config property ?

i believe so

Hive timestamp doesn't have the concept of precision -

i know. they are always nanoseconds precision

how do we handle them in trino

regular timestamps get truncated to selected precision for performance reasons
for columns that were timestamps and are now varchars, we don't have these performance considerations and we should be compatible with hive.

findepi

92e4572#r1212807610

Get type directly from HiveColumnHandle instead of generating them from HiveType.

This will be irrespective of the precision configured or specified as session property.

Praveen2112 · 2023-06-02T13:57:37Z

@findepi I have addressed the comments - Now we are returning same values as that of hive i.e we are treating it as nanoseconds irrespective of coercion specified in config or session property.

Praveen2112 · 2023-06-02T13:59:47Z

If the overall PR is okay - I could extract them into two

findepi · 2023-06-05T10:56:31Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSource.java

-        Type fromType = fromHiveType.getType(typeManager, timestampPrecision);
+        // Hive treats TIMESTAMP with NANOSECONDS precision and when we try to coerce from a timestamp column,
+        // we read it as TIMESTAMP(9) column and coerce accordingly.
+        Type fromType = fromHiveType.getType(typeManager, HiveTimestampPrecision.NANOSECONDS);


instead of changing here, let's inject this on return Optional.of(new LongTimestampToVarcharCoercer(timestampType, varcharType)); line, replacing timestampType with TIMESATAMP_NANOS

findepi · 2023-06-05T10:59:45Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSourceProvider.java

-                                    fromHiveType.getType(typeManager));
+                                    // Hive treats TIMESTAMP with NANOSECONDS precision and when we try to coerce from a timestamp column,
+                                    // we read it as TIMESTAMP(9) column and coerce accordingly.
+                                    fromHiveType.getType(typeManager, HiveTimestampPrecision.NANOSECONDS));


what if fromHiveType is a Row/Struct and timestamp field isn't being coerced.
can injecting NANOS leak here?

in fact, i don't know why change here at all

Changing here would allows to read the coerced column as NANO from the underlying file - is there any extension point we could use so as to force the column to be read as NANO ?

fromHiveType.getType(typeManager) doesn't consider the precision from the session or config - so aren't we injecting MILLIS - which is mapped to DEFAULT precision here ?

fromHiveType.getType(typeManager) doesn't consider the precision from the session or config

that's why it was deprecated. the code before the changes wasn't good

but i don't know why NANOSECONDS is always good here.
is this applied for coerced columns only?

If we didn't apply NANOSECONDS then the data from the underlying ConnectorPageSource is read as MILLISECONDS - and we can't get a LongTimestamp object from the block - IIUC this applied if the coerced column is a timestamp column, I'll double check for the complex column like STRUCT - but without this change we would be leaking MILLI - it is not intentional right ?

findepi · 2023-06-05T10:59:51Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSourceProvider.java

                                columnHandle.getBaseHiveColumnIndex(),
                                fromHiveTypeBase,
-                                fromHiveTypeBase.getType(typeManager),
+                                fromHiveTypeBase.getType(typeManager, HiveTimestampPrecision.NANOSECONDS),


(same here)

I think we can continue the discussion here - #17604 (comment)

findepi · 2023-06-05T11:00:27Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/orc/OrcTypeTranslator.java

-            }
+            // Hive treats TIMESTAMP with NANOSECONDS precision and when we try to coerce from a timestamp column,
+            // we read it as TIMESTAMP(9) column and coerce accordingly.
+            TimestampType timestampType = createTimestampType(HiveTimestampPrecision.NANOSECONDS.getPrecision());


use TIMESTAMP_NANOS

findepi · 2023-06-05T11:02:53Z

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java

+        }
+    }
+
+    protected Map<String, List<Object>> expectedRowsForEngineProvider(Engine engine, HiveTimestampPrecision hiveTimestampPrecision)


expectedRowsForEngineProvider no longer uses its parameters (which is good)

inline
and replace expectedTinoResults and expectedHiveResults with one variable

findepi · 2023-06-05T11:05:10Z

...ct-tests/src/main/java/io/trino/tests/product/hive/TestHiveCoercionOnUnpartitionedTable.java

        // TODO: These expected failures should be fixed.
        return ImmutableMap.<ColumnContext, String>builder()
+                // Expected failures from BaseTestHiveCoercion
+                .putAll(super.expectedExceptionsWithTrinoContext())


this is now squashed, but remains meaningless, as super remains empty.

maybe make super abstract instead?

findepi · 2023-06-05T11:06:37Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/coercions/TestTimestampCoercer.java

add 0001-01-01 00:00 test case. this is favorite users' date

For historical dates we don't support coercion so this is the closest favorite data

This will be irrespective of the precision configured or specified as session property.

Hive 2.+ and Hive 3.+ uses `java.sql.Timestamp#toString` for coercing Timestamp to Varchar types. `java.sql.Timestamp#toString` doesn't capture the historical dates correctly

Minor cleanup in OrcTypeTranslator

4f0aecc

Use == for enum comparison

Praveen2112 requested review from atanasenko, findepi, huberty89, kokosing and krvikash May 23, 2023 09:38

cla-bot bot added the cla-signed label May 23, 2023

github-actions bot added hive Hive connector tests:hive labels May 23, 2023

Praveen2112 mentioned this pull request May 23, 2023

Add support for varchar to timestamp coercer in hive tables #17071

Closed

Praveen2112 force-pushed the praveen/timestamp_string_coercion_fix branch 5 times, most recently from 11f847e to e3ac258 Compare May 24, 2023 05:10

huberty89 reviewed May 24, 2023

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/TimestampCoercer.java Outdated Show resolved Hide resolved

Praveen2112 force-pushed the praveen/timestamp_string_coercion_fix branch from e3ac258 to 744bcd3 Compare May 24, 2023 07:01

krvikash reviewed May 30, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/timestamp_string_coercion_fix branch from 744bcd3 to f204670 Compare May 31, 2023 11:39

findepi reviewed Jun 1, 2023

View reviewed changes

findepi requested changes Jun 1, 2023

View reviewed changes

Praveen2112 requested a review from hashhar June 1, 2023 08:54

Praveen2112 force-pushed the praveen/timestamp_string_coercion_fix branch from f204670 to f6b307a Compare June 2, 2023 11:43

Praveen2112 added 2 commits June 2, 2023 18:02

Cleanup in RcFilePageSourceFactory

0596d32

Get type directly from HiveColumnHandle instead of generating them from HiveType.

For Timestamp to String coercions treat precision as nanoseconds

34a105e

This will be irrespective of the precision configured or specified as session property.

Praveen2112 force-pushed the praveen/timestamp_string_coercion_fix branch from f6b307a to 5427d60 Compare June 2, 2023 13:56

findepi reviewed Jun 5, 2023

View reviewed changes

Praveen2112 added 2 commits June 5, 2023 16:56

fixup! For Timestamp to String coercions treat precision as nanoseconds

c64d36b

This will be irrespective of the precision configured or specified as session property.

Restrict timestamp to varchar coercion for historical dates

030c40c

Hive 2.+ and Hive 3.+ uses `java.sql.Timestamp#toString` for coercing Timestamp to Varchar types. `java.sql.Timestamp#toString` doesn't capture the historical dates correctly

Praveen2112 force-pushed the praveen/timestamp_string_coercion_fix branch from 5427d60 to 030c40c Compare June 5, 2023 11:33

Praveen2112 closed this Jul 26, 2023

Fix timestamp to varchar coercer in hive tables #17604

Fix timestamp to varchar coercer in hive tables #17604

Uh oh!

Conversation

Praveen2112 commented May 23, 2023

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

Praveen2112 commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Jun 2, 2023

Uh oh!

Praveen2112 commented Jun 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Praveen2112 commented May 29, 2023 •

edited

Loading