Skip to content

Respect hive.timestamp-precision in Hive views #16258

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

Merged
findepi merged 1 commit into from
Mar 7, 2023
Merged

Respect hive.timestamp-precision in Hive views #16258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ public class HiveMetadata
private final PartitionProjectionService partitionProjectionService;
private final boolean allowTableRename;
private final long maxPartitionDropsPerQuery;
private final HiveTimestampPrecision hiveViewsTimestampPrecision;

public HiveMetadata(
CatalogName catalogName,
Expand All @@ -412,7 +413,8 @@ public HiveMetadata(
DirectoryLister directoryLister,
PartitionProjectionService partitionProjectionService,
boolean allowTableRename,
long maxPartitionDropsPerQuery)
long maxPartitionDropsPerQuery,
HiveTimestampPrecision hiveViewsTimestampPrecision)
{
this.catalogName = requireNonNull(catalogName, "catalogName is null");
this.metastore = requireNonNull(metastore, "metastore is null");
Expand All @@ -439,6 +441,7 @@ public HiveMetadata(
this.partitionProjectionService = requireNonNull(partitionProjectionService, "partitionProjectionService is null");
this.allowTableRename = allowTableRename;
this.maxPartitionDropsPerQuery = maxPartitionDropsPerQuery;
this.hiveViewsTimestampPrecision = requireNonNull(hiveViewsTimestampPrecision, "hiveViewsTimestampPrecision is null");
}

@Override
Expand Down Expand Up @@ -2622,7 +2625,7 @@ private Optional<ConnectorViewDefinition> toConnectorViewDefinition(ConnectorSes
throw new HiveViewNotSupportedException(viewName);
}

ConnectorViewDefinition definition = createViewReader(metastore, session, view, typeManager, this::redirectTable, metadataProvider, hiveViewsRunAsInvoker)
ConnectorViewDefinition definition = createViewReader(metastore, session, view, typeManager, this::redirectTable, metadataProvider, hiveViewsRunAsInvoker, hiveViewsTimestampPrecision)
.decodeViewData(view.getViewOriginalText().get(), view, catalogName);
// use owner from table metadata if it exists
if (view.getOwner().isPresent() && !definition.isRunAsInvoker()) {
Expand Down
11 changes: 8 additions & 3 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public class HiveMetadataFactory
private final long perTransactionFileStatusCacheMaximumSize;
private final PartitionProjectionService partitionProjectionService;
private final boolean allowTableRename;
private final HiveTimestampPrecision hiveViewsTimestampPrecision;

@Inject
public HiveMetadataFactory(
Expand Down Expand Up @@ -138,7 +139,8 @@ public HiveMetadataFactory(
directoryLister,
hiveConfig.getPerTransactionFileStatusCacheMaximumSize(),
partitionProjectionService,
allowTableRename);
allowTableRename,
hiveConfig.getTimestampPrecision());
}

public HiveMetadataFactory(
Expand Down Expand Up @@ -175,7 +177,8 @@ public HiveMetadataFactory(
DirectoryLister directoryLister,
long perTransactionFileStatusCacheMaximumSize,
PartitionProjectionService partitionProjectionService,
boolean allowTableRename)
boolean allowTableRename,
HiveTimestampPrecision hiveViewsTimestampPrecision)
{
this.catalogName = requireNonNull(catalogName, "catalogName is null");
this.skipDeletionForAlter = skipDeletionForAlter;
Expand Down Expand Up @@ -218,6 +221,7 @@ public HiveMetadataFactory(
this.perTransactionFileStatusCacheMaximumSize = perTransactionFileStatusCacheMaximumSize;
this.partitionProjectionService = requireNonNull(partitionProjectionService, "partitionProjectionService is null");
this.allowTableRename = allowTableRename;
this.hiveViewsTimestampPrecision = requireNonNull(hiveViewsTimestampPrecision, "hiveViewsTimestampPrecision is null");
}

@Override
Expand Down Expand Up @@ -266,6 +270,7 @@ public TransactionalMetadata create(ConnectorIdentity identity, boolean autoComm
directoryLister,
partitionProjectionService,
allowTableRename,
maxPartitionDropsPerQuery);
maxPartitionDropsPerQuery,
hiveViewsTimestampPrecision);
}
}
22 changes: 14 additions & 8 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/ViewReaderUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ public static ViewReader createViewReader(
TypeManager typeManager,
BiFunction<ConnectorSession, SchemaTableName, Optional<CatalogSchemaTableName>> tableRedirectionResolver,
MetadataProvider metadataProvider,
boolean runHiveViewRunAsInvoker)
boolean runHiveViewRunAsInvoker,
HiveTimestampPrecision hiveViewsTimestampPrecision)
{
if (isPrestoView(table)) {
return new PrestoViewReader();
Expand All @@ -91,7 +92,8 @@ public static ViewReader createViewReader(
return new HiveViewReader(
new CoralSemiTransactionalHiveMSCAdapter(metastore, coralTableRedirectionResolver(session, tableRedirectionResolver, metadataProvider)),
typeManager,
runHiveViewRunAsInvoker);
runHiveViewRunAsInvoker,
hiveViewsTimestampPrecision);
}

private static CoralTableRedirectionResolver coralTableRedirectionResolver(
Expand Down Expand Up @@ -198,12 +200,14 @@ public static class HiveViewReader
private final HiveMetastoreClient metastoreClient;
private final TypeManager typeManager;
private final boolean hiveViewsRunAsInvoker;
private final HiveTimestampPrecision hiveViewsTimestampPrecision;

public HiveViewReader(HiveMetastoreClient hiveMetastoreClient, TypeManager typeManager, boolean hiveViewsRunAsInvoker)
public HiveViewReader(HiveMetastoreClient hiveMetastoreClient, TypeManager typeManager, boolean hiveViewsRunAsInvoker, HiveTimestampPrecision hiveViewsTimestampPrecision)
{
this.metastoreClient = requireNonNull(hiveMetastoreClient, "hiveMetastoreClient is null");
this.typeManager = requireNonNull(typeManager, "typeManager is null");
this.hiveViewsRunAsInvoker = hiveViewsRunAsInvoker;
this.hiveViewsTimestampPrecision = requireNonNull(hiveViewsTimestampPrecision, "hiveViewsTimestampPrecision is null");
}

@Override
Expand All @@ -218,7 +222,7 @@ public ConnectorViewDefinition decodeViewData(String viewSql, Table table, Catal
List<ViewColumn> columns = rowType.getFieldList().stream()
.map(field -> new ViewColumn(
field.getName(),
typeManager.fromSqlType(getTypeString(field.getType())).getTypeId(),
typeManager.fromSqlType(getTypeString(field.getType(), hiveViewsTimestampPrecision)).getTypeId(),
Optional.empty()))
.collect(toImmutableList());
return new ConnectorViewDefinition(
Expand All @@ -241,7 +245,7 @@ public ConnectorViewDefinition decodeViewData(String viewSql, Table table, Catal

// Calcite does not provide correct type strings for non-primitive types.
// We add custom code here to make it work. Goal is for calcite/coral to handle this
private static String getTypeString(RelDataType type)
private static String getTypeString(RelDataType type, HiveTimestampPrecision timestampPrecision)
{
switch (type.getSqlTypeName()) {
case ROW: {
Expand All @@ -250,7 +254,7 @@ private static String getTypeString(RelDataType type)
// We add the Coral function here to parse data types successfully.
// Goal is to use data type mapping instead of translating to strings
return type.getFieldList().stream()
.map(field -> quoteWordIfNotQuoted(field.getName().toLowerCase(Locale.ENGLISH)) + " " + getTypeString(field.getType()))
.map(field -> quoteWordIfNotQuoted(field.getName().toLowerCase(Locale.ENGLISH)) + " " + getTypeString(field.getType(), timestampPrecision))
.collect(joining(",", "row(", ")"));
}
case CHAR:
Expand All @@ -263,14 +267,16 @@ private static String getTypeString(RelDataType type)
case MAP: {
RelDataType keyType = type.getKeyType();
RelDataType valueType = type.getValueType();
return "map(" + getTypeString(keyType) + "," + getTypeString(valueType) + ")";
return "map(" + getTypeString(keyType, timestampPrecision) + "," + getTypeString(valueType, timestampPrecision) + ")";
}
case ARRAY: {
return "array(" + getTypeString(type.getComponentType()) + ")";
return "array(" + getTypeString(type.getComponentType(), timestampPrecision) + ")";
}
case DECIMAL: {
return "decimal(" + type.getPrecision() + "," + type.getScale() + ")";
}
case TIMESTAMP:
return "timestamp(" + timestampPrecision.getPrecision() + ")";
default:
return type.getSqlTypeName().toString();
}
Expand Down
3 changes: 2 additions & 1 deletion plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,8 @@ public Optional<ConnectorMaterializedViewDefinition> getMaterializedView(Connect
countingDirectoryLister,
1000,
new PartitionProjectionService(hiveConfig, ImmutableMap.of(), new TestingTypeManager()),
true);
true,
HiveTimestampPrecision.DEFAULT_PRECISION);
transactionManager = new HiveTransactionManager(metadataFactory);
splitManager = new HiveSplitManager(
transactionManager,
Expand Down
18 changes: 0 additions & 18 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8241,47 +8241,29 @@ public void testSelectFromPrestoViewReferencingHiveTableWithTimestamps()

assertThat(query(defaultSession, "SELECT ts FROM " + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(defaultSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123456789'")
assertThat(query(defaultSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'");
Copy link
Copy Markdown
Member

@findepi findepi Mar 2, 2023

Choose a reason for hiding this comment

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

The comment indicated the hive_timestamp_nanos.tpch.xxx view should return ts in nano precision (....14.123456789). Why isn't it the case after the change?

Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Mar 2, 2023

Choose a reason for hiding this comment

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

The definition of the Trino view is encoded internally in the JSON saved in the metastore.

{"originalSql":"SELECT *\nFROM\n  test1\n","catalog":"hive","schema":"default","columns":[{"name":"ts","type":"timestamp(3)"}],"owner":"marius","runAsInvoker":false}

I think in this case the assumption from the comment was false.


assertThat(query(millisSession, "SELECT ts FROM " + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'");
assertThat(query(millisSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(nanosSessions, "SELECT ts FROM " + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123456789'")
assertThat(query(nanosSessions, "SELECT ts FROM " + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(nanosSessions, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123456789'")
assertThat(query(nanosSessions, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameDefault)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'");

// Presto view created with config property set to MILLIS and session property set to NANOS
String prestoViewNameNanos = "presto_view_ts_nanos_" + randomNameSuffix();
assertUpdate(nanosSessions, "CREATE VIEW " + prestoViewNameNanos + " AS SELECT * FROM " + tableName);

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(defaultSession, "SELECT ts FROM " + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'")
assertThat(query(defaultSession, "SELECT ts FROM " + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123000000'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(defaultSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123456789'")
assertThat(query(defaultSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123000000'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(millisSession, "SELECT ts FROM " + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'")
assertThat(query(millisSession, "SELECT ts FROM " + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123000000'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(millisSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123'")
assertThat(query(millisSession, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123000000'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(nanosSessions, "SELECT ts FROM " + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123456789'")
assertThat(query(nanosSessions, "SELECT ts FROM " + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123000000'");

// TODO(https://github.com/trinodb/trino/issues/6295) Presto view schema is fixed on creation
// should be: assertThat(query(nanosSessions, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123456789'")
assertThat(query(nanosSessions, "SELECT ts FROM hive_timestamp_nanos.tpch." + prestoViewNameNanos)).matches("VALUES TIMESTAMP '1990-01-02 12:13:14.123000000'");
}

Expand Down
20 changes: 6 additions & 14 deletions .../trino-product-tests/src/main/java/io/trino/tests/product/hive/AbstractTestHiveViews.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,30 +482,22 @@ public void testTimestampHiveView()
unsetSessionProperty("hive_timestamp_nanos.timestamp_precision");

assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123"));
assertThatThrownBy(
// TODO(https://github.com/trinodb/trino/issues/6295) it is not possible to query Hive view with timestamps if hive.timestamp-precision=NANOSECONDS
() -> assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM hive_timestamp_nanos.default.timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123456789"))
).hasMessageContaining("timestamp(9) projected from query view at position 0 cannot be coerced to column [ts] of type timestamp(3) stored in view definition");
assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM hive_timestamp_nanos.default.timestamp_hive_view"))
.containsOnly(row("1990-01-02 12:13:14.123456789"));

setSessionProperty("hive.timestamp_precision", "'MILLISECONDS'");
setSessionProperty("hive_timestamp_nanos.timestamp_precision", "'MILLISECONDS'");

assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123"));
assertThatThrownBy(
// TODO(https://github.com/trinodb/trino/issues/6295) it is not possible to query Hive view with timestamps if hive.timestamp-precision=NANOSECONDS
() -> assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM hive_timestamp_nanos.default.timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123"))
).hasMessageContaining("timestamp(9) projected from query view at position 0 cannot be coerced to column [ts] of type timestamp(3) stored in view definition");
assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM hive_timestamp_nanos.default.timestamp_hive_view"))
.containsOnly(row("1990-01-02 12:13:14.123456789"));

setSessionProperty("hive.timestamp_precision", "'NANOSECONDS'");
setSessionProperty("hive_timestamp_nanos.timestamp_precision", "'NANOSECONDS'");

// TODO(https://github.com/trinodb/trino/issues/6295) timestamp_precision has no effect on Hive views
// should be: assertThat(query("SELECT CAST(ts AS varchar) FROM timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123456789"))
assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123"));
Copy link
Copy Markdown
Member

@findepi findepi Mar 2, 2023

Choose a reason for hiding this comment

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

Was the TODO here incorrect?

Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Mar 2, 2023

Choose a reason for hiding this comment

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

Given that hive.timestamp-precision is set to MILLISECONDS (default value) for the hive catalog, I tend to say that the TODO comment was incorrect.

assertThatThrownBy(
// TODO(https://github.com/trinodb/trino/issues/6295) it is not possible to query Hive view with timestamps if hive.timestamp-precision=NANOSECONDS
() -> assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM hive_timestamp_nanos.default.timestamp_hive_view")).containsOnly(row("1990-01-02 12:13:14.123456789"))
).hasMessageContaining("timestamp(9) projected from query view at position 0 cannot be coerced to column [ts] of type timestamp(3) stored in view definition");
assertThat(onTrino().executeQuery("SELECT CAST(ts AS varchar) FROM hive_timestamp_nanos.default.timestamp_hive_view"))
.containsOnly(row("1990-01-02 12:13:14.123456789"));
}

@Test(groups = HIVE_VIEWS)
Expand Down