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

Treat precision as NANOSECONDS for timestamp to be coerced #18003

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Treat precision as NANOSECONDS for timestamp to be coerced #18003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbd03d9
Move coercion related methods to a util class
Praveen2112 Jun 21, 2023
760917f
Use TypeCoercer instead of Function<Block, Block>
Praveen2112 Jun 21, 2023
df2f2eb
For structural type derive from/to types from underlying coercer
Praveen2112 Jun 21, 2023
ee473c0
Derive read column type from the TypeCoercer
Praveen2112 Jun 22, 2023
e864bce
Treat precision as NANOSECONDS for timestamp to be coerced
Praveen2112 Jun 22, 2023
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
279 changes: 5 additions & 274 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSource.java
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import static io.trino.plugin.hive.HivePageSourceProvider.ColumnMapping.toColumnHandles;
import static io.trino.plugin.hive.HivePageSourceProvider.ColumnMappingKind.PREFILLED;
import static io.trino.plugin.hive.HiveSessionProperties.getTimestampPrecision;
import static io.trino.plugin.hive.coercions.CoercionUtils.createTypeFromCoercer;
import static io.trino.plugin.hive.util.HiveBucketing.HiveBucketFilter;
import static io.trino.plugin.hive.util.HiveBucketing.getHiveBucketFilter;
import static io.trino.plugin.hive.util.HiveUtil.getPrefilledColumnValue;
Expand Down Expand Up @@ -564,14 +565,14 @@ public static List<HiveColumnHandle> toColumnHandles(List<ColumnMapping> regular
projectedColumn.getDereferenceIndices(),
projectedColumn.getDereferenceNames(),
fromHiveType,
fromHiveType.getType(typeManager, timestampPrecision));
createTypeFromCoercer(typeManager, fromHiveType, columnHandle.getHiveType(), timestampPrecision));
});

return new HiveColumnHandle(
columnHandle.getBaseColumnName(),
columnHandle.getBaseHiveColumnIndex(),
fromHiveTypeBase,
fromHiveTypeBase.getType(typeManager, timestampPrecision),
createTypeFromCoercer(typeManager, fromHiveTypeBase, columnHandle.getBaseHiveType(), timestampPrecision),
newColumnProjectionInfo,
columnHandle.getColumnType(),
columnHandle.getComment());
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CharCoercer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import io.trino.spi.type.CharType;

import static com.google.common.base.Preconditions.checkArgument;
import static io.trino.plugin.hive.HivePageSource.narrowerThan;
import static io.trino.plugin.hive.coercions.CoercionUtils.narrowerThan;
import static io.trino.spi.type.Chars.truncateToLengthAndTrimSpaces;

public class CharCoercer
Expand Down
389 changes: 389 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 6 additions & 8 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/DecimalCoercers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
import io.trino.spi.type.RealType;
import io.trino.spi.type.VarcharType;

import java.util.function.Function;

import static io.trino.spi.StandardErrorCode.INVALID_ARGUMENTS;
import static io.trino.spi.type.DecimalConversions.doubleToLongDecimal;
import static io.trino.spi.type.DecimalConversions.doubleToShortDecimal;
Expand All @@ -49,7 +47,7 @@ public final class DecimalCoercers
{
private DecimalCoercers() {}

public static Function<Block, Block> createDecimalToDecimalCoercer(DecimalType fromType, DecimalType toType)
public static TypeCoercer<DecimalType, DecimalType> createDecimalToDecimalCoercer(DecimalType fromType, DecimalType toType)
{
if (fromType.isShort()) {
if (toType.isShort()) {
Expand Down Expand Up @@ -148,7 +146,7 @@ protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int pos
}
}

public static Function<Block, Block> createDecimalToDoubleCoercer(DecimalType fromType)
public static TypeCoercer<DecimalType, DoubleType> createDecimalToDoubleCoercer(DecimalType fromType)
{
if (fromType.isShort()) {
return new ShortDecimalToDoubleCoercer(fromType);
Expand Down Expand Up @@ -191,7 +189,7 @@ protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int pos
}
}

public static Function<Block, Block> createDecimalToRealCoercer(DecimalType fromType)
public static TypeCoercer<DecimalType, RealType> createDecimalToRealCoercer(DecimalType fromType)
{
if (fromType.isShort()) {
return new ShortDecimalToRealCoercer(fromType);
Expand Down Expand Up @@ -234,7 +232,7 @@ protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int pos
}
}

public static Function<Block, Block> createDecimalToVarcharCoercer(DecimalType fromType, VarcharType toType)
public static TypeCoercer<DecimalType, VarcharType> createDecimalToVarcharCoercer(DecimalType fromType, VarcharType toType)
{
if (fromType.isShort()) {
return new ShortDecimalToVarcharCoercer(fromType, toType);
Expand Down Expand Up @@ -288,7 +286,7 @@ protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int pos
}
}

public static Function<Block, Block> createDoubleToDecimalCoercer(DecimalType toType)
public static TypeCoercer<DoubleType, DecimalType> createDoubleToDecimalCoercer(DecimalType toType)
{
if (toType.isShort()) {
return new DoubleToShortDecimalCoercer(toType);
Expand Down Expand Up @@ -328,7 +326,7 @@ protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int pos
}
}

public static Function<Block, Block> createRealToDecimalCoercer(DecimalType toType)
public static TypeCoercer<RealType, DecimalType> createRealToDecimalCoercer(DecimalType toType)
{
if (toType.isShort()) {
return new RealToShortDecimalCoercer(toType);
Expand Down
23 changes: 0 additions & 23 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/TimestampCoercer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,29 +48,6 @@ public final class TimestampCoercer

private TimestampCoercer() {}

public static class ShortTimestampToVarcharCoercer
Copy link
Contributor

@vlad-lyutenko vlad-lyutenko Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am a little bit missed, why it's safe to remove it here?
It's because of this change in HivePageSource:

if (fromType instanceof TimestampType timestampType && toType instanceof VarcharType varcharType) {
            // --deleted-- if (timestampType.isShort()) {
            //  --deleted--  return Optional.of(new ShortTimestampToVarcharCoercer(timestampType, varcharType));
            // }
            return Optional.of(new LongTimestampToVarcharCoercer(timestampType, varcharType));
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we were respecting the timestamp precision - so we need a coercer which would could have timestamp read as long or it could be as LongTimestamp - but this change would ensure that we would be reading the timestamp column as a NANOSECOND - so we don't need ShortTimestampToVarcharCoercer

extends TypeCoercer<TimestampType, VarcharType>
{
public ShortTimestampToVarcharCoercer(TimestampType fromType, VarcharType toType)
{
super(fromType, toType);
}

@Override
protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int position)
{
long epochMicros = fromType.getLong(block, position);
long epochSecond = floorDiv(epochMicros, MICROSECONDS_PER_SECOND);
int nanoFraction = floorMod(epochMicros, MICROSECONDS_PER_SECOND) * NANOSECONDS_PER_MICROSECOND;
toType.writeSlice(
blockBuilder,
truncateToLength(
Slices.utf8Slice(
LOCAL_DATE_TIME.format(LocalDateTime.ofEpochSecond(epochSecond, nanoFraction, UTC))),
toType));
}
}

public static class LongTimestampToVarcharCoercer
extends TypeCoercer<TimestampType, VarcharType>
{
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/VarcharCoercer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import io.trino.spi.type.VarcharType;

import static com.google.common.base.Preconditions.checkArgument;
import static io.trino.plugin.hive.HivePageSource.narrowerThan;
import static io.trino.plugin.hive.coercions.CoercionUtils.narrowerThan;
import static io.trino.spi.type.Varchars.truncateToLength;

public class VarcharCoercer
Expand Down
3 changes: 1 addition & 2 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/orc/OrcPageSourceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@
import static io.trino.plugin.hive.HiveSessionProperties.getOrcMaxReadBlockSize;
import static io.trino.plugin.hive.HiveSessionProperties.getOrcStreamBufferSize;
import static io.trino.plugin.hive.HiveSessionProperties.getOrcTinyStripeThreshold;
import static io.trino.plugin.hive.HiveSessionProperties.getTimestampPrecision;
import static io.trino.plugin.hive.HiveSessionProperties.isOrcBloomFiltersEnabled;
import static io.trino.plugin.hive.HiveSessionProperties.isOrcNestedLazy;
import static io.trino.plugin.hive.HiveSessionProperties.isUseOrcColumnNames;
Expand Down Expand Up @@ -366,7 +365,7 @@ else if (column.getBaseHiveColumnIndex() < fileColumns.size()) {
Type readType = column.getType();
if (orcColumn != null) {
int sourceIndex = fileReadColumns.size();
Optional<TypeCoercer<?, ?>> coercer = createCoercer(orcColumn.getColumnType(), readType, getTimestampPrecision(session));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question: We are no longer relay on a session?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we rely on the session to determine the timestamp precision so that we could read the timestamp column with that precision and now we would read them as NANOSECOND irrespective of the precision configured.

Optional<TypeCoercer<?, ?>> coercer = createCoercer(orcColumn.getColumnType(), readType);
if (coercer.isPresent()) {
fileReadTypes.add(coercer.get().getFromType());
columnAdaptations.add(ColumnAdaptation.coercedColumn(sourceIndex, coercer.get()));
Expand Down
13 changes: 3 additions & 10 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/orc/OrcTypeTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,24 @@
package io.trino.plugin.hive.orc;

import io.trino.orc.metadata.OrcType.OrcTypeKind;
import io.trino.plugin.hive.HiveTimestampPrecision;
import io.trino.plugin.hive.coercions.TimestampCoercer.LongTimestampToVarcharCoercer;
import io.trino.plugin.hive.coercions.TimestampCoercer.ShortTimestampToVarcharCoercer;
import io.trino.plugin.hive.coercions.TypeCoercer;
import io.trino.spi.type.TimestampType;
import io.trino.spi.type.Type;
import io.trino.spi.type.VarcharType;

import java.util.Optional;

import static io.trino.orc.metadata.OrcType.OrcTypeKind.TIMESTAMP;
import static io.trino.spi.type.TimestampType.createTimestampType;
import static io.trino.spi.type.TimestampType.TIMESTAMP_NANOS;

public final class OrcTypeTranslator
{
private OrcTypeTranslator() {}

public static Optional<TypeCoercer<? extends Type, ? extends Type>> createCoercer(OrcTypeKind fromOrcType, Type toTrinoType, HiveTimestampPrecision timestampPrecision)
public static Optional<TypeCoercer<? extends Type, ? extends Type>> createCoercer(OrcTypeKind fromOrcType, Type toTrinoType)
{
if (fromOrcType == TIMESTAMP && toTrinoType instanceof VarcharType varcharType) {
TimestampType timestampType = createTimestampType(timestampPrecision.getPrecision());
if (timestampType.isShort()) {
return Optional.of(new ShortTimestampToVarcharCoercer(timestampType, varcharType));
}
return Optional.of(new LongTimestampToVarcharCoercer(timestampType, varcharType));
return Optional.of(new LongTimestampToVarcharCoercer(TIMESTAMP_NANOS, varcharType));
}
return Optional.empty();
}
Expand Down
Loading