Skip to content

Cherry-pick GH actions script from TrinoDB#15649

Merged
arhimondr merged 3 commits intoprestodb:masterfrom
aweisberg:gh_actions_prestodb
Feb 24, 2021
Merged

Cherry-pick GH actions script from TrinoDB#15649
arhimondr merged 3 commits intoprestodb:masterfrom
aweisberg:gh_actions_prestodb

Conversation

@aweisberg
Copy link
Copy Markdown
Contributor

@aweisberg aweisberg commented Jan 28, 2021

Test plan
Look at the test results and see that they are good.
You can see it in action in my repo here https://github.com/aweisberg/presto/pull/3/checks?check_run_id=1965794614 - Finally got a passing run!

It doesn't seem to run in Presto yet, maybe it won't until we have at least one workflow landed.

I think we should run both for a little bit and see how GH actions does.

== NO RELEASE NOTE ==

depends on https://github.com/facebookexternal/presto-facebook/pull/1430

@aweisberg aweisberg force-pushed the gh_actions_prestodb branch from a4c602e to 7deb53f Compare January 28, 2021 23:03
@aweisberg aweisberg requested review from a team and caithagoras January 29, 2021 00:02
@aweisberg aweisberg force-pushed the gh_actions_prestodb branch 3 times, most recently from eb5d58b to 823c3c6 Compare January 29, 2021 17:49
Copy link
Copy Markdown
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

LGTM % nits. Thanks for the work!

@aweisberg
Copy link
Copy Markdown
Contributor Author

I am still fixing some things so give me a bit. Some really annoying time zone issues.

@aweisberg aweisberg force-pushed the gh_actions_prestodb branch from 823c3c6 to 4848c68 Compare January 29, 2021 22:52
@aweisberg
Copy link
Copy Markdown
Contributor Author

@wenleix I updated edb3289fafe0b23ec9f1295315908a2f145f822e 'Add America/Nuuk time zone' and it's more substantial now so worth reviewing. The issue was that I was getting test failures and I found a PR from PrestoSQL that addresses them and also adds a modernizer rule to prohibit using an API for TimeZone and DateTimeZone that can result in unrecognized time zones silently defaulting to GMT.

@aweisberg aweisberg requested a review from wenleix January 29, 2021 23:24
@aweisberg aweisberg force-pushed the gh_actions_prestodb branch 6 times, most recently from b632759 to d8f38c1 Compare February 23, 2021 23:25
aweisberg and others added 3 commits February 23, 2021 18:40
Co-authored-by: Anu Sudarsan <anu.at.infy@gmail.com>
Co-authored-by: David Phillips <david@acz.org>
Co-authored-by: Bin Fan <fanbin103@gmail.com>
Co-authored-by: Grzegorz Kokosiński <grzegorz@starburstdata.com>
Co-authored-by: Jacob I. Komissar <jacob.komissar@starburstdata.com>
Co-authored-by: yashaswaj <jainyashaswa2@gmail.com>
Co-authored-by: Karol Sobczak <karol.sobczak@karolsobczak.com>
Co-authored-by: Łukasz Osipiuk <lukasz@osipiuk.net>
Co-authored-by: Mateusz Gajewski <mateusz.gajewski@gmail.com>
Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
Co-authored-by: Michał Ślizak <michal.slizak+github@gmail.com>
Cherrypick of trinodb/trino#4463
Cherrypick of trinodb/trino#5676

