fix: pad sub-microsecond digits when parsing nanosecond timestamps#3614
Open
anxkhn wants to merge 1 commit into
Open
fix: pad sub-microsecond digits when parsing nanosecond timestamps#3614anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
timestamp_to_nanos and timestamptz_to_nanos read the sub-microsecond digits of the fraction (regex group 3 of ISO_TIMESTAMP_NANO / ISO_TIMESTAMPTZ_NANO) as a bare integer. For a 7- or 8-digit fraction that group is 1 or 2 digits and represents positions 7-8 of the fraction, so it must be right-padded to 3 digits before being interpreted as nanoseconds. Without padding, .1234567 was parsed as +7 ns instead of +700 ns, silently corrupting the value. Right-pad the group to 3 digits with ljust so all fraction widths round-trip correctly, and add 7- and 8-digit cases to the tests, which previously only covered 6- and 9-digit fractions. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the fraction has exactly 7 or 8 digits,
group(3)is only 1 or 2 digits andrepresents fraction positions 7-8, so it has to be right-padded to 3 digits before
being interpreted as a number of nanoseconds. Without padding, the digits are
scaled as if they were the last positions of the fraction, so the value is off by
up to ~999 ns:
6-digit and full 9-digit fractions were already correct, which is why the bug went
unnoticed.
timestamptz_to_nanoshas the identical defect.Are these changes tested?
Yes. Both functions now right-pad
group(3)to 3 digits withljust(3, "0")before it is read as nanoseconds, which is correct for every fraction width (the
group(3) is Nonecase is unchanged:"0".ljust(3, "0") == "000").tests/utils/test_datetime.py::test_timestamp_to_nanosand::test_timestamptz_to_nanospreviously only parametrized 6-digit and 9-digitfractions, so the 7/8-digit boundary was untested. I added a 7-digit
(
.3756127) and an 8-digit (.37561278) case to each. Those new cases failagainst the current code and pass with this fix; the full
tests/utils/test_datetime.pyfile passes (53 tests), andmake lintis clean.Are there any user-facing changes?
Yes, a bug fix: parsing a nanosecond timestamp string with a 7- or 8-digit
fractional second now returns the correct nanosecond value instead of a silently
corrupted one. There is no API change.