-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Bump Coral to 1.0.120 #9530
Bump Coral to 1.0.120 #9530
Conversation
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViews.java
Show resolved
Hide resolved
9fb64f8
to
4e22f76
Compare
onHive().executeQuery("DROP VIEW IF EXISTS cast_timestamp_as_decimal_view"); | ||
onHive().executeQuery("CREATE VIEW cast_timestamp_as_decimal_view AS SELECT CAST(a_timestamp as DECIMAL(10,0)) a_cast_timestamp FROM cast_timestamp_as_decimal"); | ||
|
||
// this currently misbehave; Trino returns 631307594 while Hive returns 631261694 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but I tried on our hive and trino, both are 631282394
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
631282394
is yet a third option it seems
@ljfgem for the record, what is "out hive and trino" versions?
and, do you run the test using bin/ptl test run
or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment in code was coming from execution on HIve 1.2.
If I run on HDP3 result is different: 631282394
Hive 3.1.0 (HDP3):
ON Hive: 631282394
ON Trino: 631307594
Hive 1.2:
ON Hive: 631261694
ON Trino: 631307594
It looks it boils down to the fact that value returned to by to_unixtime
in Trino depends on client session timezone.
If I do SET TIME ZONE 'UTC'
then I get 631282394
in Trino (same as Hive 3.1).
To get 631261694 (as Hive 1.2) I need to do SET TIME ZONE 'Asia/Kathmandu'
in Trino (Asia/Kathmandu is timezone used by Hive JVM in product tests).
We cannot do anything about incompatibility in behaviour of Hive 1.2 and 3.1 I guess. But I think we should not use to_unixtime
in rewrites, as the returned values must not depend on session timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think we should not use to_unixtime in rewrites, as the returned values must not depend on session timezone
Or rather we need to wrap argument in with_timezone"(..., 'UTC')
before passing it to to_unixtime
, so it is session timezone agnostic. I do that in linkedin/coral#109.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining! Looks like it is because our hive/trino are using UTC
timezone.
Or rather we need to wrap argument in with_timezone"(..., 'UTC') before passing it to to_unixtime, so it is session timezone agnostic.
But since it doesn't specify the timezone that Hive uses, I think such a wrapper couldn't help, or we need to assume Hive is using UTC
timezone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining! Looks like it is because our hive/trino are using
UTC
timezone.Or rather we need to wrap argument in with_timezone"(..., 'UTC') before passing it to to_unixtime, so it is session timezone agnostic.
But since it doesn't specify the timezone that Hive uses, I think such a wrapper couldn't help, or we need to assume Hive is using
UTC
timezone?
It will help for Hive 3.1 as the semantics of casting timestamp to decimal there is not timezone dependent.
I think we need to live with the fact that 1.2 will behave incorrectly. We have same situation with from_utc_timestamp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, opened a PR: linkedin/coral#169
Feel free to review it, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @losipiuk thanks for the explanation - looks like I fell prey to having both my Hive/Trino setup in UTC and did not catch that.
19d1952
to
f44b7d5
Compare
It also adds test for CAST(timestamp AS DECIMAL()); which apparently is not working great.
f44b7d5
to
c7531ef
Compare
CI: #8691 |
It also adds test for CAST(timestamp AS DECIMAL()); which apparently is
not working great.