Skip to content

Conversation

@ankit-eiq
Copy link

@ankit-eiq ankit-eiq commented Mar 17, 2025

This pull request addresses the issue described in #578, where handling of timestamp precision was not correctly adjusted when converting nanoseconds to microseconds.

Changes made:
Implemented logic to adjust nanoseconds to microseconds precision when the timestamp has nanoseconds precision (9 digits followed by 'Z').

Testing:
Updated unit tests to check for timestamp conversion and precision handling with different values (e.g., second, millisecond, nanosecond).
Validated that timestamp precision is correctly truncated for nanosecond precision when required.

@CLAassistant
Copy link

CLAassistant commented Mar 17, 2025

CLA assistant check
All committers have signed the CLA.

@ankit-eiq ankit-eiq force-pushed the fix-datetime-nanosecond-precision branch from 5a2cf05 to d045088 Compare March 18, 2025 08:04
@rpiazza
Copy link
Contributor

rpiazza commented Apr 9, 2025

@chisholm - can you look at this to see if it is worth being merged?

@chisholm
Copy link
Contributor

This PR does not address the larger issue, which is that all implementations have limits, and those limits may or may not satisfy a particular user's requirements. If an implementation's limits are below what a user requires, one can try to extend the limits a little farther, but there are still limits. The next user might need the limit extended again, and again... this is impossible to "fix".

I didn't read that issue as claiming the library has a problem that needs a fix. It pointed out a limitation, and asked how should implementations deal with situations like that. They apparently implemented a workaround which worked for them (unclear if it entailed forking/changing the library at all), but it may not work for all. They could afford to throw away some precision, but others may not be willing/able to do that. They were really asking what should an implementation do when asked to process data which exceeds the implementation's limits. I don't think there's any one right answer to that. The conservative approach is to error out, which is what this library does. That ensures that the user is aware of those limits and doesn't get any nasty surprises. Silently changing data could be considered more dangerous, and if that data is from an established STIX object, it implies we could be changing the object, which is a spec compliance violation.

So perhaps the best thing to do is leave the aforementioned conservative approach as this implementation, and let library users address the limitations in their own individual ways, rather than subjecting all users to a seemingly one-size-fits-all approach which may not actually work for everyone.

If I were to consider this PR, observations include:

  • This implementation truncates extra digits; the issue specifically refers to "rounding", so this implementation may not be exactly equivalent to their approach.
  • It errors on 7, 8 fractional second digits, succeeds with 9 digits, errors with 10. Why so specific to exactly 9 digits? If you're going to implement via string operations, you could handle as many digits as you wanted. Why not generalize?
  • If nanosecond precision is so common that we really need to be able handle it losslessly somehow, I guess that means we'd need to try to find another Python library which can do that, and stop using builtin datetime.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #578 by adding support for timestamp strings with nanosecond precision (9 fractional digits). The implementation introduces a new adjust_nanoseconds function that detects timestamps with exactly 9 fractional digits and truncates them to microseconds (6 digits) before parsing, as Python's datetime module only supports microsecond precision.

Changes:

  • Added adjust_nanoseconds function to detect and truncate nanosecond-precision timestamps to microseconds
  • Integrated the adjustment into the parse_into_datetime function before timestamp parsing
  • Added test cases to verify nanosecond timestamp handling across different precision and constraint combinations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
stix2/utils.py Added adjust_nanoseconds function and integrated it into parse_into_datetime to handle nanosecond-precision timestamps
stix2/test/v21/test_timestamp_precision.py Added 5 test cases to verify nanosecond timestamp handling with various precision/constraint combinations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +216 to +219
"""
This function checks if the timestamp has exactly 9 digits after the decimal
and ends with 'Z'. If it does, it adjusts to microseconds precision (6 digits).
Otherwise, it returns the value unchanged.
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The adjust_nanoseconds function lacks input validation for the value parameter. While the caller (parse_into_datetime) assumes value is a string at this point, the function should be defensive and either check that value is a string or document this assumption in the docstring. Consider adding a type check or updating the docstring to explicitly state: "Args: value (str): A timestamp string to check and adjust".

Suggested change
"""
This function checks if the timestamp has exactly 9 digits after the decimal
and ends with 'Z'. If it does, it adjusts to microseconds precision (6 digits).
Otherwise, it returns the value unchanged.
"""
Adjust a timestamp string from nanoseconds to microseconds precision when applicable.
This function checks if the timestamp has exactly 9 digits after the decimal
and ends with 'Z'. If it does, it adjusts to microseconds precision (6 digits).
Otherwise, it returns the value unchanged.
Args:
value (str): A timestamp string to check and adjust.
Returns:
str: The adjusted timestamp string if nanosecond precision is detected,
otherwise the original value.

Copilot uses AI. Check for mistakes.
Comment on lines +215 to +238
def adjust_nanoseconds(value):
"""
This function checks if the timestamp has exactly 9 digits after the decimal
and ends with 'Z'. If it does, it adjusts to microseconds precision (6 digits).
Otherwise, it returns the value unchanged.
"""

# Return the value if there is no decimal or
# if the frac_seconds_part length is not 10
if '.' not in value:
return value

seconds_part, frac_seconds_part = value.split('.', 1)

if len(frac_seconds_part) != 10:
return value

# Ensure the fractional part has exactly 10 characters (9 digits + 1 'Z')
if frac_seconds_part[:9].isdigit() and frac_seconds_part[9] == 'Z':
# Adjust precision to microseconds (6 digits)
microseconds_part = frac_seconds_part[:6]
value = f'{seconds_part}.{microseconds_part}Z'

return value
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The new adjust_nanoseconds function lacks dedicated unit tests. While it is tested indirectly through test_parse_datetime with the new test cases, adding direct unit tests would improve maintainability and make it easier to verify edge cases. Consider adding tests for: timestamps without decimals, timestamps with various fractional digit counts (1-10+), timestamps with non-digit characters in the fractional part, and timestamps not ending with 'Z'.

Copilot uses AI. Check for mistakes.
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.

4 participants