Co-authored-by: David Phillips <david@acz.org>
Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
@Test
public void testNewBlockBuilderLikeForLargeBlockBuilder()
{
if (true) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you instead disable it in the @Test annotation like@Test(enabled = false). That's how we usually disable things

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then it doesn't show up in the count as skipped?
Originally that is what I did, but then it hid the fact we were skipping a test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

interesting. maybe we should be doing that in general then.

@arhimondr arhimondr merged commit e2ceb67 into prestodb:master Feb 24, 2021
@aweisberg
Copy link
Copy Markdown
Contributor Author

Unfortunately this seems to have broken a query in timezone 'America/Dawson'. We have an instance where it is returning different results after the upgrade. Haven't root caused why exactly yet, but I hope to be able to share a reproducer soon.

0.247

presto:ad_delivery> select "to_unixtime"( "date_trunc"( 'DAY', "parse_datetime"( "concat"( '2021-03-02 06:00:00 ', 'America/Dawson' ), 'yyyy-MM-dd HH:mm:ss ZZZ' ) ));
   _col0              
------------
 1.614672E9 
(1 row)

0.248

presto:ad_delivery> select "to_unixtime"( "date_trunc"( 'DAY', "parse_datetime"( "concat"( '2021-03-02 06:00:00 ', 'America/Dawson' ), 'yyyy-MM-dd HH:mm:ss ZZZ' ) ));
    _col0             
-------------
 1.6146684E9 
(1 row)

So they wouldn't group the same way, but this also suggests that the predicates will be slightly off as well. These are off by an hour.

          (
            (
              (active.ds = '2021-03-01') 
              AND (
                time_stop > "to_unixtime"(
                  "date_trunc"(
                    'DAY', 
                    "parse_datetime"(
                      "concat"(
                        '2021-03-01 06:00:00 ', lookup.timezone_name
                      ), 
                      'yyyy-MM-dd HH:mm:ss ZZZ'
                    )
                  )
                )
              )
            ) 
            AND (
              time_start < "to_unixtime"(
                "date_trunc"(
                  'DAY', 
                  "parse_datetime"(
                    "concat"(
                      '2021-03-02 06:00:00 ', lookup.timezone_name
                    ), 
                    'yyyy-MM-dd HH:mm:ss ZZZ'
                  )
                )
              )
            )
          )

The predicate that is inconsistent, Lookup.timezone_name is always 'America/Dawson'. time_stop is either 0 or a value like 1615100879 and one example is of time start (paired with the non-zero time stop) is 1614668884.

https://pastebin.com/FjemTqE4 is the values. Every instance of these was present in 0.247, but was not materialized by the query in 0.248.

It's also possible it's not actually this predicate, but the grouping being different on these two columns:

          IF(
            (
              active.time_stop > "to_unixtime"(
                "date_trunc"(
                  'DAY', 
                  "parse_datetime"(
                    "concat"(
                      '2021-03-02 06:00:00 ', lookup.timezone_name
                    ), 
                    'yyyy-MM-dd HH:mm:ss ZZZ'
                  )
                )
              )
            ), 
            "to_unixtime"(
              "date_trunc"(
                'DAY', 
                "parse_datetime"(
                  "concat"(
                    '2021-03-02 06:00:00 ', lookup.timezone_name
                  ), 
                  'yyyy-MM-dd HH:mm:ss ZZZ'
                )
              )
            ), 
            active.time_stop
          ) time_stop_daily, 
          IF(
            (
              active.time_start < "to_unixtime"(
                "date_trunc"(
                  'DAY', 
                  "parse_datetime"(
                    "concat"(
                      '2021-03-01 06:00:00 ', lookup.timezone_name
                    ), 
                    'yyyy-MM-dd HH:mm:ss ZZZ'
                  )
                )
              )
            ), 
            "to_unixtime"(
              "date_trunc"(
                'DAY', 
                "parse_datetime"(
                  "concat"(
                    '2021-03-01 06:00:00 ', lookup.timezone_name
                  ), 
                  'yyyy-MM-dd HH:mm:ss ZZZ'
                )
              )
            ), 
            active.time_start
          ) time_start_daily 

I should add it cuts both ways and there are values (215) that appeared in 0.248 that didn't appear in 0.247

@aweisberg aweisberg mentioned this pull request Mar 8, 2021
@aweisberg
Copy link
Copy Markdown
Contributor Author

Switching from 2.10.6 to 2.10.7 upgrades the time zone data from 2020a to 2020c which impacts America/Dawson
http://mm.icann.org/pipermail/tz-announce/2020-October/000059.html

Changes to past and future time zone abbreviations and DST flags

     Canada's Yukon, represented by America/Whitehorse and
     America/Dawson, changes its time zone rules from -08/-07 to
     permanent -07 on 2020-11-01, not on 2020-03-08 as 2020a had it.
     This change affects only the time zone abbreviation (MST vs PDT)
     and daylight saving flag for the period between the two dates.
     (Thanks to Andrew G. Smith.)

https://www.joda.org/joda-time/changes-report.html#a2.10.7

Release 2.10.7 – 2020-10-21
Type | Changes | By
-- | -- | --
  | DateTimeZone data updated to version 2020c. Remove systemv and pacificnew time zones as per TZDB changes. | jodastephen
  | Better error message for year-month-day. Fixes #540. | jodastephen
  | Fix localization for Russia. Fixes #533. | AlexMe951

Release 2.10.6 – 2020-04-24
Type | Changes | By
-- | -- | --
  | DateTimeZone data updated to version 2020a. | jodastephen
  | Add localization for Ukraine. Fixes #523. | stari4ek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants