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

tzinfo objects with sub-minute offsets are not supported (e.g. UTC+05:53:28) #49538

Closed
jamesh mannequin opened this issue Feb 17, 2009 · 23 comments
Closed

tzinfo objects with sub-minute offsets are not supported (e.g. UTC+05:53:28) #49538

jamesh mannequin opened this issue Feb 17, 2009 · 23 comments
Assignees
Labels
3.7 (EOL) end of life easy extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@jamesh
Copy link
Mannequin

jamesh mannequin commented Feb 17, 2009

BPO 5288
Nosy @gvanrossum, @tim-one, @mdickinson, @abalkin, @vstinner, @benjaminp
PRs
  • Closes issue bpo-5288: Allow tzinfo objects with sub-minute offsets. #2896
  • bpo-15873: Implement [date][time].fromisoformat #4699
  • Files
  • issue5288-proto.diff
  • issue5288.diff: patch for the C code
  • old-datetimemodule.c.gcov: Test coverage before change
  • new-datetimemodule.c.gcov: Test coverage after change
  • issue5288a.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/abalkin'
    closed_at = <Date 2017-07-31.14:27:55.922>
    created_at = <Date 2009-02-17.04:33:24.503>
    labels = ['extension-modules', 'easy', 'type-feature', '3.7']
    title = 'tzinfo objects with sub-minute offsets are not supported (e.g. UTC+05:53:28)'
    updated_at = <Date 2017-12-13.02:13:30.662>
    user = 'https://bugs.python.org/jamesh'

    bugs.python.org fields:

    activity = <Date 2017-12-13.02:13:30.662>
    actor = 'p-ganssle'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2017-07-31.14:27:55.922>
    closer = 'belopolsky'
    components = ['Extension Modules']
    creation = <Date 2009-02-17.04:33:24.503>
    creator = 'jamesh'
    dependencies = []
    files = ['17844', '17865', '17874', '17875', '17897']
    hgrepos = []
    issue_num = 5288
    keywords = ['easy']
    message_count = 23.0
    messages = ['82299', '106374', '108606', '109151', '109262', '109347', '109348', '109504', '109508', '109510', '174494', '174742', '174760', '248468', '251870', '299240', '299254', '299330', '299377', '299389', '299394', '299395', '299557']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'tim.peters', 'jamesh', 'mark.dickinson', 'belopolsky', 'vstinner', 'benjamin.peterson', 'Fergus.Noble']
    pr_nums = ['2896', '4699']
    priority = None
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5288'
    versions = ['Python 3.7']

    @jamesh
    Copy link
    Mannequin Author

    jamesh mannequin commented Feb 17, 2009

    The datetime module does not support time zones whose offset from UTC is
    not an integer number of minutes.

    The Olson time zone database (used by most UNIX systems and Mac OS X)
    has a number of time zones with historic offsets that use second
    resolution (from before those locations switched to a rounded offset
    from GMT).

    If you are working purely with the Python date time module, you can
    round these offsets to whole numbers of minutes and get consistent
    results (this is what the pytz module does), there are problems if you
    want to interact with other programs using the Olson database.

    As an example, PostgreSQL can return dates with sub-minute UTC offsets
    and it would be nice to be able to represent them exactly.

    The documentation doesn't explain why offsets need to be a whole number
    of minutes, and the interface (returning timedeltas) doesn't look like
    it'd need changing to support second offsets. I wouldn't expect any
    more complexity in the code to support them either.

    @jamesh jamesh mannequin added the extension-modules C modules in the Modules dir label Feb 17, 2009
    @abalkin abalkin self-assigned this May 21, 2010
    @abalkin abalkin added the type-feature A feature request or enhancement label May 21, 2010
    @abalkin
    Copy link
    Member

    abalkin commented May 24, 2010

    As far as I can tell, the TZ offset code can be simplified by eliminating conversion from timedelta to int and back in utcoffset() and dst() methods of time and datetime objects.

    The only reason for the restriction that I can think of is that some text representation of datetime only provide 4 digits for timezone. The behavior of formatting functions, can be preserved while allowing fractional minute offsets elsewhere.

    On the other hand, given that this issue did not see a single response in more than a year, it seems that there is little interest in fixing it.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 25, 2010

    This proved to require a lot of changes to C implementation because most of timezone arithmetics is done using integer operations with offset in minutes. It is easy, however to do this in pure python implementation which can be found at http://svn.python.org/view/*checkout*/sandbox/branches/py3k-datetime/datetime.py .

    Marking this "easy" to attract volunteers who may want to do Python prototype.

    @abalkin abalkin added the easy label Jun 25, 2010
    @abalkin
    Copy link
    Member

    abalkin commented Jul 2, 2010

    I am attaching a patch against sandbox version of datetime.py. With this patch, there is a single place where subminute offset is rejected, _check_utc_offset() function. I have also added "whole minute" asserts in places where sub-minute part of the offset is discarded or assumed 0.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 4, 2010

    I am attaching a rough patch which removes timedelta -> int minutes -> timedelta round trips from utcoffset handling code. I think the result is an improvement, but needs more polishing.

    Mark,

    Do you think this is worth pursuing? I am not intending to add support for sub-minute offsets yet, just pass offsets around as timedeltas internally without unnecessary conversions to int and back.

    @abalkin abalkin removed the easy label Jul 4, 2010
    @abalkin
    Copy link
    Member

    abalkin commented Jul 5, 2010

    I am attaching a test coverage file for the patched datetimemodule.c.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 5, 2010

    It looks like I attached coverage for the original file. See new-datetimemodule.c.gcov for coverage after the change.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 7, 2010

    Uploaded bpo-5288.diff to Rietveld:

    http://codereview.appspot.com/1698050

    @abalkin
    Copy link
    Member

    abalkin commented Jul 7, 2010

    issue5288a.diff addressed comments from Rietveld.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 8, 2010

    C code changes eliminating round-trips between timdelta and int offsets committed in r82642 and Python code changes committed to sandbox in r82641.

    Now the requested behavior change is easy and I am about +0.5 on relaxing the offset checks to allow seconds in the offsets and about +0.1 on removing the checks altogether and allowing arbitrary timedeltas even over 1 day.

    The only substantive question in my mind is how to handle printing in formats like %z which only provide 4 digits for UTC offset. Two options seem reasonable: a) truncate sub-minute offsets; and b) follow RFC 3339 recommendation and translate the time into the nearest whole-minute timezone.

    I am slightly is favor of (a), but this whole problem may be a reason to reject this RFE.

    @abalkin abalkin added the easy label Jul 8, 2010
    @FergusNoble
    Copy link
    Mannequin

    FergusNoble mannequin commented Nov 2, 2012

    Digging up an old issue but I am also interested in seeing this enhancement. Specifically to represent GPS time which is (currently) 16 seconds ahead of UTC.

    @abalkin
    Copy link
    Member

    abalkin commented Nov 4, 2012

    Is it practical to implement GPS time as datetime given that we don't have support for leap seconds?

    @FergusNoble
    Copy link
    Mannequin

    FergusNoble mannequin commented Nov 4, 2012

    GPS time doesn't include leap seconds so I think datetime is a good representation. If datetime doesn't know about leap seconds then there would still be some issues with finding the timedelta between a GPS time and a UTC time straddling a leap second but I guess a similar issue also exists with two UTC times.

    For my application all the times I am dealing with are in a short period and will have the same UTC offset so its a little easier, I can probably avoid most of these issues.

    However, wouldn't it be possible to implement the general case with a non-constant utcoffset function (and new fromutc implementation) in the tzinfo class? Of course there is no way to properly handle UTC times more than 6 months or so in the future...

    @abalkin abalkin removed their assignment Jun 30, 2014
    @tim-one
    Copy link
    Member

    tim-one commented Aug 12, 2015

    The only reason for the restriction that
    I can think of is that some text representation
    of datetime only provide 4 digits for timezone.

    There never was a compelling reason. It was simply intended to help catch programming errors for a (at the time) brand new feature, and one where no concrete timezone support was supplied at first. Lots of people were writing their own tzinfo subclasses, and nobody at the time was, e.g., volunteering to wrap the Olson database.

    I'm in favor of removing all restrictions on offsets. Speed is of minor concern here - if it simplifies the code to delegate all offset arithmetic to classic datetime +/- timedelta operations, fine.

    String representations are a mess. If some popular standard doesn't cater to sub-minute (or sub-second!) offsets, fine, make up a format consistent with what such a standard "would have" defined had it addressed the issue, and document that if a programmer picks a timezone whose offsets go beyond what that standard supports, tough luck. Then Python will give something sensible Python can live with, but won't try to hide that what they're doing does in fact go beyond what the standard supports.

    @abalkin
    Copy link
    Member

    abalkin commented Sep 29, 2015

    In my PEP-495 work (see bpo-24773,) I relaxed the offset checks to allow any integer number of *seconds*. This was necessary to support all timezones in the Olson database.

    With respect to string representation of such offset, I would like to bring up bpo-24954. We don't even have support for printing regular offsets in ISO format.

    @gvanrossum
    Copy link
    Member

    I haven't reviewed the code, but given Tim Peters' response (which matches my own gut feeling) we should just allow/support tz offsets with second-precision (and deal with the default formatting issues in a backwards compatible way, of course). Hope the patches aren't too stale -- good luck moving them to GitHub!

    @abalkin abalkin added the 3.7 (EOL) end of life label Jul 26, 2017
    @abalkin
    Copy link
    Member

    abalkin commented Jul 26, 2017

    Most of the code supporting arbitrary offsets has already been committed. The only part left was to remove the checks and implement printing.

    @abalkin abalkin self-assigned this Jul 26, 2017
    @abalkin
    Copy link
    Member

    abalkin commented Jul 27, 2017

    In PR 2896, I've modified %z formatting code to output sub-minute data if present. I think %z parsing should be also modified to accept sub-minute data, but I would like to do it in the context of bpo-24954. Thoughts?

    @vstinner
    Copy link
    Member

    James Henstridge:

    The Olson time zone database (used by most UNIX systems and Mac OS X)
    has a number of time zones with historic offsets that use second
    resolution (from before those locations switched to a rounded offset
    from GMT).

    Ok for increasing the resolution to seconds. But PR 2896 increases the resolution to microseconds. What is the rationale for supporting microseconds?

    I would prefer to only accept microseconds=0.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 28, 2017

    Victor,

    Tim called for removal of all restrictions on the offsets. See msg248468. I left the range restriction intact because we have some algorithms that rely on that, but in general I agree with Tim. There is nothing to be gained from restricting the offsets. It is just some extra code to enforce gratuitous limitations.

    Note that Olson's database limits its precision to seconds for historical reasons. The mean solar time offsets that they record are known to subsecond precision.

    I did add a few lines of code to support subsecond formatting, but at some point we should be able to unify timedelta and timezone formatting.

    @vstinner
    Copy link
    Member

    I did add a few lines of code to support subsecond formatting, but at some point we should be able to unify timedelta and timezone formatting.

    My concern is that it makes timestamp parsing more complex because we would have to handle the theorical case of timezone with microsecond precision.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 28, 2017

    My concern is that it makes timestamp parsing more complex

    To the contrary. The timezone field can now be parsed the same way as the time field plus the sign.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 31, 2017

    New changeset 018d353 by Alexander Belopolsky in branch 'master':
    Closes issue bpo-5288: Allow tzinfo objects with sub-minute offsets. (bpo-2896)
    018d353

    @abalkin abalkin closed this as completed Jul 31, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Sep 27, 2023
    The previous code did not work because timedelta arithmetic does not do timezone conversions. The result of adding a timedelta to a datetime has the same fixed UTC offset as the original datetime. This resulted in the correct timezone not being applied by `Timestamp.ToDatetime(tz)` whenever the UTC offset for the timezone at the represented moment was not the same as the UTC offset for that timezone at the epoch.
    
    Instead, construct the datetime directly from the seconds part of the timestamp. It would be nice to include the nanoseconds as well (truncated to datetime's millisecond precision, but without unnecessary loss of precision). However, that doesn't work, since there isn't a way to construct a datetime from an integer-milliseconds timestamp, just float-seconds, which doesn't allow some datetime values to round-trip correctly. (This does assume that `tzinfo.utcoffset(dt).microseconds` is always zero, and that the value returned by `tzinfo.utcoffset(dt)` doesn't change mid-second. Neither of these is necessarily the case (see python/cpython#49538), though I'd hope they hold in practice.)
    
    This does take some care to still handle non-standard Timestamps where nanos is more than 1,000,000,000 (i.e. more than a second), since previously ToDatetime handled that, as do the other To* conversion methods.
    
    The bug doesn't manifest for UTC (or any fixed-offset timezone), so it can be worked around in a way that will be correct before and after the fix by replacing `ts.ToDatetime(tz)` with `ts.ToDatetime(datetime.timezone.utc).astimezone(tz)`.
    
    PiperOrigin-RevId: 567478198
    copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Sep 27, 2023
    The previous code did not work because timedelta arithmetic does not do timezone conversions. The result of adding a timedelta to a datetime has the same fixed UTC offset as the original datetime. This resulted in the correct timezone not being applied by `Timestamp.ToDatetime(tz)` whenever the UTC offset for the timezone at the represented moment was not the same as the UTC offset for that timezone at the epoch.
    
    Instead, construct the datetime directly from the seconds part of the timestamp. It would be nice to include the nanoseconds as well (truncated to datetime's millisecond precision, but without unnecessary loss of precision). However, that doesn't work, since there isn't a way to construct a datetime from an integer-milliseconds timestamp, just float-seconds, which doesn't allow some datetime values to round-trip correctly. (This does assume that `tzinfo.utcoffset(dt).microseconds` is always zero, and that the value returned by `tzinfo.utcoffset(dt)` doesn't change mid-second. Neither of these is necessarily the case (see python/cpython#49538), though I'd hope they hold in practice.)
    
    This does take some care to still handle non-standard Timestamps where nanos is more than 1,000,000,000 (i.e. more than a second), since previously ToDatetime handled that, as do the other To* conversion methods.
    
    The bug doesn't manifest for UTC (or any fixed-offset timezone), so it can be worked around in a way that will be correct before and after the fix by replacing `ts.ToDatetime(tz)` with `ts.ToDatetime(datetime.timezone.utc).astimezone(tz)`.
    
    PiperOrigin-RevId: 567478198
    copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Sep 28, 2023
    The previous code did not work because timedelta arithmetic does not do timezone conversions. The result of adding a timedelta to a datetime has the same fixed UTC offset as the original datetime. This resulted in the correct timezone not being applied by `Timestamp.ToDatetime(tz)` whenever the UTC offset for the timezone at the represented moment was not the same as the UTC offset for that timezone at the epoch.
    
    Instead, construct the datetime directly from the seconds part of the timestamp. It would be nice to include the nanoseconds as well (truncated to datetime's millisecond precision, but without unnecessary loss of precision). However, that doesn't work, since there isn't a way to construct a datetime from an integer-milliseconds timestamp, just float-seconds, which doesn't allow some datetime values to round-trip correctly. (This does assume that `tzinfo.utcoffset(dt).microseconds` is always zero, and that the value returned by `tzinfo.utcoffset(dt)` doesn't change mid-second. Neither of these is necessarily the case (see python/cpython#49538), though I'd hope they hold in practice.)
    
    This does take some care to still handle non-standard Timestamps where nanos is more than 1,000,000,000 (i.e. more than a second), since previously ToDatetime handled that, as do the other To* conversion methods.
    
    The bug doesn't manifest for UTC (or any fixed-offset timezone), so it can be worked around in a way that will be correct before and after the fix by replacing `ts.ToDatetime(tz)` with `ts.ToDatetime(datetime.timezone.utc).astimezone(tz)`.
    
    PiperOrigin-RevId: 567478198
    copybara-service bot pushed a commit to protocolbuffers/protobuf that referenced this issue Sep 28, 2023
    The previous code did not work because timedelta arithmetic does not do timezone conversions. The result of adding a timedelta to a datetime has the same fixed UTC offset as the original datetime. This resulted in the correct timezone not being applied by `Timestamp.ToDatetime(tz)` whenever the UTC offset for the timezone at the represented moment was not the same as the UTC offset for that timezone at the epoch.
    
    Instead, construct the datetime directly from the seconds part of the timestamp. It would be nice to include the nanoseconds as well (truncated to datetime's millisecond precision, but without unnecessary loss of precision). However, that doesn't work, since there isn't a way to construct a datetime from an integer-milliseconds timestamp, just float-seconds, which doesn't allow some datetime values to round-trip correctly. (This does assume that `tzinfo.utcoffset(dt).microseconds` is always zero, and that the value returned by `tzinfo.utcoffset(dt)` doesn't change mid-second. Neither of these is necessarily the case (see python/cpython#49538), though I'd hope they hold in practice.)
    
    This does take some care to still handle non-standard Timestamps where nanos is more than 1,000,000,000 (i.e. more than a second), since previously ToDatetime handled that, as do the other To* conversion methods.
    
    The bug doesn't manifest for UTC (or any fixed-offset timezone), so it can be worked around in a way that will be correct before and after the fix by replacing `ts.ToDatetime(tz)` with `ts.ToDatetime(datetime.timezone.utc).astimezone(tz)`.
    
    PiperOrigin-RevId: 569012931
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life easy extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants