Skip to content

Fix timestamps in system.runtime.queries#5465

Merged
findepi merged 4 commits intotrinodb:masterfrom
findepi:findepi/queries
Oct 9, 2020
Merged

Fix timestamps in system.runtime.queries#5465
findepi merged 4 commits intotrinodb:masterfrom
findepi:findepi/queries

Conversation

@findepi
Copy link
Member

@findepi findepi commented Oct 8, 2020

Fixes #5462
Fixes #5464

@cla-bot cla-bot bot added the cla-signed label Oct 8, 2020
@findepi findepi force-pushed the findepi/queries branch 2 times, most recently from 06a8c43 to 857ae24 Compare October 8, 2020 09:36
@findepi findepi requested review from losipiuk and martint October 8, 2020 10:05
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit concerned about backward compatibility of this change. Not sure how commonly are those system schemas used by Presto users.

Copy link
Member Author

Choose a reason for hiding this comment

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

See
#4772
#4756
#4753
#4752
i guess system connector simply got missed during that effort, but @electrum @dain may know better.

Copy link
Member

Choose a reason for hiding this comment

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

fair

Copy link
Member

Choose a reason for hiding this comment

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

possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@findepi
Copy link
Member Author

findepi commented Oct 8, 2020

AC

@losipiuk
Copy link
Member

losipiuk commented Oct 8, 2020

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

We can do a sanity check to make the test failure reason obvious in case the system clock happens to be adjusted in between the calls

assertThat(timeAfter).isAfterOrEqualTo(timeBefore);

Copy link
Member Author

Choose a reason for hiding this comment

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

if the clock is adjusted, we still can fail, even if timeAfter >= timeBefore, so not sure how helpful it would be

Copy link
Member

Choose a reason for hiding this comment

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

We used the server time zone for JmxRecordSetProvider since that seems like the right thing to return. If the administrator chooses to run their server in a specific time zone, then that's probably what they want to see when looking at system tables. cc @dain

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@findepi findepi Oct 9, 2020

Choose a reason for hiding this comment

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

Also, the problem with server zone is that it may be a political zone.
Presto engine has no problem representing (point-in-time, zone) pairs.
However, due to how they are formatted for the client, client can no longer distinguish values within
summer→winter DST change.

presto> SELECT
     ->     increment,
     ->     from_unixtime(base + increment * 3600, 'America/Los_Angeles')
     -> FROM (VALUES(1572766200)) t(base)
     -> CROSS JOIN UNNEST(sequence(0, 3)) u(increment);
     ->
 increment |                    _col1
-----------+---------------------------------------------
         0 | 2019-11-03 00:30:00.000 America/Los_Angeles
         1 | 2019-11-03 01:30:00.000 America/Los_Angeles
         2 | 2019-11-03 01:30:00.000 America/Los_Angeles
         3 | 2019-11-03 02:30:00.000 America/Los_Angeles
(4 rows)

Copy link
Member Author

Choose a reason for hiding this comment

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

@findepi
Copy link
Member Author

findepi commented Oct 9, 2020

CI failed -- #4936

@findepi findepi merged commit 4de8430 into trinodb:master Oct 9, 2020
@findepi findepi deleted the findepi/queries branch October 9, 2020 10:55
@findepi findepi mentioned this pull request Oct 9, 2020
10 tasks
@findepi findepi added this to the 344 milestone Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

system.runtime.queries should use timestamp with time zone type Invalid timestamps returned by system.runtime.queries

3 participants