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.

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

Internal #582: Remove utc_offset Dependency #9521

Merged
merged 1 commit into from Oct 31, 2023

Conversation

hawkfish
Copy link
Contributor

pg_timezone_names.utc_offset changes over time
so tests should not rely on it. We were lucky for a while but our luck just ran out...

fixes: duckdblabs/duckdb-internal#582

pg_timezone_names.utc_offset changes over time
so tests should not rely on it. We were lucky for a while
but our luck just ran out...
Copy link
Contributor

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

Only though, this seems something we could benefit from having more dynamic tests, something like:

  1. spawn postgres, popolate a table with pg_timezone_names(), save it in a postgres1.db file
  2. spawn duckdb, popolate a table with pg_timezone_names(), save it in a duckdb.db file
  3. spawn popolate a table with pg_timezone_names(), save it in a postgres2.db file
  4. if postgres1.db and postgres2.db have different logical content, we are mid-change / bad luck, bail out
  5. spawn duckdb, check postgres1.db and duckdb.db if they contain the same logical data
  6. take a new timezone + sudo date --set "random date in the next couple of years", iterate a few times

Basically having tests where we aim to mimik postgres behaviour (until proven they do it wrong!)

But this requires some more infrastructure, for now I think we are good with the changes.

@hawkfish
Copy link
Contributor Author

Thanks! Looks good to me.

Only though, this seems something we could benefit from having more dynamic tests, something like:

  1. spawn postgres, popolate a table with pg_timezone_names(), save it in a postgres1.db file
  2. spawn duckdb, popolate a table with pg_timezone_names(), save it in a duckdb.db file
  3. spawn popolate a table with pg_timezone_names(), save it in a postgres2.db file
  4. if postgres1.db and postgres2.db have different logical content, we are mid-change / bad luck, bail out
  5. spawn duckdb, check postgres1.db and duckdb.db if they contain the same logical data
  6. take a new timezone + sudo date --set "random date in the next couple of years", iterate a few times

Basically having tests where we aim to mimik postgres behaviour (until proven they do it wrong!)

But this requires some more infrastructure, for now I think we are good with the changes.

Thanks @carlopi - that's better than me sitting there diffing blocks in my editor(!) The only issue is that we track ICU a bit differently than PG (we have more zones) so we couldn't compare the data files directly, but some sort of outer join and filter would solve that I think.

@Mytherin Mytherin merged commit b57f057 into duckdb:main Oct 31, 2023
45 checks passed
@hawkfish hawkfish deleted the timezone-offsets branch November 1, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants