Treat precision as NANOSECONDS for timestamp to be coerced #18003
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Praveen2112
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
from
0c657ad to
b941d64
Compare
| @@ -48,29 +48,6 @@ public final class TimestampCoercer | |||
|
|
|||
| private TimestampCoercer() {} | |||
|
|
|||
| public static class ShortTimestampToVarcharCoercer | |||
There was a problem hiding this comment.
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));
}
There was a problem hiding this comment.
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
Praveen2112
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
from
b941d64 to
e520eb3
Compare
Praveen2112
requested review from
findepi,
atanasenko,
huberty89,
phd3,
kokosing and
krvikash
krvikash
reviewed
Jul 3, 2023
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
Praveen2112
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
from
e520eb3 to
c34eeea
Compare
|
@krvikash Thanks for the review. Addressed the comments |
Praveen2112
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
2 times, most recently
from
eb18007 to
6be4816
Compare
findepi
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
from
6be4816 to
a2cf304
Compare
|
squashed to make reviewable |
|
skimmed. looks directionally correct |
Praveen2112
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
2 times, most recently
from
656b77a to
d3bd7e2
Compare
huberty89
reviewed
Jul 5, 2023
| @@ -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)); | |||
There was a problem hiding this comment.
Just a question: We are no longer relay on a session?
There was a problem hiding this comment.
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.
krvikash
reviewed
Jul 5, 2023
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
This will be irrespective of the precision configured or specified as session property.
Praveen2112
force-pushed
the
praveen/nano_second_precision_for_coerced_timestamp_column
branch
from
d3bd7e2 to
e864bce
Compare
|
@krvikash AC |
vlad-lyutenko
approved these changes
Jul 10, 2023
krvikash
approved these changes
Jul 11, 2023
raunaqmorarka
approved these changes
Jul 13, 2023
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This will be irrespective of the precision configured or specified as
session property.
Additional context and related issues
Depends on #17900.
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: