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

Praveen2112 · 2023-04-04T07:04:26Z

Description

Add support for timestamp to varchar coercer in hive tables. In case of partitioned table it is supported by most of the format and in case of unpartitioned tables only ORC format is supported as of now.

The coercion that was supported as of current master for unpartitioned tables are inherently supported by the ColumnReader and this PR introduces a framework which maps a OrcTypeKind to a corresponding TrinoType and also re-uses the TypeCoercer used by partitioned tables.

Additional context and related issues

Release notes

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

# Section
* Add support for timestamp to varchar coercer in hive tables

Praveen2112 · 2023-04-04T07:05:14Z

Will be adding some additional test coverage for TimestampCoercer but wanted to have a early feedback here for the design/approach.

skrzypo987

Looks ok

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

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

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

Praveen2112 · 2023-04-11T13:03:15Z

@skrzypo987 AC

skrzypo987

lgtm

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

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

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

findepi · 2023-04-19T08:51:01Z

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

i have an IDE warning on this line

Can you share the warning here - Mine is not showing any warning.

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

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

findepi · 2023-04-19T09:06:53Z

...uct-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/configs/ConfigHdp3.java

what is this for?

When doing implicit coercion to timestamp, hive cast to a zone specific String - so wanted it to be in sync with UTC (like we do in Trino). This is only specific to ORC file for other formats it doesn't consider the timezone of the HiveServer.

quick google search suggests that TZ env variable means time zone name. /usr/share/zoneinfo/UTC looks like a file path, not a time zone name. How does it work?

When doing implicit coercion to timestamp, hive cast to a zone specific String - so wanted it to be in sync with UTC (like we do in Trino).

would this information be useful for future code maintainers?

Quick google search suggested me to set TZ to this file and it worked for me. Is there any workaround I could try here.

Quick google search suggested me to set TZ to this file

interesting. i should have taken a note what was the search terms in my quick google search...

what happens if you

container.withEnv("TZ", "UTC")

(instead of the file)?

and what happens if you

container.withEnv("TZ", "/dev/null")

or

container.withEnv("TZ", "absolutely-random-string")

?

It looks like specifying a timezone also works like passing the file

container.withEnv("TZ", "UTC")

If we pass

container.withEnv("TZ", "/dev/null")

It switches to default value UTC

For

container.withEnv("TZ", "absolutely-random-string")

It switches to default value UTC.

If we pass

container.withEnv("TZ", "/dev/null")

It switches to default value UTC

For

container.withEnv("TZ", "absolutely-random-string")

It switches to default value UTC.

so /usr/share/zoneinfo/UTC looks like something meaningful, but actually is "garbage time zone" and causes something (the JVM?) to switch to UTC.

funny

Actually /usr/share/zoneinfo/UTC is not a garbage timezone - pointing to a different file like usr/share/zoneinfo/EET changes to EEST timezone unlike UTC

Praveen2112 · 2023-04-19T17:15:58Z

@findepi AC

Praveen2112 · 2023-04-19T17:47:44Z

Will fix a few test failures

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

raunaqmorarka · 2023-04-19T18:05:36Z

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

It would be clearer to define a BlockTypeCoercion interface for public Block apply(Block block) and then use that in TypeCoercer instead of Function<Block, Block> and here instead of UnaryOperator<Block>.
It will also be easier to track possible implementations.

raunaqmorarka · 2023-04-19T18:07:35Z

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

The from and to types of the coercion would be useful info in the toString

Praveen2112 · 2023-04-20T08:16:20Z

@findepi / @raunaqmorarka AC

Praveen2112 · 2023-04-20T11:20:49Z

@raunaqmorarka AC (first one)

Praveen2112 · 2023-04-26T11:34:43Z

@findepi / @raunaqmorarka Can you PTAL ?

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

findepi · 2023-04-26T12:42:55Z

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

nit: move above all other members

But the other members are static final so do we need to move the the default constructor ?

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

findepi · 2023-04-26T12:50:12Z

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

Alternatively we could support to-varchar directly in io.trino.orc.reader.TimestampColumnReader,

no change requested (i like the current approach)

We could but orc is being used in iceberg connector also so we might be applying coercion (IIRC it is not supported by iceberg)

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

findepi · 2023-04-26T12:54:13Z

...uct-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/configs/ConfigHdp3.java

Quick google search suggested me to set TZ to this file

interesting. i should have taken a note what was the search terms in my quick google search...

what happens if you

container.withEnv("TZ", "UTC")

(instead of the file)?

and what happens if you

container.withEnv("TZ", "/dev/null")

or

container.withEnv("TZ", "absolutely-random-string")

?

Praveen2112 · 2023-04-26T14:44:44Z

@findepi AC

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

findepi · 2023-04-26T15:38:59Z

...uct-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/configs/ConfigHdp3.java

If we pass

container.withEnv("TZ", "/dev/null")

It switches to default value UTC

For

container.withEnv("TZ", "absolutely-random-string")

It switches to default value UTC.

so /usr/share/zoneinfo/UTC looks like something meaningful, but actually is "garbage time zone" and causes something (the JVM?) to switch to UTC.

funny

Praveen2112 · 2023-04-26T16:21:23Z

