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: DateTime incorrectly encoded when microsecond precision > # digits #175

Merged

Conversation

vlymar
Copy link
Contributor

@vlymar vlymar commented May 29, 2024

@ruslandoga, I found this bug while investigating these errors in my application:

** (Ch.Error) Code: 457. DB::Exception: Value 1716753646. 99856 cannot be parsed as DateTime64 for query parameter '$6' because it isn't parsed completely: only 11 of 17 bytes was parsed: 1716753646. (BAD_QUERY_PARAMETER) (version 23.11.5.29 (official build))

I found a bug in our logic for encoding microsecond values. We left pad the value, but with the default space parameter rather than a 0 😅. See the failing test in the first commit for the repro.

iex(23)> d
~U[2024-05-26 20:00:46.099856Z]
iex(24)> d.microsecond
{99856, 6}

@vlymar vlymar marked this pull request as ready for review May 29, 2024 20:46
@@ -10,7 +14,7 @@
## 0.2.4 (2024-01-29)

- use `ch-#{version}` as user-agent https://github.com/plausible/ch/pull/154
- fix query string escaping for `\t`, `\\`, and `\n` https://github.com/plausible/ch/pull/155
- fix query string escaping for `\t`, `\\`, and `\n` https://github.com/plausible/ch/pull/155
Copy link
Contributor Author

Choose a reason for hiding this comment

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

my formatter did this, lemme know if you'd rather this line not be touched

@ruslandoga
Copy link
Contributor

Thank you!

@ruslandoga ruslandoga merged commit 9324fb9 into plausible:master May 30, 2024
10 checks passed
@ruslandoga
Copy link
Contributor

ruslandoga commented May 30, 2024

I published the fix as v0.2.6: https://hex.pm/packages/ch

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.

2 participants