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

Jump to bottom

Variable precision timestamp support for Hive write operations #5283

Merged
merged 6 commits into from
Nov 3, 2020
Merged

Variable precision timestamp support for Hive write operations #5283

merged 6 commits into from
Nov 3, 2020

Conversation

aalbu
Copy link
Member

@aalbu aalbu commented Sep 24, 2020 •
edited
Loading

Write path for timestamps now honors the precision specified in the Hive config (or session). Stats collection is only supported for millisecond precision; when a higher precision is specified, timestamp columns will be ignored for the purpose of collecting stats. The limitation will be addressed when #5170 is implemented.

@cla-bot cla-bot bot added the cla-signed label Sep 24, 2020
@aalbu aalbu added the WIP label Sep 24, 2020
@aalbu aalbu force-pushed the hive-nano-write branch 2 times, most recently from 403b98a to a05eb58 Compare September 24, 2020 16:47
@aalbu aalbu removed the WIP label Sep 25, 2020
@aalbu aalbu requested a review from findepi September 25, 2020 01:38
@findepi
Copy link
Member

findepi commented Sep 25, 2020

The data gets written with abundant precision

presto:tpch> set session hive.timestamp_precision = 'MILLISECONDS';
SET SESSION
presto:tpch> create table t(a) as SELECT localtimestamp(9);
CREATE TABLE: 1 row

presto:tpch> DESC t;
 Column |     Type     | Extra | Comment
--------+--------------+-------+---------
 a      | timestamp(3) |       |

but then:

presto:tpch> set session hive.timestamp_precision = 'NANOSECONDS';
SET SESSION
presto:tpch> SELECT * FROM t;
               a
-------------------------------
 2020-09-25 16:04:25.672304000

findepi
findepi reviewed Sep 25, 2020
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

initial feedback

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveFileFormats.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved
presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveStorageFormats.java Outdated Show resolved Hide resolved
presto-rcfile/src/main/java/io/prestosql/rcfile/TimestampUtils.java Outdated Show resolved Hide resolved
presto-rcfile/src/main/java/io/prestosql/rcfile/TimestampUtils.java Outdated Show resolved Hide resolved
findepi
findepi reviewed Sep 28, 2020
View reviewed changes
presto-main/src/main/java/io/prestosql/testing/MaterializedResult.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/testing/MaterializedResult.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveFileFormats.java Outdated Show resolved Hide resolved
presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/type/TimestampRepresentations.java Outdated Show resolved Hide resolved
presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/type/TimestampRepresentations.java Outdated Show resolved Hide resolved
presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveStorageFormats.java Outdated Show resolved Hide resolved
presto-rcfile/src/main/java/io/prestosql/rcfile/TimestampUtils.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveFileFormats.java Outdated Show resolved Hide resolved
@findepi
Copy link
Member

findepi commented Sep 28, 2020

@dain a question to you -- a05fb8b#r495844291

@electrum a question to you -- a05fb8b#r495843768

(posting here, as this could have been lost in the noise)

electrum
electrum requested changes Sep 29, 2020
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

See comments (mostly minor)

@findepi findepi mentioned this pull request Sep 30, 2020
Closed
findepi
findepi approved these changes Sep 30, 2020
View reviewed changes
presto-main/src/main/java/io/prestosql/testing/MaterializedResult.java Outdated Show resolved Hide resolved
presto-spi/src/main/java/io/prestosql/spi/type/Timestamps.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/testing/MaterializedResult.java Outdated Show resolved Hide resolved
presto-orc/src/test/java/io/prestosql/orc/OrcTester.java Outdated Show resolved Hide resolved
presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveStorageFormats.java Outdated Show resolved Hide resolved
presto-rcfile/src/main/java/io/prestosql/rcfile/TimestampUtils.java Outdated Show resolved Hide resolved
@aalbu aalbu force-pushed the hive-nano-write branch 3 times, most recently from 8b3b684 to 2b157a5 Compare October 1, 2020 16:19
@losipiuk losipiuk mentioned this pull request Oct 2, 2020
Merged
findepi
findepi approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

%

presto-orc/src/test/java/io/prestosql/orc/OrcTester.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveWriterFactory.java Outdated
@@ -522,7 +523,7 @@ else if (insertExistingPartitionsBehavior == InsertExistingPartitionsBehavior.ER
}

List<Type> types = dataColumns.stream()
.map(column -> column.getHiveType().getType(typeManager))
.map(column -> column.getHiveType().getType(typeManager, getTimestampPrecision(session).getPrecision()))
Copy link
Member

Choose a reason for hiding this comment

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

getType(TypeManager) overload is still used; for example io.prestosql.plugin.hive.HiveMetadata#finishCreateTable, io.prestosql.plugin.hive.HiveMetadata#finishInsert call sites look relevant

please make sure there is an issue covering cleanup of the other ones

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcFileWriterFactory.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/FieldSetterFactory.java Outdated Show resolved Hide resolved
presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveStorageFormats.java Outdated Show resolved Hide resolved
presto-rcfile/src/main/java/io/prestosql/rcfile/TimestampUtils.java Outdated Show resolved Hide resolved
@aalbu aalbu mentioned this pull request Oct 7, 2020
Closed
@findepi findepi mentioned this pull request Oct 13, 2020
Merged
@jirassimok jirassimok mentioned this pull request Oct 26, 2020
Merged
@findepi findepi requested a review from electrum October 30, 2020 09:35
electrum
electrum approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

A few minor comments. @dain should look at the potential RCBinary writer regression

presto-spi/src/main/java/io/prestosql/spi/type/Timestamps.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved
presto-rcfile/src/main/java/io/prestosql/rcfile/TimestampUtils.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved
@aalbu aalbu force-pushed the hive-nano-write branch 2 times, most recently from b2158ed to b483624 Compare November 3, 2020 00:50
@electrum electrum merged commit 7cf9f39 into trinodb:master Nov 3, 2020
@electrum
Copy link
Member

electrum commented Nov 3, 2020

Thanks!

@findepi findepi added this to the 346 milestone Nov 3, 2020
@electrum electrum mentioned this pull request Nov 10, 2020
Closed
10 tasks
@findinpath findinpath mentioned this pull request Sep 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum approved these changes

@findepi findepi findepi approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
346
Development

Successfully merging this pull request may close these issues.
3 participants
@aalbu @findepi @electrum