@findepi AC

Praveen2112 · 2023-04-27T06:36:53Z

Did a few minor changes on TestTimestampCoercer which could help us in #17071

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

Praveen2112 · 2023-04-27T10:13:04Z

@huberty89 AC

Praveen2112 · 2023-05-02T07:10:01Z

@raunaqmorarka Gentle ping

raunaqmorarka

lgtm
I'm wondering why this is specifically supported for ORC and not parquet or other file formats for unpartitioned columns. Does Apache Hive support this for non-orc formats ? If this coercion is to be supported regardless of file format, then we might want the coercion to be done in HivePageSource rather than OrcPageSource

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

Praveen2112 · 2023-05-02T10:26:40Z

why this is specifically supported for ORC and not parquet or other file formats for unpartitioned columns

Parquet doesn't support all types of coercion like ORC - For instance Parquet supports timestamp to varchar coercion but it doesn't support varchar to timestamp coercion, while both are supported by ORC formats. So to start with we are planning for orc and then we could extend it for other file formats.

then we might want the coercion to be done in HivePageSource rather than OrcPageSource

In that case we might need to introduce some sort of an abstraction that allows to Reader to provide the schema and compare with the HivePageSource in case of partitioned tables we compare the global schema with a local partition schema and we apply coercion policy.

Implementing for ORC allows us to create some sort of a testing framework which could be utilized for other formats in the future.

Praveen2112 requested review from atanasenko, ebyhr, findepi, findinpath, huberty89, kokosing and skrzypo987 April 4, 2023 07:04

cla-bot bot added the cla-signed label Apr 4, 2023

skrzypo987 reviewed Apr 4, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 53b81db to 5468428 Compare April 4, 2023 11:44

github-actions bot added hive Hive connector tests:hive labels Apr 4, 2023

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 5468428 to 17eb591 Compare April 11, 2023 13:02

Praveen2112 marked this pull request as ready for review April 11, 2023 13:03

Praveen2112 force-pushed the praveen/timestamp_coercion branch 2 times, most recently from 67ef13a to dcee921 Compare April 12, 2023 16:52

skrzypo987 approved these changes Apr 13, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/timestamp_coercion branch from dcee921 to 969cfb7 Compare April 17, 2023 11:38

Praveen2112 mentioned this pull request Apr 17, 2023

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

Closed

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 969cfb7 to 18210ea Compare April 17, 2023 16:56

findepi reviewed Apr 19, 2023

View reviewed changes

raunaqmorarka reviewed Apr 19, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/timestamp_coercion branch from a3246bd to 174032d Compare April 20, 2023 08:15

Praveen2112 force-pushed the praveen/timestamp_coercion branch 2 times, most recently from 0bc0616 to 3c55156 Compare April 20, 2023 08:18

findepi reviewed Apr 26, 2023

View reviewed changes

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

findepi reviewed Apr 26, 2023

View reviewed changes

findepi approved these changes Apr 26, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 05556a4 to 367940c Compare April 26, 2023 14:32

findepi reviewed Apr 26, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 367940c to 420ef58 Compare April 26, 2023 16:21

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 420ef58 to af5b3d2 Compare April 27, 2023 06:36

Praveen2112 force-pushed the praveen/timestamp_coercion branch from af5b3d2 to 5e298c7 Compare April 27, 2023 08:38

huberty89 reviewed Apr 27, 2023

View reviewed changes

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

Remove deprecated API usage for translating type in HivePageSource

fc6c95e

Praveen2112 force-pushed the praveen/timestamp_coercion branch from 5e298c7 to c888e65 Compare April 27, 2023 10:12

Praveen2112 requested a review from raunaqmorarka May 2, 2023 08:37

raunaqmorarka approved these changes May 2, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/coercions/TestTimestampCoercer.java Outdated Show resolved Hide resolved

Add support for timestamp to varchar coercer in hive tables

60285de

Praveen2112 force-pushed the praveen/timestamp_coercion branch from c888e65 to 60285de Compare May 2, 2023 10:11

Praveen2112 merged commit 5ee5916 into trinodb:master May 2, 2023

github-actions bot added this to the 416 milestone May 2, 2023

colebow mentioned this pull request May 3, 2023

Add Trino 416 release notes #17328

Merged

Praveen2112 mentioned this pull request May 23, 2023

Fix timestamp to varchar coercer in hive tables #17604

Closed

This was referenced Oct 24, 2023

Add timestamp to date schema evolution for hive #19513

Merged

Add varchar to double coercion for hive tables #19517

Merged

Add tinyint/smallint/integer/bigint to double coercion for hive tables #19520

Merged

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

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

Uh oh!

Conversation

Praveen2112 commented Apr 4, 2023

Description

Additional context and related issues

Release notes

Uh oh!

Praveen2112 commented Apr 4, 2023

Uh oh!

skrzypo987 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Praveen2112 commented Apr 11, 2023

Uh oh!

skrzypo987 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Apr 19, 2023

Uh oh!

Praveen2112 commented Apr 19, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Apr 20, 2023

Uh oh!

Praveen2112 commented Apr 20, 2023

Uh oh!

Praveen2112 commented Apr 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Apr 26, 2023