adjust datetime str nanoseconds to microseconds#624
adjust datetime str nanoseconds to microseconds#624ankit-eiq wants to merge 1 commit intooasis-open:masterfrom
Conversation
5a2cf05 to
d045088
Compare
|
@chisholm - can you look at this to see if it is worth being merged? |
|
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:
|
There was a problem hiding this comment.
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_nanosecondsfunction to detect and truncate nanosecond-precision timestamps to microseconds - Integrated the adjustment into the
parse_into_datetimefunction 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.
| """ | ||
| 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. |
There was a problem hiding this comment.
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".
| """ | |
| 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. |
| 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 |
There was a problem hiding this comment.
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'.
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.