Skip to content

Fix SnowSQL rendering negative sub-microsecond timestamps one second too high - #3040

Open
winklemad wants to merge 1 commit into
snowflakedb:mainfrom
winklemad:fix-snowsql-negative-subsecond-timestamp
Open

winklemad wants to merge 1 commit into
snowflakedb:mainfrom
winklemad:fix-snowsql-negative-subsecond-timestamp

Conversation

@winklemad

Copy link
Copy Markdown

Description

The SnowSQL converter (SnowflakeConverterSnowSQL, used by the snowsql CLI and by anyone passing converter_class=SnowflakeConverterSnowSQL) renders a negative timestamp whose only nonzero fraction is sub-microsecond one full second too high — and it can roll the date:

conv = SnowflakeConverterSnowSQL()
conv.set_parameter("TIMESTAMP_NTZ_OUTPUT_FORMAT", "YYYY-MM-DD HH24:MI:SS.FF9")
m = conv.to_python_method("TIMESTAMP_NTZ", {"scale": 9})

m("-2208943503.000000001")  # -> "1900-01-01 12:34:57.999999999"  (should be ...56.999999999)
m("-0.000000001")           # -> "1970-01-01 00:00:00.999999999"  (should be 1969-12-31 23:59:59.999999999)

Root cause

In _extract_timestamp (converter.py), for scale > 6 the integer-seconds part is taken from the value truncated to microseconds and passed to time.gmtime, while _adjust_fraction_of_nanoseconds computes the fraction as max_fraction - frac for negative values — i.e. it borrows a whole second. When the fraction reaches into the microseconds, gmtime floors the non-whole microsecond value and borrows the same second, so the two agree (this is why the existing test_more_timestamps cases such as -2208943503.012000000 are correct). But when the only nonzero fraction is sub-microsecond, the microsecond-truncated value is a whole number, gmtime does not floor, and the borrowed second is dropped — leaving the result one second too high.

Fix

Mirror the fraction's borrow in the integer-seconds part when the fraction borrowed (negative, nonzero) and the microsecond part is whole. It is a class fix: TIMESTAMP_NTZ/TZ/LTZ at scales 7–9, for any pre-1970 UTC timestamp whose fraction is sub-microsecond.

Verification

  • Added test_negative_subsecond_timestamps (matching the existing test_more_timestamps style).
  • The existing test_more_timestamps assertions still pass — including the all-zero-fraction -2208943503.000000000 → 12:34:57.000000000, which the fraction != 0 guard leaves untouched.
  • Cross-checked the output against the default SnowflakeConverter (which is unaffected) across TIMESTAMP_NTZ/LTZ, scales 7–9, and positive/negative/whole/µs/ns fractions — 0 second-or-date mismatches.

The default snowflake.connector converter is not affected (it doesn't use _extract_timestamp for this path). Happy to add a DESCRIPTION.md changelog entry under whatever SNOW ticket you'd like.

…too high

A negative timestamp whose only nonzero fraction is sub-microsecond was
rendered one full second too high, and could roll the date. For negative
values _adjust_fraction_of_nanoseconds borrows a whole second in the fraction
(max_fraction - frac), but the integer-seconds part only reflected that borrow
when gmtime floored a non-whole microsecond value. When the fraction is
entirely sub-microsecond the microsecond part is a whole number, so gmtime did
not floor and the borrowed second was dropped:

  -2208943503.000000001 -> 12:34:57.999999999 (should be 12:34:56.999999999)
  -0.000000001          -> 1970-01-01 00:00:00... (should be 1969-12-31 ...)

Borrow the second in the integer part too when the fraction borrowed and the
microsecond part is whole. Cross-checked against the default SnowflakeConverter
across TIMESTAMP_NTZ/LTZ and scales 7-9.

@snowflake-security-bot snowflake-security-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Snowflake Security Review

Security grade: A — Passed ✅

This PR was classified as LOW risk by the automated pre-screen.

@winklemad

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

This branch has not been deployed

No deployments
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.

1 participant