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

Fix Issue1070 - Unix time natural_language: "now" and other commands are generating local time #1071

Conversation

idiom-bytes
Copy link
Member

@idiom-bytes idiom-bytes commented May 20, 2024

Fixes #1070

We're running into issues where UnixTimeS and UnixTimeMs respond w/ timestamps that should be in UTC, but in reality are in local time. This is causing all sorts of issues in the data.

Changes proposed in this PR:

  • Not yet fixed, I have created a test to show the problem
  • Using "now" should generate the correct timestamp (not local time, but UTC)

@idiom-bytes idiom-bytes marked this pull request as draft May 20, 2024 21:00
@idiom-bytes idiom-bytes requested a review from calina-c May 20, 2024 21:00
@idiom-bytes idiom-bytes changed the title Fix Issue1070 - Unixtime natural_language: "now" is wrong Fix Issue1070 - Unix time natural_language: "now" and other commands are generating local time May 20, 2024
@calina-c calina-c force-pushed the issue1070-unixtime-natural-language-now-wrong branch from 75c6c5e to 5d3b4e8 Compare May 21, 2024 07:28
@calina-c
Copy link
Contributor

I fixed the localisation, but I don't think the test is accurate to describe the problem.
https://www.geeksforgeeks.org/python-time-time_ns-method/
time.time() is also platform dependent, so they will be equal.

@KatunaNorbert is time.time() or time.gmtime() used in any relevant way for the ETL? If so, I think we need to reconsider those as well.

@calina-c
Copy link
Contributor

Ref the failing CI tests, @idiom-bytes do you want to keep/adjust this test in particular, or will it suffice to add the timezone? I can (and probably will) add some tests using freezegun, which is much safer wrt. defining the time in tests (I think it already is a dependency)

@calina-c calina-c force-pushed the issue1070-unixtime-natural-language-now-wrong branch from 8f7fd34 to 35c0f0a Compare May 21, 2024 09:04
@KatunaNorbert
Copy link
Member

@KatunaNorbert is time.time() or time.gmtime() used in any relevant way for the ETL? If so, I think we need to reconsider those as well.

From what I see they are not used inside the ETL

@KatunaNorbert
Copy link
Member

Tested it and the end date it's fixed now and it uses the local time to fetch from subgraph.
Both of the bronze tables are now updating, but bronze_predictions table it's a little bit behind the pdr_predictions table. Slots tables are up to date.

@idiom-bytes
Copy link
Member Author

idiom-bytes commented May 21, 2024

As I understood it, replace(tzinfo=timezone.utc) behaves differently than astimezone(pytz.utc), it replaces the "UTC" tz unit, but does not actually transform the value. To do that, you have to use astimezone.

my challenge, is that I didn't understand if "now" was to return local time or UTC time, and what the expectations from the lib was.... thank you for explaining @calina-c

  • the system should use UTC
  • "now" will return the right time in UTC (which will also transform to the right local time)

@idiom-bytes idiom-bytes marked this pull request as ready for review May 21, 2024 14:48
@idiom-bytes idiom-bytes merged commit 35b3c94 into issue685-duckdb-integration May 21, 2024
3 of 4 checks passed
@idiom-bytes idiom-bytes deleted the issue1070-unixtime-natural-language-now-wrong branch May 21, 2024 14:50
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.

3 participants